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..388512c4447059943a657216a4532e0934d08449 100644
--- a/chrome/browser/extensions/extension_context_menu_model_unittest.cc
+++ b/chrome/browser/extensions/extension_context_menu_model_unittest.cc
@@ -4,10 +4,14 @@
 
 #include "chrome/browser/extensions/extension_context_menu_model.h"
 
+#include "chrome/browser/extensions/context_menu_matcher.h"

[Devlin, 2014/07/29 20:49:46]

Do we need this?

[gpdavis, 2014/07/30 20:53:02]

Ditto last comment.  Done.

 #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/common/extensions/api/context_menus.h"
 #include "chrome/test/base/test_browser_window.h"
 #include "chrome/test/base/testing_profile.h"
 #include "extensions/browser/extension_system.h"
@@ -17,25 +21,92 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
-namespace {
+
+const char kPageAction[] = "page_action";
 
 class ExtensionContextMenuModelTest : public ExtensionServiceTestBase {
+ public:
+  ExtensionContextMenuModelTest();
+
+  // Build an extension to pass to the menu constructor.  It needs an
+  // ExtensionAction.
+  scoped_refptr<Extension> BuildExtension(const char* action_type);

[Devlin, 2014/07/29 20:49:46]

It looks like |action_type| is always just kPageAction...?

[gpdavis, 2014/07/30 20:53:02]

I added this in to make it simple to create a browser_action if necessary, but
didn't end up using it in my test (and had to remove the associated constant
since it was an unused variable).  I figured I'd leave it in since it seems
reasonable to pass in the type of action, but I'll revert it back since it's
unnecessary.

+
+  // 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);
+
+  // Reinitializes the given |model|.
+  void RefreshMenu(ExtensionContextMenuModel* model);
+
+  // Returns the number of extension menu items that show up in |model|.
+  int CountExtensionItems(ExtensionContextMenuModel* model);
+
+  // TestingFactoryFunction for setting up a MenuManager for testing.
+  static KeyedService* BuildMenuManagerForTesting(
+      content::BrowserContext* context);
+
+ private:
+  int cur_id_;
 };
 
+ExtensionContextMenuModelTest::ExtensionContextMenuModelTest()
+    : cur_id_(0) {}
+
+scoped_refptr<Extension> ExtensionContextMenuModelTest::BuildExtension(
+    const char* 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();
+}
+
+void ExtensionContextMenuModelTest::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,  // checked
+                                                  true,  // enabled
+                                                  type,
+                                                  contexts));
+}
+
+void ExtensionContextMenuModelTest::RefreshMenu(
+    ExtensionContextMenuModel* model) {
+  model->InitMenu(model->GetExtension());
+}
+
+int ExtensionContextMenuModelTest::CountExtensionItems(
+    ExtensionContextMenuModel* model) {
+  return model->extension_items_count_;
+}
+
+// static
+KeyedService* ExtensionContextMenuModelTest::BuildMenuManagerForTesting(
+      content::BrowserContext* context) {
+    return MenuManagerFactory::GetInstance()->BuildServiceInstanceFor(context);

[Devlin, 2014/07/29 20:49:45]

nit: indentation

[gpdavis, 2014/07/30 20:53:02]

Done.

+}
+
+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(kPageAction);
   ASSERT_TRUE(extension.get());
   service_->AddExtension(extension.get());
 
@@ -46,7 +117,7 @@ TEST_F(ExtensionContextMenuModelTest, PolicyDisablesItems) {
   Browser browser(params);
 
   scoped_refptr<ExtensionContextMenuModel> menu(
-      new ExtensionContextMenuModel(extension.get(), &browser, NULL));
+      new ExtensionContextMenuModel(extension.get(), &browser));
 
   extensions::ExtensionSystem* system =
       extensions::ExtensionSystem::Get(profile_.get());
@@ -66,5 +137,64 @@ TEST_F(ExtensionContextMenuModelTest, PolicyDisablesItems) {
   system->management_policy()->UnregisterAllProviders();
 }
 
+TEST_F(ExtensionContextMenuModelTest, ExtensionItemTest) {
+  InitializeEmptyExtensionService();
+  scoped_refptr<Extension> extension = BuildExtension(kPageAction);
+  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(
+          profile_.get(), &BuildMenuManagerForTesting)));
+  ASSERT_TRUE(manager);
+
+  scoped_refptr<ExtensionContextMenuModel> menu(
+      new ExtensionContextMenuModel(extension.get(), &browser));
+
+  // There should be no extension items yet.
+  EXPECT_EQ(0, CountExtensionItems(menu));
+
+  // Add a browser action menu item for |extension| to |manager|.
+  AddContextItem(manager, extension.get(), MenuItem::BROWSER_ACTION);
+  RefreshMenu(menu);
+
+  // Since |extension| has a page action, the browser action menu item should
+  // not be present.
+  EXPECT_EQ(0, CountExtensionItems(menu));
+
+  // Add a page action menu item and reset the context menu.
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  RefreshMenu(menu);
+
+  // The page action item should be present because |extension| has a page
+  // action.
+  EXPECT_EQ(1, CountExtensionItems(menu));
+
+  // Create more page action items to test top level menu item limitations.
+  for (int i = 0; i < api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT; ++i)
+    AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  RefreshMenu(menu);
+
+  // The menu should only have a limited number of extension items, since they
+  // are all top level items, and we limit the number of top level extension
+  // items.
+  EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
+            CountExtensionItems(menu));
+
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  RefreshMenu(menu);

[Devlin, 2014/07/29 20:49:46]

The fact we say "RefreshMenu" every time we AddContextItem() (with the
semi-exception of the for-loop) makes me think that it should just be done in
AddContextItem().

[gpdavis, 2014/07/30 20:53:02]

That's probably a good idea.  I was going to combine them, but I figured I'd
leave RefreshMenu as its own method in case future tests want to refresh the
model without adding an item.  Done.

+
+  // Adding another top level item should not increase the count.
+  EXPECT_EQ(api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT,
+            CountExtensionItems(menu));
+}
+
 }  // namespace
 }  // namespace extensions