More cleanup of extensions context menu API.

This changes the contextMenus.create method to synchronously return an id
instead of aysynchronously in the callback. This meant moving the generating of
id's from a globally unique integer in C++ code to an extension-unique integer
in javascript. The C++ unique id thus becomes a pair of <extension_id,int>.

Also a couple of small drive-by cleanups while I was in some of the files.


BUG=48198
TEST=The context menu API should work normally, given the changes described 
above.


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

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

1 files changed, 45 insertions, 53 deletions

diff --git a/chrome/browser/extensions/extension_menu_manager_unittest.cc b/chrome/browser/extensions/extension_menu_manager_unittest.cc
index f0960cb..e3f82e8 100644
--- a/chrome/browser/extensions/extension_menu_manager_unittest.cc
+++ b/chrome/browser/extensions/extension_menu_manager_unittest.cc
@@ -29,15 +29,15 @@ using testing::SaveArg;
 // Base class for tests.
 class ExtensionMenuManagerTest : public testing::Test {
  public:
-  ExtensionMenuManagerTest() {}
+  ExtensionMenuManagerTest() : next_id_(1) {}
   ~ExtensionMenuManagerTest() {}
 
   // Returns a test item.
-  static ExtensionMenuItem* CreateTestItem(Extension* extension) {
+  ExtensionMenuItem* CreateTestItem(Extension* extension) {
     ExtensionMenuItem::Type type = ExtensionMenuItem::NORMAL;
     ExtensionMenuItem::ContextList contexts(ExtensionMenuItem::ALL);
-    return new ExtensionMenuItem(extension->id(), "test", false, type,
-                                 contexts);
+    ExtensionMenuItem::Id id(extension->id(), next_id_++);
+    return new ExtensionMenuItem(id, "test", false, type, contexts);
   }
 
   // Creates and returns a test Extension. The caller does *not* own the return
@@ -52,6 +52,7 @@ class ExtensionMenuManagerTest : public testing::Test {
   ExtensionMenuManager manager_;
   ScopedVector<Extension> extensions_;
   TestExtensionPrefs prefs_;
+  int next_id_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ExtensionMenuManagerTest);
@@ -64,9 +65,8 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   // Add a new item, make sure you can get it back.
   ExtensionMenuItem* item1 = CreateTestItem(extension);
   ASSERT_TRUE(item1 != NULL);
-  int id1 = manager_.AddContextItem(extension, item1);
-  ASSERT_GT(id1, 0);
-  ASSERT_EQ(item1, manager_.GetItemById(id1));
+  ASSERT_TRUE(manager_.AddContextItem(extension, item1));
+  ASSERT_EQ(item1, manager_.GetItemById(item1->id()));
   const ExtensionMenuItem::List* items =
       manager_.MenuItems(item1->extension_id());
   ASSERT_EQ(1u, items->size());
@@ -74,10 +74,8 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
 
   // Add a second item, make sure it comes back too.
   ExtensionMenuItem* item2 = CreateTestItem(extension);
-  int id2 = manager_.AddContextItem(extension, item2);
-  ASSERT_GT(id2, 0);
-  ASSERT_NE(id1, id2);
-  ASSERT_EQ(item2, manager_.GetItemById(id2));
+  ASSERT_TRUE(manager_.AddContextItem(extension, item2));
+  ASSERT_EQ(item2, manager_.GetItemById(item2->id()));
   items = manager_.MenuItems(item2->extension_id());
   ASSERT_EQ(2u, items->size());
   ASSERT_EQ(item1, items->at(0));
@@ -85,9 +83,9 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
 
   // Try adding item 3, then removing it.
   ExtensionMenuItem* item3 = CreateTestItem(extension);
+  ExtensionMenuItem::Id id3 = item3->id();
   std::string extension_id = item3->extension_id();
-  int id3 = manager_.AddContextItem(extension, item3);
-  ASSERT_GT(id3, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension, item3));
   ASSERT_EQ(item3, manager_.GetItemById(id3));
   ASSERT_EQ(3u, manager_.MenuItems(extension_id)->size());
   ASSERT_TRUE(manager_.RemoveContextMenuItem(id3));
@@ -95,7 +93,8 @@ TEST_F(ExtensionMenuManagerTest, AddGetRemoveItems) {
   ASSERT_EQ(2u, manager_.MenuItems(extension_id)->size());
 
   // Make sure removing a non-existent item returns false.
-  ASSERT_FALSE(manager_.RemoveContextMenuItem(5));
+  ExtensionMenuItem::Id id(extension->id(), id3.second + 50);
+  ASSERT_FALSE(manager_.RemoveContextMenuItem(id));
 }
 
 // Test adding/removing child items.
@@ -114,18 +113,19 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
   scoped_ptr<ExtensionMenuItem> item3(CreateTestItem(extension3));
 
   // Add in the first two items.
-  int id1 = manager_.AddContextItem(extension1, item1);
-  int id2 = manager_.AddContextItem(extension2, item2);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item2));
 
-  ASSERT_NE(id1, id2);
+  ExtensionMenuItem::Id id1 = item1->id();
+  ExtensionMenuItem::Id id2 = item2->id();
 
   // Try adding item3 as a child of item2 - this should fail because item3 has
   // a different extension id.
-  ASSERT_EQ(0, manager_.AddChildItem(id2, item3.get()));
+  ASSERT_FALSE(manager_.AddChildItem(id2, item3.get()));
 
   // Add item2_child as a child of item2.
-  int id2_child = manager_.AddChildItem(id2, item2_child);
-  ASSERT_GT(id2_child, 0);
+  ExtensionMenuItem::Id id2_child = item2_child->id();
+  ASSERT_TRUE(manager_.AddChildItem(id2, item2_child));
   ASSERT_EQ(1, item2->child_count());
   ASSERT_EQ(0, item1->child_count());
   ASSERT_EQ(item2_child, manager_.GetItemById(id2_child));
@@ -134,8 +134,8 @@ TEST_F(ExtensionMenuManagerTest, ChildFunctions) {
   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);
-  ASSERT_GT(id2_grandchild, 0);
+  ExtensionMenuItem::Id id2_grandchild = item2_grandchild->id();
+  ASSERT_TRUE(manager_.AddChildItem(id2_child, item2_grandchild));
   ASSERT_EQ(1, item2->child_count());
   ASSERT_EQ(1, item2_child->child_count());
   ASSERT_TRUE(manager_.RemoveContextMenuItem(id2_grandchild));
@@ -160,10 +160,8 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   ExtensionMenuItem* item1 = CreateTestItem(extension1);
   ExtensionMenuItem* item2 = CreateTestItem(extension1);
 
-  int id1 = manager_.AddContextItem(extension1, item1);
-  ASSERT_GT(id1, 0);
-  int id2 = manager_.AddContextItem(extension1, item2);
-  ASSERT_GT(id2, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item2));
 
   const ExtensionMenuItem::List* items =
       manager_.MenuItems(item1->extension_id());
@@ -175,18 +173,17 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   // it to be a child of item2.
   ExtensionMenuItem* item3 = CreateTestItem(extension1);
 
-  int id3 = manager_.AddChildItem(id1, item3);
-  ASSERT_GT(id3, 0);
+  ASSERT_TRUE(manager_.AddChildItem(item1->id(), item3));
   ASSERT_EQ(1, item1->child_count());
   ASSERT_EQ(item3, item1->children()[0]);
 
-  ASSERT_TRUE(manager_.ChangeParent(id3, id2));
+  ASSERT_TRUE(manager_.ChangeParent(item3->id(), &item2->id()));
   ASSERT_EQ(0, item1->child_count());
   ASSERT_EQ(1, item2->child_count());
   ASSERT_EQ(item3, item2->children()[0]);
 
   // Move item2 to be a child of item1.
-  ASSERT_TRUE(manager_.ChangeParent(id2, id1));
+  ASSERT_TRUE(manager_.ChangeParent(item2->id(), &item1->id()));
   ASSERT_EQ(1, item1->child_count());
   ASSERT_EQ(item2, item1->children()[0]);
   ASSERT_EQ(1, item2->child_count());
@@ -199,13 +196,13 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   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_TRUE(manager_.ChangeParent(item3->id(), &item1->id()));
   ASSERT_EQ(2, item1->child_count());
   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_FALSE(manager_.ChangeParent(item1->id(), &item3->id()));
   ASSERT_EQ(0, item3->child_count());
   ASSERT_EQ(2, item1->child_count());
   ASSERT_EQ(item2, item1->children()[0]);
@@ -215,7 +212,7 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   ASSERT_EQ(item1, items->at(0));
 
   // Move item2 to be a top-level item.
-  ASSERT_TRUE(manager_.ChangeParent(id2, 0));
+  ASSERT_TRUE(manager_.ChangeParent(item2->id(), NULL));
   items = manager_.MenuItems(item1->extension_id());
   ASSERT_EQ(2u, items->size());
   ASSERT_EQ(item1, items->at(0));
@@ -226,13 +223,12 @@ TEST_F(ExtensionMenuManagerTest, ChangeParent) {
   // Make sure you can't move a node to be a child of another extension's item.
   Extension* extension2 = AddExtension("2222");
   ExtensionMenuItem* item4 = CreateTestItem(extension2);
-  int id4 = manager_.AddContextItem(extension2, item4);
-  ASSERT_GT(id4, 0);
-  ASSERT_FALSE(manager_.ChangeParent(id4, id1));
-  ASSERT_FALSE(manager_.ChangeParent(id1, id4));
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item4));
+  ASSERT_FALSE(manager_.ChangeParent(item4->id(), &item1->id()));
+  ASSERT_FALSE(manager_.ChangeParent(item1->id(), &item4->id()));
 
   // Make sure you can't make an item be it's own parent.
-  ASSERT_FALSE(manager_.ChangeParent(id1, id1));
+  ASSERT_FALSE(manager_.ChangeParent(item1->id(), &item1->id()));
 }
 
 // Tests that we properly remove an extension's menu item when that extension is
@@ -246,17 +242,16 @@ TEST_F(ExtensionMenuManagerTest, ExtensionUnloadRemovesMenuItems) {
 
   // Create an ExtensionMenuItem and put it into the manager.
   ExtensionMenuItem* item1 = CreateTestItem(extension1);
+  ExtensionMenuItem::Id id1 = item1->id();
   ASSERT_EQ(extension1->id(), item1->extension_id());
-  int id1 = manager_.AddContextItem(extension1, item1);
-  ASSERT_GT(id1, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
   ASSERT_EQ(1u, manager_.MenuItems(extension1->id())->size());
 
   // Create a menu item with a different extension id and add it to the manager.
   Extension* extension2 = AddExtension("2222");
   ExtensionMenuItem* item2 = CreateTestItem(extension2);
   ASSERT_NE(item1->extension_id(), item2->extension_id());
-  int id2 = manager_.AddContextItem(extension2, item2);
-  ASSERT_GT(id2, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item2));
 
   // Notify that the extension was unloaded, and make sure the right item is
   // gone.
@@ -266,7 +261,7 @@ TEST_F(ExtensionMenuManagerTest, ExtensionUnloadRemovesMenuItems) {
   ASSERT_EQ(NULL, manager_.MenuItems(extension1->id()));
   ASSERT_EQ(1u, manager_.MenuItems(extension2->id())->size());
   ASSERT_TRUE(manager_.GetItemById(id1) == NULL);
-  ASSERT_TRUE(manager_.GetItemById(id2) != NULL);
+  ASSERT_TRUE(manager_.GetItemById(item2->id()) != NULL);
 }
 
 // A mock message service for tests of ExtensionMenuManager::ExecuteCommand.
@@ -305,17 +300,14 @@ TEST_F(ExtensionMenuManagerTest, RemoveAll) {
   ExtensionMenuItem* item1 = CreateTestItem(extension1);
   ExtensionMenuItem* item2 = CreateTestItem(extension1);
   ExtensionMenuItem* item3 = CreateTestItem(extension1);
-  int id1 = manager_.AddContextItem(extension1, item1);
-  int id2 = manager_.AddContextItem(extension1, item2);
-  EXPECT_GT(id1, 0);
-  EXPECT_GT(id2, 0);
-  int id3 = manager_.AddChildItem(id1, item3);
-  EXPECT_GT(id3, 0);
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item1));
+  ASSERT_TRUE(manager_.AddContextItem(extension1, item2));
+  ASSERT_TRUE(manager_.AddChildItem(item1->id(), item3));
 
   // Add one top-level item for extension 2.
   Extension* extension2 = AddExtension("2222");
   ExtensionMenuItem* item4 = CreateTestItem(extension2);
-  manager_.AddContextItem(extension2, item4);
+  ASSERT_TRUE(manager_.AddContextItem(extension2, item4));
 
   EXPECT_EQ(2u, manager_.MenuItems(extension1->id())->size());
   EXPECT_EQ(1u, manager_.MenuItems(extension2->id())->size());
@@ -348,8 +340,8 @@ TEST_F(ExtensionMenuManagerTest, ExecuteCommand) {
 
   Extension* extension = AddExtension("test");
   ExtensionMenuItem* item = CreateTestItem(extension);
-  int id = manager_.AddContextItem(extension, item);
-  ASSERT_GT(id, 0);
+  ExtensionMenuItem::Id id = item->id();
+  ASSERT_TRUE(manager_.AddContextItem(extension, item));
 
   EXPECT_CALL(profile, GetExtensionMessageService())
       .Times(1)
@@ -387,7 +379,7 @@ TEST_F(ExtensionMenuManagerTest, ExecuteCommand) {
 
   int tmp_id = 0;
   ASSERT_TRUE(info->GetInteger(L"menuItemId", &tmp_id));
-  ASSERT_EQ(id, tmp_id);
+  ASSERT_EQ(id.second, tmp_id);
 
   std::string tmp;
   ASSERT_TRUE(info->GetString(L"mediaType", &tmp));