Make sure we fully remove child context menu items when deleting a parent.

BUG=49742
TEST=Follow steps in the bug report

Review URL: http://codereview.chromium.org/3081009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@54628 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 91 insertions, 20 deletions

diff --git a/chrome/browser/extensions/extension_menu_manager.cc b/chrome/browser/extensions/extension_menu_manager.cc
index cafb95a..ff87f94 100644
--- a/chrome/browser/extensions/extension_menu_manager.cc
+++ b/chrome/browser/extensions/extension_menu_manager.cc
@@ -37,16 +37,6 @@ ExtensionMenuItem::~ExtensionMenuItem() {
   STLDeleteElements(&children_);
 }
 
-bool ExtensionMenuItem::RemoveChild(const Id& child_id) {
-  ExtensionMenuItem* child = ReleaseChild(child_id, true);
-  if (child) {
-    delete child;
-    return true;
-  } else {
-    return false;
-  }
-}
-
 ExtensionMenuItem* ExtensionMenuItem::ReleaseChild(const Id& child_id,
                                                    bool recursive) {
   for (List::iterator i = children_.begin(); i != children_.end(); ++i) {
@@ -251,24 +241,41 @@ bool ExtensionMenuManager::RemoveContextMenuItem(
   }
 
   bool result = false;
+  std::set<ExtensionMenuItem::Id> items_removed;
   ExtensionMenuItem::List& list = i->second;
   ExtensionMenuItem::List::iterator j;
   for (j = list.begin(); j < list.end(); ++j) {
-    // See if the current item is a match, or if one of its children was.
+    // See if the current top-level item is a match.
     if ((*j)->id() == id) {
+      items_removed = (*j)->RemoveAllDescendants();
+      items_removed.insert(id);
       delete *j;
       list.erase(j);
-      items_by_id_.erase(id);
-      result = true;
-      break;
-    } else if ((*j)->RemoveChild(id)) {
-      items_by_id_.erase(id);
       result = true;
       break;
+    } else {
+      // See if the item to remove was found as a descendant of the current
+      // top-level item.
+      ExtensionMenuItem* child = (*j)->ReleaseChild(id, true /* recursive */);
+      if (child) {
+        items_removed = child->RemoveAllDescendants();
+        items_removed.insert(id);
+        delete child;
+        result = true;
+        break;
+      }
     }
   }
   DCHECK(result);  // The check at the very top should have prevented this.
 
+  // Clear entries from the items_by_id_ map.
+  std::set<ExtensionMenuItem::Id>::iterator removed_iter;
+  for (removed_iter = items_removed.begin();
+       removed_iter != items_removed.end();
+       ++removed_iter) {
+    items_by_id_.erase(*removed_iter);
+  }
+
   if (list.empty())
     icon_manager_.RemoveIcon(extension_id);
 
diff --git a/chrome/browser/extensions/extension_menu_manager.h b/chrome/browser/extensions/extension_menu_manager.h
index 84c0588..ea1758c 100644
--- a/chrome/browser/extensions/extension_menu_manager.h
+++ b/chrome/browser/extensions/extension_menu_manager.h
@@ -12,6 +12,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/scoped_ptr.h"
 #include "base/string16.h"
 #include "chrome/browser/extensions/extension_icon_manager.h"
@@ -134,10 +135,6 @@ class ExtensionMenuItem {
   // Takes ownership of |item| and sets its parent_id_.
   void AddChild(ExtensionMenuItem* item);
 
-  // Removes child menu item with the given id, returning true if the item was
-  // found and removed, or false otherwise.
-  bool RemoveChild(const Id& child_id);
-
   // Takes the child item from this parent. The item is returned and the caller
   // then owns the pointer.
   ExtensionMenuItem* ReleaseChild(const Id& child_id, bool recursive);
@@ -247,6 +244,8 @@ class ExtensionMenuManager : public NotificationObserver {
   static bool HasAllowedScheme(const GURL& url);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(ExtensionMenuManagerTest, DeleteParent);
+
   // This is a helper function which takes care of de-selecting any other radio
   // items in the same group (i.e. that are adjacent in the list).
   void RadioItemSelected(ExtensionMenuItem* item);
diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc
index e3f82e8..39ddcb7 100644
--- a/chrome/browser/extensions/extension_menu_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc
@@ -152,6 +152,71 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
   ASSERT_EQ(0, item2->child_count());
 }
 
+// Tests that deleting a parent properly removes descendants.
+TEST_F(ExtensionMenuManagerTest, DeleteParent) {
+  Extension* extension = AddExtension("1111");
+
+  // Set up 5 items to add.
+  ExtensionMenuItem* item1 = CreateTestItem(extension);
+  ExtensionMenuItem* item2 = CreateTestItem(extension);
+  ExtensionMenuItem* item3 = CreateTestItem(extension);
+  ExtensionMenuItem* item4 = CreateTestItem(extension);
+  ExtensionMenuItem* item5 = CreateTestItem(extension);
+  ExtensionMenuItem* item6 = CreateTestItem(extension);
+  ExtensionMenuItem::Id item1_id = item1->id();
+  ExtensionMenuItem::Id item2_id = item2->id();
+  ExtensionMenuItem::Id item3_id = item3->id();
+  ExtensionMenuItem::Id item4_id = item4->id();
+  ExtensionMenuItem::Id item5_id = item5->id();
+  ExtensionMenuItem::Id item6_id = item6->id();
+
+  // Add the items in the hierarchy
+  // item1 -> item2 -> item3 -> item4 -> item5 -> item6.
+  ASSERT_TRUE(manager_.AddContextItem(extension, item1));
+  ASSERT_TRUE(manager_.AddChildItem(item1_id, item2));
+  ASSERT_TRUE(manager_.AddChildItem(item2_id, item3));
+  ASSERT_TRUE(manager_.AddChildItem(item3_id, item4));
+  ASSERT_TRUE(manager_.AddChildItem(item4_id, item5));
+  ASSERT_TRUE(manager_.AddChildItem(item5_id, item6));
+  ASSERT_EQ(item1, manager_.GetItemById(item1_id));
+  ASSERT_EQ(item2, manager_.GetItemById(item2_id));
+  ASSERT_EQ(item3, manager_.GetItemById(item3_id));
+  ASSERT_EQ(item4, manager_.GetItemById(item4_id));
+  ASSERT_EQ(item5, manager_.GetItemById(item5_id));
+  ASSERT_EQ(item6, manager_.GetItemById(item6_id));
+  ASSERT_EQ(1u, manager_.MenuItems(extension->id())->size());
+  ASSERT_EQ(6u, manager_.items_by_id_.size());
+
+  // Remove item6 (a leaf node).
+  ASSERT_TRUE(manager_.RemoveContextMenuItem(item6_id));
+  ASSERT_EQ(item1, manager_.GetItemById(item1_id));
+  ASSERT_EQ(item2, manager_.GetItemById(item2_id));
+  ASSERT_EQ(item3, manager_.GetItemById(item3_id));
+  ASSERT_EQ(item4, manager_.GetItemById(item4_id));
+  ASSERT_EQ(item5, manager_.GetItemById(item5_id));
+  ASSERT_EQ(NULL, manager_.GetItemById(item6_id));
+  ASSERT_EQ(1u, manager_.MenuItems(extension->id())->size());
+  ASSERT_EQ(5u, manager_.items_by_id_.size());
+
+  // Remove item4 and make sure item5 is gone as well.
+  ASSERT_TRUE(manager_.RemoveContextMenuItem(item4_id));
+  ASSERT_EQ(item1, manager_.GetItemById(item1_id));
+  ASSERT_EQ(item2, manager_.GetItemById(item2_id));
+  ASSERT_EQ(item3, manager_.GetItemById(item3_id));
+  ASSERT_EQ(NULL, manager_.GetItemById(item4_id));
+  ASSERT_EQ(NULL, manager_.GetItemById(item5_id));
+  ASSERT_EQ(1u, manager_.MenuItems(extension->id())->size());
+  ASSERT_EQ(3u, manager_.items_by_id_.size());
+
+  // Now remove item1 and make sure item2 and item3 are gone as well.
+  ASSERT_TRUE(manager_.RemoveContextMenuItem(item1_id));
+  ASSERT_EQ(0u, manager_.MenuItems(extension->id())->size());
+  ASSERT_EQ(0u, manager_.items_by_id_.size());
+  ASSERT_EQ(NULL, manager_.GetItemById(item1_id));
+  ASSERT_EQ(NULL, manager_.GetItemById(item2_id));
+  ASSERT_EQ(NULL, manager_.GetItemById(item3_id));
+}
+
 // Tests changing parents.
 TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   Extension* extension1 = AddExtension("1111");