[Extensions Toolbar] Only include relevant items in component extensions' menus

Only include items that are relevant to component extensions in their context
menus. That is to say, don't show the "Remove", "Manage extensions", or "Inspect
popup" items, and only show the "Options" item if there is an options page.

BUG=486223

Review URL: https://codereview.chromium.org/1135763004

Cr-Commit-Position: refs/heads/master@{#329928}

chrome/browser/extensions/extension_context_menu_model.cc

@@ -26,6 +26,7 @@
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
+#include "chrome/grit/theme_resources.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/context_menu_params.h"
 #include "extensions/browser/extension_prefs.h"
@@ -38,6 +39,8 @@
 #include "extensions/common/manifest_handlers/options_page_info.h"
 #include "extensions/common/manifest_url_handlers.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/image/image.h"
 
 using content::OpenURLParams;
 using content::Referrer;
@@ -113,6 +116,8 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(
     PopupDelegate* delegate)
     : SimpleMenuModel(this),
       extension_id_(extension->id()),
+      is_component_(extensions::Manifest::IsComponentLocation(
+                        extension->location())),
       browser_(browser),
       profile_(browser->profile()),
       delegate_(delegate),
@@ -121,7 +126,8 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(
   InitMenu(extension, button_visibility);
 
   if (profile_->GetPrefs()->GetBoolean(prefs::kExtensionsUIDeveloperMode) &&
-      delegate_) {
+      delegate_ &&
+      !is_component_) {
     AddSeparator(ui::NORMAL_SEPARATOR);
     AddItemWithStringId(INSPECT_POPUP, IDS_EXTENSION_ACTION_INSPECT_POPUP);
   }
@@ -129,14 +135,7 @@ ExtensionContextMenuModel::ExtensionContextMenuModel(
 
 ExtensionContextMenuModel::ExtensionContextMenuModel(const Extension* extension,
                                                      Browser* browser)
-    : SimpleMenuModel(this),
-      extension_id_(extension->id()),
-      browser_(browser),
-      profile_(browser->profile()),
-      delegate_(NULL),
-      action_type_(NO_ACTION),
-      extension_items_count_(0) {
-  InitMenu(extension, VISIBLE);
+    : ExtensionContextMenuModel(extension, browser, VISIBLE, nullptr) {
 }
 
 bool ExtensionContextMenuModel::IsCommandIdChecked(int command_id) const {
@@ -158,8 +157,11 @@ bool ExtensionContextMenuModel::IsCommandIdEnabled(int command_id) const {
     return extensions::OptionsPageInfo::HasOptionsPage(extension);
   } else if (command_id == NAME) {
     // The NAME links to the Homepage URL. If the extension doesn't have a
-    // homepage, we just disable this menu item.
-    return extensions::ManifestURL::GetHomepageURL(extension).is_valid();
+    // homepage, we just disable this menu item. We also disable for component
+    // extensions, because it doesn't make sense to link to a webstore page or
+    // chrome://extensions.
+    return extensions::ManifestURL::GetHomepageURL(extension).is_valid() &&
+        !is_component_;
   } else if (command_id == INSPECT_POPUP) {
     WebContents* web_contents = GetActiveWebContents();
     if (!web_contents)
@@ -301,8 +303,11 @@ void ExtensionContextMenuModel::InitMenu(const Extension* extension,
     AddItemWithStringId(ALWAYS_RUN, IDS_EXTENSIONS_ALWAYS_RUN);
   }
 
-  AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS_MENU_ITEM);
-  AddItem(UNINSTALL, l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL));
+  if (!is_component_ || extensions::OptionsPageInfo::HasOptionsPage(extension))
+    AddItemWithStringId(CONFIGURE, IDS_EXTENSIONS_OPTIONS_MENU_ITEM);
+
+  if (!is_component_)
+    AddItem(UNINSTALL, l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNINSTALL));
 
   // Add a toggle visibility (show/hide) if the extension icon is shown on the
   // toolbar.
@@ -311,8 +316,10 @@ void ExtensionContextMenuModel::InitMenu(const Extension* extension,
   if (visibility_string_id != -1)
     AddItemWithStringId(TOGGLE_VISIBILITY, visibility_string_id);
 
-  AddSeparator(ui::NORMAL_SEPARATOR);
-  AddItemWithStringId(MANAGE, IDS_MANAGE_EXTENSION);
+  if (!is_component_) {
+    AddSeparator(ui::NORMAL_SEPARATOR);
+    AddItemWithStringId(MANAGE, IDS_MANAGE_EXTENSION);
+  }
 }
 
 const Extension* ExtensionContextMenuModel::GetExtension() const {

chrome/browser/extensions/extension_context_menu_model.h

@@ -115,6 +115,9 @@ class ExtensionContextMenuModel
   // A copy of the extension's id.
   std::string extension_id_;
 
+  // Whether the menu is for a component extension.
+  bool is_component_;
+
   // The extension action of the extension we are displaying the menu for (if
   // it has one, otherwise NULL).
   ExtensionAction* extension_action_;

chrome/browser/extensions/extension_context_menu_model_unittest.cc

@@ -23,9 +23,11 @@
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/manifest_handlers/options_page_info.h"
 #include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/image/image.h"
 
 namespace extensions {
 
@@ -93,7 +95,6 @@ class ExtensionContextMenuModelTest : public ExtensionServiceTestBase {
 ExtensionContextMenuModelTest::ExtensionContextMenuModelTest() : cur_id_(0) {
 }
 
-
 void ExtensionContextMenuModelTest::AddContextItemAndRefreshModel(
     MenuManager* manager,
     const Extension* extension,
@@ -129,50 +130,90 @@ int ExtensionContextMenuModelTest::CountExtensionItems(
 TEST_F(ExtensionContextMenuModelTest, RequiredInstallationsDisablesItems) {
   InitializeEmptyExtensionService();
 
-  // First, test that a component extension cannot be uninstalled by the
-  // standard management policy.
+  // Test that management policy can determine whether or not policy-installed
+  // extensions can be installed/uninstalled.
   scoped_refptr<const Extension> extension =
-      BuildExtension("component",
-                     manifest_keys::kBrowserAction,
-                     Manifest::COMPONENT);
+      BuildExtension("extension",
+                     manifest_keys::kPageAction,
+                     Manifest::EXTERNAL_POLICY);
   ASSERT_TRUE(extension.get());
   service()->AddExtension(extension.get());
 
   scoped_ptr<Browser> browser = CreateBrowser(profile());
-
   scoped_refptr<ExtensionContextMenuModel> menu(
       new ExtensionContextMenuModel(extension.get(), browser.get()));
 
-  // Uninstallation should be disabled.
-  EXPECT_FALSE(menu->IsCommandIdEnabled(ExtensionContextMenuModel::UNINSTALL));
-
-  // Also test that management policy can determine whether or not
-  // policy-installed extensions can be installed/uninstalled.
-  extension = BuildExtension("extension",
-                             manifest_keys::kPageAction,
-                             Manifest::INTERNAL);
-  ASSERT_TRUE(extension.get());
-  service()->AddExtension(extension.get());
-
-  menu = new ExtensionContextMenuModel(extension.get(), browser.get());
-
   ExtensionSystem* system = ExtensionSystem::Get(profile());
   system->management_policy()->UnregisterAllProviders();
 
-  // Actions should be enabled.
+  // Uninstallation should be, by default, enabled.
   ASSERT_TRUE(menu->IsCommandIdEnabled(ExtensionContextMenuModel::UNINSTALL));
 
   TestManagementPolicyProvider policy_provider(
       TestManagementPolicyProvider::PROHIBIT_MODIFY_STATUS);
   system->management_policy()->RegisterProvider(&policy_provider);
 
-  // Now the actions are disabled.
+  // If there's a policy provider that requires the extension stay enabled, then
+  // uninstallation should be disabled.
   ASSERT_FALSE(menu->IsCommandIdEnabled(ExtensionContextMenuModel::UNINSTALL));
 
   // Don't leave |policy_provider| dangling.
   system->management_policy()->UnregisterProvider(&policy_provider);
 }
 
+// Tests the context menu for a component extension.
+TEST_F(ExtensionContextMenuModelTest, ComponentExtensionContextMenu) {
+  InitializeEmptyExtensionService();
+
+  std::string name("component");
+  scoped_ptr<base::DictionaryValue> manifest =
+      DictionaryBuilder().Set("name", name)
+                         .Set("version", "1")
+                         .Set("manifest_version", 2)
+                         .Set("browser_action", DictionaryBuilder().Pass())
+                         .Build();
+
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder().SetManifest(make_scoped_ptr(manifest->DeepCopy()))
+                        .SetID(crx_file::id_util::GenerateId("component"))
+                        .SetLocation(Manifest::COMPONENT)
+                        .Build();
+  service()->AddExtension(extension.get());
+
+  scoped_ptr<Browser> browser = CreateBrowser(profile());
+
+  scoped_refptr<ExtensionContextMenuModel> menu(
+      new ExtensionContextMenuModel(extension.get(), browser.get()));
+
+  // A component extension's context menu should not include options for
+  // managing extensions or removing it, and should only include an option for
+  // the options page if the extension has one (which this one doesn't).
+  EXPECT_EQ(-1,
+            menu->GetIndexOfCommandId(ExtensionContextMenuModel::CONFIGURE));
+  EXPECT_EQ(-1,
+            menu->GetIndexOfCommandId(ExtensionContextMenuModel::UNINSTALL));
+  EXPECT_EQ(-1, menu->GetIndexOfCommandId(ExtensionContextMenuModel::MANAGE));
+  // The "name" option should be present, but not enabled for component
+  // extensions.
+  EXPECT_NE(-1, menu->GetIndexOfCommandId(ExtensionContextMenuModel::NAME));
+  EXPECT_FALSE(menu->IsCommandIdEnabled(ExtensionContextMenuModel::NAME));
+
+  // Check that a component extension with an options page does have the options
+  // menu item, and it is enabled.
+  manifest->SetString("options_page", "options_page.html");
+  extension =
+      ExtensionBuilder().SetManifest(manifest.Pass())
+                        .SetID(crx_file::id_util::GenerateId("component_opts"))
+                        .SetLocation(Manifest::COMPONENT)
+                        .Build();
+  menu = new ExtensionContextMenuModel(extension.get(), browser.get());
+  service()->AddExtension(extension.get());
+  EXPECT_TRUE(extensions::OptionsPageInfo::HasOptionsPage(extension.get()));
+  EXPECT_NE(-1,
+            menu->GetIndexOfCommandId(ExtensionContextMenuModel::CONFIGURE));
+  EXPECT_TRUE(menu->IsCommandIdEnabled(ExtensionContextMenuModel::CONFIGURE));
+}
+
 TEST_F(ExtensionContextMenuModelTest, ExtensionItemTest) {
   InitializeEmptyExtensionService();
   scoped_refptr<const Extension> extension =