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..b0f40778d131d43971c504a14307bc1a52adb0ea 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;

[Devlin, 2014/07/24 21:51:28]

why not just include the .h?

[gpdavis, 2014/07/28 21:08:22]

Done.

+
+}  // 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) {

[Devlin, 2014/07/24 21:51:27]

If you take my suggestion on line 80, this should be a const char*. Otherwise,
this should be a const std::string&. It should not take a copy.

[Devlin, 2014/07/24 21:51:27]

even in tests, we *should* have the practice of not inlining our functions. 
(You are certainly not the only one who does this, but let's set a good example
for everyone else :))

[gpdavis, 2014/07/28 21:08:22]

Sure thing!  I wasn't sure if this was still protocol, since surrounding code is
different and there's no header file for this.  Should there be a header for the
class declaration?

[gpdavis, 2014/07/28 21:08:22]

Done (parameter change).

[Yoyo Zhou, 2014/07/28 23:23:25]

You don't need a separate header file unless you need to include the class (e.g.
as a base class, like ExtensionServiceTestBase).

+    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));

[Devlin, 2014/07/24 21:51:28]

doc anon bools

[gpdavis, 2014/07/28 21:08:22]

Done.

+  }
+
+  // Reinitializes the given |model|.
+  void RefreshMenu(ExtensionContextMenuModel* model) {
+    model->InitMenu(model->GetExtension());
+  }
+
+  // Returns the number of extension menu items that show up in |model|.
+  int CountExtensionItems(ExtensionContextMenuModel* model) {
+    return model->extension_items_count_;
+  }
+
+  int cur_id = 0;

[Devlin, 2014/07/24 21:51:28]

Similarly, we should not have public class variables.  Protected might be okay,
though I prefer making them private if possible.  Also, this is initialized
incorrectly.

[gpdavis, 2014/07/28 21:08:22]

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("page_action");

[Devlin, 2014/07/24 21:51:28]

Why don't we define these as const char[]s kPageAction and kBrowserAction.

[gpdavis, 2014/07/28 21:08:22]

Done.  I left out kBrowserAction for now, since it's an unused variable.

   ASSERT_TRUE(extension.get());
   service_->AddExtension(extension.get());
 
@@ -66,5 +108,65 @@ 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*>(

[Yoyo Zhou, 2014/07/24 22:21:03]

What about creating a MenuManager without using the factory?

MenuManager manager(profile, state_store);

[gpdavis, 2014/07/28 21:08:22]

MenuManager::Get(profile_.get()) returns null if I do it this way.

+      (MenuManagerFactory::GetInstance()->SetTestingFactoryAndUse(
+          profile_.get(), &MenuManagerFactory::BuildInstanceFor)));
+  ASSERT_TRUE(manager);
+
+  scoped_refptr<ExtensionContextMenuModel> menu(
+      new ExtensionContextMenuModel(extension.get(), &browser, NULL));

[Devlin, 2014/07/24 21:51:28]

doc NULL

[gpdavis, 2014/07/28 21:08:22]

I can actually just use the other constructor, which doesn't take a delegate
instead of passing in NULL.  Done.

+
+  // There should be no extension items yet.
+  ASSERT_TRUE(CountExtensionItems(menu) == 0);

[Devlin, 2014/07/24 21:51:28]

Some of these can be EXPECTs, and you should also be using ASSERT/EXPECT_EQ for
equality operations.

[gpdavis, 2014/07/28 21:08:22]

Done.

+
+  // 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.
+  ASSERT_TRUE(CountExtensionItems(menu) == 0);
+
+  // 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.
+  ASSERT_TRUE(CountExtensionItems(menu) == 1);
+
+  // 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) {

[Devlin, 2014/07/24 21:51:28]

nit: no brackets.

[gpdavis, 2014/07/28 21:08:22]

Done.

+    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.
+  ASSERT_TRUE(CountExtensionItems(menu) ==
+              api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT);
+
+  AddContextItem(manager, extension.get(), MenuItem::PAGE_ACTION);
+  RefreshMenu(menu);
+
+  // Adding another top level item should not increase the count.
+  ASSERT_TRUE(CountExtensionItems(menu) ==
+              api::context_menus::ACTION_MENU_TOP_LEVEL_LIMIT);
+}
+
 }  // namespace
 }  // namespace extensions