Extend contextMenus API to support browser/page actions

chrome/browser/extensions/extension_context_menu_model_unittest.cc

Index: chrome/browser/extensions/extension_context_menu_model_unittest.cc
diff --git a/chrome/browser/extensions/extension_context_menu_model_unittest.cc b/chrome/browser/extensions/extension_context_menu_model_unittest.cc
index f02d6e3390af570bd1ba1a856140fd1e659d1253..d930648039987300203dc9d6cb1f7a82ca1bb83f 100644
--- a/chrome/browser/extensions/extension_context_menu_model_unittest.cc
+++ b/chrome/browser/extensions/extension_context_menu_model_unittest.cc
@@ -4,8 +4,11 @@
 
 #include "chrome/browser/extensions/extension_context_menu_model.h"
 
+#include "chrome/browser/extensions/context_menu_matcher.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
+#include "chrome/browser/extensions/menu_manager.h"
+#include "chrome/browser/extensions/menu_manager_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/test/base/test_browser_window.h"
@@ -17,25 +20,64 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
-namespace {
+namespace api {
+namespace context_menus {
+
+extern const int ACTION_MENU_TOP_LEVEL_LIMIT;
+
+}  // namespace context_menus
+}  // namespace api
 
 class ExtensionContextMenuModelTest : public ExtensionServiceTestBase {
+ public:
+  // Build an extension to pass to the menu constructor.  It needs an
+  // ExtensionAction.
+  scoped_refptr<Extension> BuildExtension(std::string action_type) {
+    return ExtensionBuilder()
+        .SetManifest(
+             DictionaryBuilder()
+                 .Set("name", "Page Action Extension")
+                 .Set("version", "1")
+                 .Set("manifest_version", 2)
+                 .Set(action_type,
+                      DictionaryBuilder().Set("default_title", "Hello")))
+        .Build();
+  }
+
+  // Creates an extension menu item for |extension| with the given |context|
+  // and adds it to |manager|.
+  void AddContextItem(MenuManager* manager,
+                      Extension* extension,
+                      MenuItem::Context context) {
+    MenuItem::Type type = MenuItem::NORMAL;
+    MenuItem::ContextList contexts(context);
+    const MenuItem::ExtensionKey key(extension->id());
+    MenuItem::Id id(false, key);
+    id.uid = ++cur_id;
+    manager->AddContextItem(
+        extension, new MenuItem(id, "test", false, true, type, contexts));
+  }
+
+  // Reinitializes the given |model|.
+  void ResetMenu(ExtensionContextMenuModel* model) {

[Yoyo Zhou, 2014/07/22 00:44:32]

"Reset" has kind of ambiguous connotations. Maybe call it Refresh?

[gpdavis, 2014/07/22 18:52:53]

Done.

+    model->InitMenu(model->GetExtension());
+  }
+
+  // Returns the number of extension menu items that show up in |model|.
+  int DetectExtensionItems(ExtensionContextMenuModel* model) {

[Yoyo Zhou, 2014/07/22 00:44:32]

nit: CountExtensionItems?

[gpdavis, 2014/07/22 18:52:52]

Done.

+    return model->extension_items_count_;
+  }
+
+  int cur_id = 0;
 };
 
+namespace {
+
 // Tests that applicable menu items are disabled when a ManagementPolicy
 // prohibits them.
 TEST_F(ExtensionContextMenuModelTest, PolicyDisablesItems) {
   InitializeEmptyExtensionService();
-  // Build an extension to pass to the menu constructor.  It needs an
-  // ExtensionAction.
-  scoped_refptr<Extension> extension = ExtensionBuilder()
-      .SetManifest(DictionaryBuilder()
-                   .Set("name", "Page Action Extension")
-                   .Set("version", "1")
-                   .Set("manifest_version", 2)
-                   .Set("page_action", DictionaryBuilder()
-                        .Set("default_title", "Hello")))
-      .Build();
+  scoped_refptr<Extension> extension = BuildExtension("page_action");
   ASSERT_TRUE(extension.get());
   service_->AddExtension(extension.get());
 
@@ -66,5 +108,67 @@ TEST_F(ExtensionContextMenuModelTest, PolicyDisablesItems) {
   system->management_policy()->UnregisterAllProviders();
 }
 
+TEST_F(ExtensionContextMenuModelTest, ExtensionItemTest) {
+  InitializeEmptyExtensionService();
+  scoped_refptr<Extension> extension = BuildExtension("page_action");
+  ASSERT_TRUE(extension.get());
+  service_->AddExtension(extension.get());
+
+  // Create a Browser for the ExtensionContextMenuModel to use.
+  Browser::CreateParams params(profile_.get(), chrome::GetActiveDesktop());
+  TestBrowserWindow test_window;
+  params.window = &test_window;
+  Browser browser(params);
+
+  // Create a MenuManager for adding context items.
+  MenuManager* manager = static_cast<MenuManager*>(
+      (MenuManagerFactory::GetInstance()->SetTestingFactoryAndUse(

[Yoyo Zhou, 2014/07/22 00:44:32]

I don't understand what's going on here. It looks like you're just using the
standard factory, not a testing factory. Just call GetForProfile?

[gpdavis, 2014/07/22 18:52:53]

Calling GetForProfile returned NULL, so I needed to use BuildInstanceFor (see
changes to menu_manager_factory).

+          profile_.get(), &MenuManagerFactory::BuildInstanceFor)));
+  ASSERT_TRUE(manager);
+
+  scoped_refptr<ExtensionContextMenuModel> menu(
+      new ExtensionContextMenuModel(extension.get(), &browser, NULL));
+
+  // There should be no extension items yet.
+  ASSERT_TRUE(DetectExtensionItems(menu) == 0);
+
+  // Add a browser action menu item for |extension| to |manager|.
+  AddContextItem(manager, extension.get(), MenuItem::BROWSER_ACTION);
+  ResetMenu(menu);
+
+  // Since |extension| has a page action, the browser action menu item should
+  // not be detected.

[Yoyo Zhou, 2014/07/22 00:44:32]

'detected' is kind of odd. Say 'present'.

[gpdavis, 2014/07/22 18:52:52]

Done.

+  ASSERT_TRUE(DetectExtensionItems(menu) == 0);
+
+  // Add a page action menu item and reset the context menu.
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  ResetMenu(menu);
+
+  // The page action item should be detected because |extension| has a page

[Yoyo Zhou, 2014/07/22 00:44:32]

ditto

[gpdavis, 2014/07/22 18:52:52]

Done.

+  // action.
+  ASSERT_TRUE(DetectExtensionItems(menu) == 1);
+
+  // Create six more page action items to test top level menu item limitations.
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  ResetMenu(menu);
+
+  // The menu should only have six extension items, since they are all top level
+  // items, and we limit the number of top level extension items to six.

[Yoyo Zhou, 2014/07/22 00:44:32]

Ok. By the way, do we count separators as top-level items for this purpose?
(Either way is possibly valid, I'm just curious.) And what happens to the 7th
item?

[gpdavis, 2014/07/22 18:52:52]

User added separators (via create calls in the extension JS) are counted as top
level items, but separators that are automatically added (as in the case of
radio items) do not count towards the count.

Any extra items are simply not appended to the action context menu model when
AppendExtensionItems is called in the ContextMenuMatcher.

[Yoyo Zhou, 2014/07/22 21:35:42]

So additional top-level items beyond the 6th are just silently dropped? This
should be documented.

[not at google - send to devlin, 2014/07/22 22:58:07]

it would be better if it set an error, if possible (if that method can take a
callback which communiates an error)

+  ASSERT_TRUE(DetectExtensionItems(menu) ==
+              api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT);
+
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  ResetMenu(menu);
+
+  // Adding another top level item should not increase the count.
+  ASSERT_TRUE(DetectExtensionItems(menu) ==
+              api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT);
+}
+
 }  // namespace
 }  // namespace extensions