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

5 files changed, 162 insertions, 194 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) {
diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index cc54c27..d3ea7d6 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -62,7 +62,12 @@ bool RenderViewContextMenu::IsSyncResourcesURL(const GURL& url) {
 }
 
 static const int kSpellcheckRadioGroup = 1;
-static const int kExtensionsRadioGroup = 2;
+
+// For extensions that have multiple top level menu items, we automatically
+// create a submenu item and push the top level menu items into it. This special
+// value takes the place of the ExtensionMenuItem's internal ID for the submenu
+// item inside the extension_item_map_ member variable.
+static const int kExtensionTopLevelItem = -1;
 
 RenderViewContextMenu::RenderViewContextMenu(
     TabContents* tab_contents,
@@ -122,97 +127,84 @@ static bool ExtensionContextMatch(ContextMenuParams params,
   return false;
 }
 
-void RenderViewContextMenu::GetItemsForExtension(
-    const std::string& extension_id,
-    std::vector<const ExtensionMenuItem*>* items) {
-  ExtensionsService* service = profile_->GetExtensionsService();
-
-  // Get the set of possible items, and iterate to find which ones are
-  // applicable.
-  std::vector<const ExtensionMenuItem*> potential_items =
-      service->menu_manager()->MenuItems(extension_id);
-
-  std::vector<const ExtensionMenuItem*>::const_iterator i;
-  for (i = potential_items.begin(); i != potential_items.end(); ++i) {
-    const ExtensionMenuItem* item = *i;
-    if (ExtensionContextMatch(params_, item->contexts()))
-      items->push_back(item);
+// Given a list of items, returns the ones that match given the contents
+// of |params|.
+static ExtensionMenuItem::List GetRelevantExtensionItems(
+    const ExtensionMenuItem::List& items,
+    ContextMenuParams params) {
+  ExtensionMenuItem::List result;
+  for (ExtensionMenuItem::List::const_iterator i = items.begin();
+       i != items.end(); ++i) {
+    if (ExtensionContextMatch(params, (*i)->contexts()))
+      result.push_back(*i);
   }
+  return result;
 }
 
 void RenderViewContextMenu::AppendExtensionItems(
     const std::string& extension_id, int* index) {
-  Extension* extension =
-      profile_->GetExtensionsService()->GetExtensionById(extension_id, false);
+  ExtensionsService* service = profile_->GetExtensionsService();
+  ExtensionMenuManager* manager = service->menu_manager();
+  Extension* extension = service->GetExtensionById(extension_id, false);
   DCHECK_GE(*index, 0);
   int max_index =
       IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST - IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST;
   if (!extension || *index >= max_index)
     return;
 
-  std::vector<const ExtensionMenuItem*> items;
-  GetItemsForExtension(extension_id, &items);
+  // Find matching items.
+  const ExtensionMenuItem::List* all_items = manager->MenuItems(extension_id);
+  if (!all_items || all_items->empty())
+    return;
+  ExtensionMenuItem::List items =
+      GetRelevantExtensionItems(*all_items, params_);
   if (items.empty())
     return;
 
-  string16 selection_text = PrintableSelectionText();
-
   // If this is the first extension-provided menu item, add a separator.
   if (*index == 0)
     menu_model_.AddSeparator();
 
-  // When extensions have more than 1 top-level item or a single parent item
-  // with children, we will start a sub menu. In the case of 1 parent with
-  // children, we will remove the parent from |items| and insert the children
-  // into it. The |index| parameter is incremented if we start a submenu. This
-  // returns true if a submenu was started. If we had multiple top-level items
-  // that needed to be pushed into a submenu, we'll use |extension_name| as the
-  // title.
-  menus::SimpleMenuModel* menu_model;
-  if (items.empty() || (items.size() == 1 && items[0]->child_count() == 0)) {
-    menu_model = &menu_model_;
+  int menu_id = IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST + (*index)++;
+
+  // Extensions are only allowed one top-level slot (and it can't be a radio or
+  // checkbox item because we are going to put the extension icon next to it).
+  // If they have more than that, we automatically push them into a submenu.
+  string16 title;
+  ExtensionMenuItem::List submenu_items;
+  if (items.size() > 1 || items[0]->type() != ExtensionMenuItem::NORMAL) {
+    title = UTF8ToUTF16(extension->name());
+    extension_item_map_[menu_id] = kExtensionTopLevelItem;
+    submenu_items = items;
   } else {
-    menu_model = new menus::SimpleMenuModel(this);
-    extension_menu_models_.push_back(menu_model);
-
-    int menu_id = IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST + (*index)++;
-    string16 title;
-    if (items[0]->child_count() > 0) {
-      title = items[0]->TitleWithReplacement(selection_text);
-      extension_item_map_[menu_id] = items[0]->id();
-    } else {
-      title = UTF8ToUTF16(extension->name());
-    }
+    ExtensionMenuItem* item = items[0];
+    extension_item_map_[menu_id] = item->id();
+    title = item->TitleWithReplacement(PrintableSelectionText());
+    submenu_items = GetRelevantExtensionItems(item->children(), params_);
+  }
 
-    menu_model_.AddSubMenu(menu_id, title, menu_model);
-
-    // If we have 1 parent item with a submenu of children, pull the
-    // parent out of |items| and put the children in.
-    if (items.size() == 1 && items[0]->child_count() > 0) {
-      const ExtensionMenuItem* parent = items[0];
-      items.clear();
-      for (int j = 0; j < parent->child_count(); j++) {
-        const ExtensionMenuItem* child = parent->ChildAt(j);
-        if (ExtensionContextMatch(params_, child->contexts()))
-          items.push_back(child);
-      }
-    }
+  // Now add our item(s) to the menu_model_.
+  if (submenu_items.empty()) {
+    menu_model_.AddItem(menu_id, title);
+  } else {
+    menus::SimpleMenuModel* submenu = new menus::SimpleMenuModel(this);
+    extension_menu_models_.push_back(submenu);
+    menu_model_.AddSubMenu(menu_id, title, submenu);
+    RecursivelyAppendExtensionItems(submenu_items, submenu, index);
   }
+}
 
+void RenderViewContextMenu::RecursivelyAppendExtensionItems(
+    const ExtensionMenuItem::List& items,
+    menus::SimpleMenuModel* menu_model,
+    int *index) {
+  string16 selection_text = PrintableSelectionText();
   ExtensionMenuItem::Type last_type = ExtensionMenuItem::NORMAL;
-  int radio_group_id = kExtensionsRadioGroup;
-  for (std::vector<const ExtensionMenuItem*>::iterator i = items.begin();
+  int radio_group_id = 1;
+
+  for (ExtensionMenuItem::List::const_iterator i = items.begin();
        i != items.end(); ++i) {
-    const ExtensionMenuItem* item = *i;
-    if (item->type() == ExtensionMenuItem::SEPARATOR) {
-      // We don't want the case of an extension with one top-level item that is
-      // just a separator, so make sure this is inside a submenu.
-      if (menu_model != &menu_model_) {
-        menu_model->AddSeparator();
-        last_type = ExtensionMenuItem::SEPARATOR;
-      }
-      continue;
-    }
+    ExtensionMenuItem* item = *i;
 
     // Auto-prepend a separator, if needed, to visually group radio items
     // together.
@@ -229,7 +221,16 @@ void RenderViewContextMenu::AppendExtensionItems(
     extension_item_map_[menu_id] = item->id();
     string16 title = item->TitleWithReplacement(selection_text);
     if (item->type() == ExtensionMenuItem::NORMAL) {
-      menu_model->AddItem(menu_id, title);
+      ExtensionMenuItem::List children =
+          GetRelevantExtensionItems(item->children(), params_);
+      if (children.size() == 0) {
+        menu_model->AddItem(menu_id, title);
+      } else {
+        menus::SimpleMenuModel* submenu = new menus::SimpleMenuModel(this);
+        extension_menu_models_.push_back(submenu);
+        menu_model->AddSubMenu(menu_id, title, submenu);
+        RecursivelyAppendExtensionItems(children, submenu, index);
+      }
     } else if (item->type() == ExtensionMenuItem::CHECKBOX) {
       menu_model->AddCheckItem(menu_id, title);
     } else if (item->type() == ExtensionMenuItem::RADIO) {
@@ -680,11 +681,18 @@ bool RenderViewContextMenu::IsCommandIdEnabled(int id) const {
   // Extension items.
   if (id >= IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST &&
       id <= IDC_EXTENSIONS_CONTEXT_CUSTOM_LAST) {
-    ExtensionMenuItem* item = GetExtensionMenuItem(id);
-    if (item)
-      return ExtensionContextMatch(params_, item->enabled_contexts());
-    else
+    std::map<int, int>::const_iterator i = extension_item_map_.find(id);
+
+    // Unknown item.
+    if (i == extension_item_map_.end())
       return false;
+
+    // Auto-inserted top-level extension parent.
+    if (i->second == kExtensionTopLevelItem)
+      return true;
+
+    return ExtensionContextMatch(params_,
+                                 GetExtensionMenuItem(id)->enabled_contexts());
   }
 
   switch (id) {
diff --git a/chrome/browser/tab_contents/render_view_context_menu.h b/chrome/browser/tab_contents/render_view_context_menu.h
index fd264ff..97d0d5c 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.h
+++ b/chrome/browser/tab_contents/render_view_context_menu.h
@@ -80,15 +80,18 @@ class RenderViewContextMenu : public menus::SimpleMenuModel::Delegate {
   void AppendSpellcheckOptionsSubMenu();
   // Add writing direction sub menu (only used on Mac).
   void AppendBidiSubMenu();
-  // Fills in |items| with matching items for extension with |extension_id|.
-  void GetItemsForExtension(const std::string& extension_id,
-                            std::vector<const ExtensionMenuItem*>* items);
 
   // This is a helper function to append items for one particular extension.
   // The |index| parameter is used for assigning id's, and is incremented for
   // each item actually added.
   void AppendExtensionItems(const std::string& extension_id, int* index);
 
+  // Used for recursively adding submenus of extension items.
+  void RecursivelyAppendExtensionItems(
+      const std::vector<ExtensionMenuItem*>& items,
+      menus::SimpleMenuModel* menu_model,
+      int *index);
+
   // Opens the specified URL string in a new tab.  If |in_current_window| is
   // false, a new window is created to hold the new tab.
   void OpenURL(const GURL& url,