Fix submenu support for extensions context menu API.

This fixes submenus which broke on all platforms during a recent refactor of
the RenderViewContextMenu class, and adds support for more than one level of
child menus.


BUG=39504
TEST=Create a test extension using the chrome.experimental.contextMenu API and
add multiple levels of child menu items.


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

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

3 files changed, 75 insertions, 118 deletions

diff --git a/chrome/browser/extensions/extension_menu_manager.cc b/chrome/browser/extensions/extension_menu_manager.cc
index 057cec1..9563a0a 100644
--- a/chrome/browser/extensions/extension_menu_manager.cc
+++ b/chrome/browser/extensions/extension_menu_manager.cc
@@ -32,12 +32,8 @@ ExtensionMenuItem::ExtensionMenuItem(const std::string& extension_id,
       enabled_contexts_(enabled_contexts),
       parent_id_(0) {}
 
-ExtensionMenuItem::~ExtensionMenuItem() {}
-
-ExtensionMenuItem* ExtensionMenuItem::ChildAt(int index) const {
-  if (index < 0 || static_cast<size_t>(index) >= children_.size())
-    return NULL;
-  return children_[index].get();
+ExtensionMenuItem::~ExtensionMenuItem() {
+  STLDeleteElements(&children_);
 }
 
 bool ExtensionMenuItem::RemoveChild(int child_id) {
@@ -55,7 +51,7 @@ ExtensionMenuItem* ExtensionMenuItem::ReleaseChild(int child_id,
   for (List::iterator i = children_.begin(); i != children_.end(); ++i) {
     ExtensionMenuItem* child = NULL;
     if ((*i)->id() == child_id) {
-      child = i->release();
+      child = *i;
       children_.erase(i);
       return child;
     } else if (recursive) {
@@ -70,12 +66,12 @@ ExtensionMenuItem* ExtensionMenuItem::ReleaseChild(int child_id,
 std::set<int> ExtensionMenuItem::RemoveAllDescendants() {
   std::set<int> result;
   for (List::iterator i = children_.begin(); i != children_.end(); ++i) {
-    ExtensionMenuItem* child = i->get();
+    ExtensionMenuItem* child = *i;
     result.insert(child->id());
     std::set<int> removed = child->RemoveAllDescendants();
     result.insert(removed.begin(), removed.end());
   }
-  children_.clear();
+  STLDeleteElements(&children_);
   return result;
 }
 
@@ -97,7 +93,7 @@ bool ExtensionMenuItem::SetChecked(bool checked) {
 
 void ExtensionMenuItem::AddChild(ExtensionMenuItem* item) {
   item->parent_id_ = id_;
-  children_.push_back(linked_ptr<ExtensionMenuItem>(item));
+  children_.push_back(item);
 }
 
 ExtensionMenuManager::ExtensionMenuManager() : next_item_id_(1) {
@@ -105,7 +101,12 @@ ExtensionMenuManager::ExtensionMenuManager() : next_item_id_(1) {
                  NotificationService::AllSources());
 }
 
-ExtensionMenuManager::~ExtensionMenuManager() {}
+ExtensionMenuManager::~ExtensionMenuManager() {
+  MenuItemMap::iterator i;
+  for (i = context_items_.begin(); i != context_items_.end(); ++i) {
+    STLDeleteElements(&(i->second));
+  }
+}
 
 std::set<std::string> ExtensionMenuManager::ExtensionIds() {
   std::set<std::string> id_set;
@@ -116,20 +117,13 @@ std::set<std::string> ExtensionMenuManager::ExtensionIds() {
   return id_set;
 }
 
-std::vector<const ExtensionMenuItem*> ExtensionMenuManager::MenuItems(
+const ExtensionMenuItem::List* ExtensionMenuManager::MenuItems(
     const std::string& extension_id) {
-  std::vector<const ExtensionMenuItem*> result;
-
   MenuItemMap::iterator i = context_items_.find(extension_id);
   if (i != context_items_.end()) {
-    ExtensionMenuItem::List& list = i->second;
-    ExtensionMenuItem::List::iterator j;
-    for (j = list.begin(); j != list.end(); ++j) {
-      result.push_back(j->get());
-    }
+    return &(i->second);
   }
-
-  return result;
+  return NULL;
 }
 
 int ExtensionMenuManager::AddContextItem(ExtensionMenuItem* item) {
@@ -141,7 +135,7 @@ int ExtensionMenuManager::AddContextItem(ExtensionMenuItem* item) {
   DCHECK_EQ(0, item->id());
   item->set_id(next_item_id_++);
 
-  context_items_[extension_id].push_back(linked_ptr<ExtensionMenuItem>(item));
+  context_items_[extension_id].push_back(item);
   items_by_id_[item->id()] = item;
 
   if (item->type() == ExtensionMenuItem::RADIO && item->checked())
@@ -216,15 +210,13 @@ bool ExtensionMenuManager::ChangeParent(int child_id, int parent_id) {
       NOTREACHED();
       return false;
     }
-    j->release();
     list.erase(j);
   }
 
   if (new_parent) {
     new_parent->AddChild(child);
   } else {
-    context_items_[child->extension_id()].push_back(
-        linked_ptr<ExtensionMenuItem>(child));
+    context_items_[child->extension_id()].push_back(child);
     child->parent_id_ = 0;
   }
   return true;
@@ -246,6 +238,7 @@ bool ExtensionMenuManager::RemoveContextMenuItem(int id) {
   for (j = list.begin(); j < list.end(); ++j) {
     // See if the current item is a match, or if one of its children was.
     if ((*j)->id() == id) {
+      delete *j;
       list.erase(j);
       items_by_id_.erase(id);
       return true;
@@ -262,7 +255,7 @@ void ExtensionMenuManager::RemoveAllContextItems(std::string extension_id) {
   ExtensionMenuItem::List::iterator i;
   for (i = context_items_[extension_id].begin();
        i != context_items_[extension_id].end(); ++i) {
-    ExtensionMenuItem* item = i->get();
+    ExtensionMenuItem* item = *i;
     items_by_id_.erase(item->id());
 
     // Remove descendants from this item and erase them from the lookup cache.
@@ -272,6 +265,7 @@ void ExtensionMenuManager::RemoveAllContextItems(std::string extension_id) {
       items_by_id_.erase(*j);
     }
   }
+  STLDeleteElements(&context_items_[extension_id]);
   context_items_.erase(extension_id);
 }
 
@@ -286,14 +280,14 @@ ExtensionMenuItem* ExtensionMenuManager::GetItemById(int id) const {
 void ExtensionMenuManager::RadioItemSelected(ExtensionMenuItem* item) {
   // If this is a child item, we need to get a handle to the list from its
   // parent. Otherwise get a handle to the top-level list.
-  ExtensionMenuItem::List* list = NULL;
+  const ExtensionMenuItem::List* list = NULL;
   if (item->parent_id()) {
     ExtensionMenuItem* parent = GetItemById(item->parent_id());
     if (!parent) {
       NOTREACHED();
       return;
     }
-    list = parent->children();
+    list = &(parent->children());
   } else {
     if (context_items_.find(item->extension_id()) == context_items_.end()) {
       NOTREACHED();
@@ -303,10 +297,10 @@ void ExtensionMenuManager::RadioItemSelected(ExtensionMenuItem* item) {
   }
 
   // Find where |item| is in the list.
-  ExtensionMenuItem::List::iterator item_location;
+  ExtensionMenuItem::List::const_iterator item_location;
   for (item_location = list->begin(); item_location != list->end();
        ++item_location) {
-    if (item_location->get() == item)
+    if (*item_location == item)
       break;
   }
   if (item_location == list->end()) {
@@ -315,7 +309,7 @@ void ExtensionMenuManager::RadioItemSelected(ExtensionMenuItem* item) {
   }
 
   // Iterate backwards from |item| and uncheck any adjacent radio items.
-  ExtensionMenuItem::List::iterator i;
+  ExtensionMenuItem::List::const_iterator i;
   if (item_location != list->begin()) {
     i = item_location;
     do {
@@ -340,28 +334,6 @@ static void AddURLProperty(DictionaryValue* dictionary,
     dictionary->SetString(key, url.possibly_invalid_spec());
 }
 
-void ExtensionMenuManager::GetItemAndIndex(int id, ExtensionMenuItem** item,
-                                           size_t* index) {
-  for (MenuItemMap::const_iterator i = context_items_.begin();
-       i != context_items_.end(); ++i) {
-    const ExtensionMenuItem::List& list = i->second;
-    for (size_t tmp_index = 0; tmp_index < list.size(); tmp_index++) {
-      if (list[tmp_index]->id() == id) {
-        if (item)
-          *item = list[tmp_index].get();
-        if (index)
-          *index = tmp_index;
-        return;
-      }
-    }
-  }
-  if (item)
-    *item = NULL;
-  if (index)
-    *index = 0;
-}
-
-
 void ExtensionMenuManager::ExecuteCommand(Profile* profile,
                                           TabContents* tab_contents,
                                           const ContextMenuParams& params,
@@ -446,12 +418,7 @@ void ExtensionMenuManager::Observe(NotificationType type,
     return;
   }
   Extension* extension = Details<Extension>(details).ptr();
-  MenuItemMap::iterator i = context_items_.find(extension->id());
-  if (i != context_items_.end()) {
-    const ExtensionMenuItem::List& list = i->second;
-    ExtensionMenuItem::List::const_iterator j;
-    for (j = list.begin(); j != list.end(); ++j)
-      items_by_id_.erase((*j)->id());
-    context_items_.erase(i);
+  if (ContainsKey(context_items_, extension->id())) {
+    RemoveAllContextItems(extension->id());
   }
 }
diff --git a/chrome/browser/extensions/extension_menu_manager.h b/chrome/browser/extensions/extension_menu_manager.h
index 771bf19..e85f5d5 100644
--- a/chrome/browser/extensions/extension_menu_manager.h
+++ b/chrome/browser/extensions/extension_menu_manager.h
@@ -26,8 +26,8 @@ class TabContents;
 // Represents a menu item added by an extension.
 class ExtensionMenuItem {
  public:
-  // A list of owned ExtensionMenuItem's.
-  typedef std::vector<linked_ptr<ExtensionMenuItem> > List;
+  // A list of ExtensionMenuItem's.
+  typedef std::vector<ExtensionMenuItem*> List;
 
   // For context menus, these are the contexts where an item can appear and
   // potentially be enabled.
@@ -89,6 +89,7 @@ class ExtensionMenuItem {
   // Simple accessor methods.
   const std::string& extension_id() const { return extension_id_; }
   const std::string& title() const { return title_; }
+  const List& children() { return children_; }
   int id() const { return id_; }
   int parent_id() const { return parent_id_; }
   int child_count() const { return children_.size(); }
@@ -103,9 +104,6 @@ class ExtensionMenuItem {
   void set_enabled_contexts(ContextList contexts) { contexts_ = contexts; }
   void set_type(Type type) { type_ = type; }
 
-  // Returns the child at the given index, or NULL.
-  ExtensionMenuItem* ChildAt(int index) const;
-
   // Returns the title with any instances of %s replaced by |selection|.
   string16 TitleWithReplacement(const string16& selection) const;
 
@@ -121,9 +119,6 @@ class ExtensionMenuItem {
     id_ = id;
   }
 
-  // Provides direct access to the children of this item.
-  List* children() { return &children_; }
-
   // Takes ownership of |item| and sets its parent_id_.
   void AddChild(ExtensionMenuItem* item);
 
@@ -183,10 +178,9 @@ class ExtensionMenuManager : public NotificationObserver {
   // Returns a list of all the *top-level* menu items (added via AddContextItem)
   // for the given extension id, *not* including child items (added via
   // AddChildItem); although those can be reached via the top-level items'
-  // ChildAt function. A view can then decide how to display these, including
-  // whether to put them into a submenu if there are more than 1.
-  std::vector<const ExtensionMenuItem*> MenuItems(
-      const std::string& extension_id);
+  // children. A view can then decide how to display these, including whether to
+  // put them into a submenu if there are more than 1.
+  const ExtensionMenuItem::List* MenuItems(const std::string& extension_id);
 
   // Takes ownership of |item|. Returns the id assigned to the item. Has the
   // side-effect of incrementing the next_item_id_ member.
@@ -228,10 +222,6 @@ class ExtensionMenuManager : public NotificationObserver {
   // items in the same group (i.e. that are adjacent in the list).
   void RadioItemSelected(ExtensionMenuItem* item);
 
-  // If an item with |id| is found, |item| will be set to point to it and
-  // |index| will be set to its index within the containing list.
-  void GetItemAndIndex(int id, ExtensionMenuItem** item, size_t* index);
-
   // Returns true if item is a descendant of an item with id |ancestor_id|.
   bool DescendantOf(ExtensionMenuItem* item, int ancestor_id);
 
diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc
index fce804a..2abeef1 100644
--- a/chrome/browser/extensions/extension_menu_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc
@@ -62,10 +62,10 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   int id1 = manager_.AddContextItem(item1);  // Ownership transferred.
   ASSERT_GT(id1, 0);
   ASSERT_EQ(item1, manager_.GetItemById(id1));
-  std::vector<const ExtensionMenuItem*> items =
+  const ExtensionMenuItem::List* items =
       manager_.MenuItems(item1->extension_id());
-  ASSERT_EQ(1u, items.size());
-  ASSERT_EQ(item1, items[0]);
+  ASSERT_EQ(1u, items->size());
+  ASSERT_EQ(item1, items->at(0));
 
   // Add a second item, make sure it comes back too.
   ExtensionMenuItem* item2 = CreateTestItem(NULL);
@@ -74,9 +74,9 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   ASSERT_NE(id1, id2);
   ASSERT_EQ(item2, manager_.GetItemById(id2));
   items = manager_.MenuItems(item2->extension_id());
-  ASSERT_EQ(2u, items.size());
-  ASSERT_EQ(item1, items[0]);
-  ASSERT_EQ(item2, items[1]);
+  ASSERT_EQ(2u, items->size());
+  ASSERT_EQ(item1, items->at(0));
+  ASSERT_EQ(item2, items->at(1));
 
   // Try adding item 3, then removing it.
   ExtensionMenuItem* item3 = CreateTestItem(NULL);
@@ -84,10 +84,10 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   int id3 = manager_.AddContextItem(item3);  // Ownership transferred.
   ASSERT_GT(id3, 0);
   ASSERT_EQ(item3, manager_.GetItemById(id3));
-  ASSERT_EQ(3u, manager_.MenuItems(extension_id).size());
+  ASSERT_EQ(3u, manager_.MenuItems(extension_id)->size());
   ASSERT_TRUE(manager_.RemoveContextMenuItem(id3));
   ASSERT_EQ(NULL, manager_.GetItemById(id3));
-  ASSERT_EQ(2u, manager_.MenuItems(extension_id).size());
+  ASSERT_EQ(2u, manager_.MenuItems(extension_id)->size());
 
   // Make sure removing a non-existent item returns false.
   ASSERT_FALSE(manager_.RemoveContextMenuItem(5));
@@ -126,8 +126,8 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
   ASSERT_EQ(0, item1->child_count());
   ASSERT_EQ(item2_child, manager_.GetItemById(id2_child));
 
-  ASSERT_EQ(1u, manager_.MenuItems(item1->extension_id()).size());
-  ASSERT_EQ(item1, manager_.MenuItems(item1->extension_id()).at(0));
+  ASSERT_EQ(1u, manager_.MenuItems(item1->extension_id())->size());
+  ASSERT_EQ(item1, manager_.MenuItems(item1->extension_id())->at(0));
 
   // Add item2_grandchild as a child of item2_child, then remove it.
   int id2_grandchild = manager_.AddChildItem(id2_child, item2_grandchild);
@@ -138,13 +138,13 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
 
   // We should only get 1 thing back when asking for item2's extension id, since
   // it has a child item.
-  ASSERT_EQ(1u, manager_.MenuItems(item2->extension_id()).size());
-  ASSERT_EQ(item2, manager_.MenuItems(item2->extension_id()).at(0));
+  ASSERT_EQ(1u, manager_.MenuItems(item2->extension_id())->size());
+  ASSERT_EQ(item2, manager_.MenuItems(item2->extension_id())->at(0));
 
   // Remove child2_item.
   ASSERT_TRUE(manager_.RemoveContextMenuItem(id2_child));
-  ASSERT_EQ(1u, manager_.MenuItems(item2->extension_id()).size());
-  ASSERT_EQ(item2, manager_.MenuItems(item2->extension_id()).at(0));
+  ASSERT_EQ(1u, manager_.MenuItems(item2->extension_id())->size());
+  ASSERT_EQ(item2, manager_.MenuItems(item2->extension_id())->at(0));
   ASSERT_EQ(0, item2->child_count());
 }
 
@@ -159,11 +159,11 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   int id2 = manager_.AddContextItem(item2);
   ASSERT_GT(id2, 0);
 
-  std::vector<const ExtensionMenuItem*> items =
+  const ExtensionMenuItem::List* items =
       manager_.MenuItems(item1->extension_id());
-  ASSERT_EQ(2u, items.size());
-  ASSERT_EQ(item1, items.at(0));
-  ASSERT_EQ(item2, items.at(1));
+  ASSERT_EQ(2u, items->size());
+  ASSERT_EQ(item1, items->at(0));
+  ASSERT_EQ(item2, items->at(1));
 
   // Now create a third item, initially add it as a child of item1, then move
   // it to be a child of item2.
@@ -172,50 +172,50 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   int id3 = manager_.AddChildItem(id1, item3);
   ASSERT_GT(id3, 0);
   ASSERT_EQ(1, item1->child_count());
-  ASSERT_EQ(item3, item1->ChildAt(0));
+  ASSERT_EQ(item3, item1->children()[0]);
 
   ASSERT_TRUE(manager_.ChangeParent(id3, id2));
   ASSERT_EQ(0, item1->child_count());
   ASSERT_EQ(1, item2->child_count());
-  ASSERT_EQ(item3, item2->ChildAt(0));
+  ASSERT_EQ(item3, item2->children()[0]);
 
   // Move item2 to be a child of item1.
   ASSERT_TRUE(manager_.ChangeParent(id2, id1));
   ASSERT_EQ(1, item1->child_count());
-  ASSERT_EQ(item2, item1->ChildAt(0));
+  ASSERT_EQ(item2, item1->children()[0]);
   ASSERT_EQ(1, item2->child_count());
-  ASSERT_EQ(item3, item2->ChildAt(0));
+  ASSERT_EQ(item3, item2->children()[0]);
 
   // Since item2 was a top-level item but is no longer, we should only have 1
   // top-level item.
   items = manager_.MenuItems(item1->extension_id());
-  ASSERT_EQ(1u, items.size());
-  ASSERT_EQ(item1, items.at(0));
+  ASSERT_EQ(1u, items->size());
+  ASSERT_EQ(item1, items->at(0));
 
   // Move item3 back to being a child of item1, so it's now a sibling of item2.
   ASSERT_TRUE(manager_.ChangeParent(id3, id1));
   ASSERT_EQ(2, item1->child_count());
-  ASSERT_EQ(item2, item1->ChildAt(0));
-  ASSERT_EQ(item3, item1->ChildAt(1));
+  ASSERT_EQ(item2, item1->children()[0]);
+  ASSERT_EQ(item3, item1->children()[1]);
 
   // Try switching item3 to be the parent of item1 - this should fail.
   ASSERT_FALSE(manager_.ChangeParent(id1, id3));
   ASSERT_EQ(0, item3->child_count());
   ASSERT_EQ(2, item1->child_count());
-  ASSERT_EQ(item2, item1->ChildAt(0));
-  ASSERT_EQ(item3, item1->ChildAt(1));
+  ASSERT_EQ(item2, item1->children()[0]);
+  ASSERT_EQ(item3, item1->children()[1]);
   items = manager_.MenuItems(item1->extension_id());
-  ASSERT_EQ(1u, items.size());
-  ASSERT_EQ(item1, items.at(0));
+  ASSERT_EQ(1u, items->size());
+  ASSERT_EQ(item1, items->at(0));
 
   // Move item2 to be a top-level item.
   ASSERT_TRUE(manager_.ChangeParent(id2, 0));
   items = manager_.MenuItems(item1->extension_id());
-  ASSERT_EQ(2u, items.size());
-  ASSERT_EQ(item1, items.at(0));
-  ASSERT_EQ(item2, items.at(1));
+  ASSERT_EQ(2u, items->size());
+  ASSERT_EQ(item1, items->at(0));
+  ASSERT_EQ(item2, items->at(1));
   ASSERT_EQ(1, item1->child_count());
-  ASSERT_EQ(item3, item1->ChildAt(0));
+  ASSERT_EQ(item3, item1->children()[0]);
 
   // Make sure you can't move a node to be a child of another extension's item.
   DictionaryValue properties;
@@ -256,7 +256,7 @@ TEST_F(ExtensionMenuManagerTest, ExtensionUnloadRemovesMenuItems) {
   ASSERT_EQ(extension.id(), item1->extension_id());
   int id1 = manager_.AddContextItem(item1);  // Ownership transferred.
   ASSERT_GT(id1, 0);
-  ASSERT_EQ(1u, manager_.MenuItems(extension.id()).size());
+  ASSERT_EQ(1u, manager_.MenuItems(extension.id())->size());
 
   // Create a menu item with a different extension id and add it to the manager.
   std::string alternate_extension_id = "0000";
@@ -271,8 +271,8 @@ TEST_F(ExtensionMenuManagerTest, ExtensionUnloadRemovesMenuItems) {
   notifier->Notify(NotificationType::EXTENSION_UNLOADED,
                    Source<Profile>(NULL),
                    Details<Extension>(&extension));
-  ASSERT_EQ(0u, manager_.MenuItems(extension.id()).size());
-  ASSERT_EQ(1u, manager_.MenuItems(alternate_extension_id).size());
+  ASSERT_EQ(NULL, manager_.MenuItems(extension.id()));
+  ASSERT_EQ(1u, manager_.MenuItems(alternate_extension_id)->size());
   ASSERT_TRUE(manager_.GetItemById(id1) == NULL);
   ASSERT_TRUE(manager_.GetItemById(id2) != NULL);
 }
@@ -326,17 +326,17 @@ TEST_F(ExtensionMenuManagerTest, RemoveAll) {
   ExtensionMenuItem* item4 = CreateTestItem(&properties);
   manager_.AddContextItem(item4);
 
-  EXPECT_EQ(2u, manager_.MenuItems("AAAA").size());
-  EXPECT_EQ(1u, manager_.MenuItems("BBBB").size());
+  EXPECT_EQ(2u, manager_.MenuItems("AAAA")->size());
+  EXPECT_EQ(1u, manager_.MenuItems("BBBB")->size());
 
   // Remove the BBBB item.
   manager_.RemoveAllContextItems("BBBB");
-  EXPECT_EQ(2u, manager_.MenuItems("AAAA").size());
-  EXPECT_EQ(0u, manager_.MenuItems("BBBB").size());
+  EXPECT_EQ(2u, manager_.MenuItems("AAAA")->size());
+  EXPECT_EQ(NULL, manager_.MenuItems("BBBB"));
 
   // Remove the AAAA items.
   manager_.RemoveAllContextItems("AAAA");
-  EXPECT_EQ(0u, manager_.MenuItems("AAAA").size());
+  EXPECT_EQ(NULL, manager_.MenuItems("AAAA"));
 }
 
 TEST_F(ExtensionMenuManagerTest, ExecuteCommand) {