Allow extensions to remove and override the bookmark shortcut key

This feature is enabled for dev behind the
--enable-override-bookmarks-ui=1 feature flag, and for all releases for
internal bookmarks extensions.

Implements the shortcut key aspect of the
Remove Bookmark Shortcut Chrome API proposal:
https://docs.google.com/a/chromium.org/document/d/1C2Mle92O9uGlji5y5gGDM5tNJ_tVE1Vb-2xgsZPNDTk

BUG=335655
R=erg@chromium.org, finnur@chromium.org, shess@chromium.org, sky@chromium.org

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

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

chrome/browser/extensions/api/commands/command_service.cc

@@ -11,6 +11,7 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/commands/commands.h"
 #include "chrome/browser/extensions/extension_commands_global_registry.h"
@@ -20,6 +21,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/accelerator_utils.h"
 #include "chrome/common/extensions/api/commands/commands_handler.h"
+#include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/notification_details.h"
@@ -27,6 +29,7 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/permissions/permissions_data.h"
 
 using extensions::Extension;
 using extensions::ExtensionPrefs;
@@ -78,22 +81,59 @@ bool InitialBindingsHaveBeenAssigned(
   return assigned;
 }
 
-bool IsWhitelistedGlobalShortcut(const extensions::Command& command) {
-  // Non-global shortcuts are always allowed.
-  if (!command.global())
+// Checks if |extension| is permitted to automatically assign the |accelerator|
+// key.
+bool CanAutoAssign(const ui::Accelerator& accelerator,
+                   const Extension* extension,
+                   Profile* profile,
+                   bool is_named_command,
+                   bool is_global) {
+  // Media Keys are non-exclusive, so allow auto-assigning them.
+  if (extensions::CommandService::IsMediaKey(accelerator))
     return true;
-  // Global shortcuts must be (Ctrl|Command)-Shift-[0-9].
+
+  if (is_global) {
+    if (!is_named_command)
+      return false;  // Browser and page actions are not global in nature.
+
+    // Global shortcuts are restricted to (Ctrl|Command)+Shift+[0-9].
 #if defined OS_MACOSX
-  if (!command.accelerator().IsCmdDown())
+    if (!accelerator.IsCmdDown())
       return false;
 #else
-  if (!command.accelerator().IsCtrlDown())
+    if (!accelerator.IsCtrlDown())
       return false;
 #endif
-  if (!command.accelerator().IsShiftDown())
+    if (!accelerator.IsShiftDown())
       return false;
-  return (command.accelerator().key_code() >= ui::VKEY_0 &&
-          command.accelerator().key_code() <= ui::VKEY_9);
+    return (accelerator.key_code() >= ui::VKEY_0 &&
+            accelerator.key_code() <= ui::VKEY_9);
+  } else {
+    // Not a global command, check if Chrome shortcut and whether
+    // we can override it.
+    if (accelerator ==
+        chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE)) {
+      using extensions::SettingsOverrides;
+      using extensions::FeatureSwitch;
+      const SettingsOverrides* settings_overrides =
+          SettingsOverrides::Get(extension);
+      if (settings_overrides &&
+          SettingsOverrides::RemovesBookmarkShortcut(*settings_overrides) &&
+          (extensions::PermissionsData::HasAPIPermission(
+              extension,
+              extensions::APIPermission::kBookmarkManagerPrivate) ||
+           FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled())) {
+        // If this check fails it either means we have an API to override a
+        // key that isn't a ChromeAccelerator (and the API can therefore be
+        // deprecated) or the IsChromeAccelerator isn't consistently
+        // returning true for all accelerators.
+        DCHECK(chrome::IsChromeAccelerator(accelerator, profile));
+        return true;
+      }
+    }
+
+    return !chrome::IsChromeAccelerator(accelerator, profile);
+  }
 }
 
 }  // namespace
@@ -362,43 +402,48 @@ void CommandService::AssignInitialKeybindings(const Extension* extension) {
 
   extensions::CommandMap::const_iterator iter = commands->begin();
   for (; iter != commands->end(); ++iter) {
-    // Make sure registered Chrome shortcuts cannot be automatically assigned
-    // (overwritten) by extension developers. Media keys are an exception here.
-    if ((!chrome::IsChromeAccelerator(iter->second.accelerator(), profile_) &&
-        IsWhitelistedGlobalShortcut(iter->second)) ||
-        extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
-      AddKeybindingPref(iter->second.accelerator(),
+    const extensions::Command command = iter->second;
+    if (CanAutoAssign(command.accelerator(),
+                      extension,
+                      profile_,
+                      true,  // Is a named command.
+                      command.global())) {
+      AddKeybindingPref(command.accelerator(),
                         extension->id(),
-                        iter->second.command_name(),
+                        command.command_name(),
                         false,  // Overwriting not allowed.
-                        iter->second.global());
+                        command.global());
     }
   }
 
   const extensions::Command* browser_action_command =
       CommandsInfo::GetBrowserActionCommand(extension);
-  if (browser_action_command) {
-    if (!chrome::IsChromeAccelerator(
-        browser_action_command->accelerator(), profile_)) {
+  if (browser_action_command &&
+      CanAutoAssign(browser_action_command->accelerator(),
+                    extension,
+                    profile_,
+                    false,     // Not a named command.
+                    false)) {  // Not global.
     AddKeybindingPref(browser_action_command->accelerator(),
                       extension->id(),
                       browser_action_command->command_name(),
                       false,   // Overwriting not allowed.
-                        false);  // Browser actions can't be global.
-    }
+                      false);  // Not global.
   }
 
   const extensions::Command* page_action_command =
       CommandsInfo::GetPageActionCommand(extension);
-  if (page_action_command) {
-    if (!chrome::IsChromeAccelerator(
-        page_action_command->accelerator(), profile_)) {
+  if (page_action_command &&
+      CanAutoAssign(page_action_command->accelerator(),
+                    extension,
+                    profile_,
+                    false,     // Not a named command.
+                    false)) {  // Not global.
     AddKeybindingPref(page_action_command->accelerator(),
                       extension->id(),
                       page_action_command->command_name(),
                       false,   // Overwriting not allowed.
-                        false);  // Page actions can't be global.
-    }
+                      false);  // Not global.
   }
 }
 

chrome/browser/extensions/extension_keybinding_apitest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/command_line.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_action.h"
@@ -188,7 +189,7 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, MAYBE_DontOverwriteSystemShortcuts) {
   WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(tab);
 
-  // Activate the shortcut (Alt+Shift+F) to make page blue.
+  // Activate the shortcut (Alt+Shift+F) to make the page blue.
   {
     ResultCatcher catcher;
     ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
@@ -205,7 +206,27 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, MAYBE_DontOverwriteSystemShortcuts) {
       &result));
   ASSERT_TRUE(result);
 
-  // Activate the shortcut (Ctrl+F) to make page red (should not work).
+  // Activate the bookmark shortcut (Ctrl+D) to make the page green (should not
+  // work without requesting via chrome_settings_overrides).
+#if defined(OS_MACOSX)
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_D, false, false, false, true));
+#else
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_D, true, false, false, false));
+#endif
+
+  // The page should still be blue.
+  result = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      tab,
+      "setInterval(function() {"
+      "  if (document.body.bgColor == 'blue') {"
+      "    window.domAutomationController.send(true)}}, 100)",
+      &result));
+  ASSERT_TRUE(result);
+
+  // Activate the shortcut (Ctrl+F) to make the page red (should not work).
   ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
       browser(), ui::VKEY_F, true, false, false, false));
 
@@ -220,6 +241,50 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, MAYBE_DontOverwriteSystemShortcuts) {
   ASSERT_TRUE(result);
 }
 
+// This test validates that an extension can override the Chrome bookmark
+// shortcut if it has requested to do so.
+IN_PROC_BROWSER_TEST_F(CommandsApiTest, OverwriteBookmarkShortcut) {
+  ASSERT_TRUE(test_server()->Start());
+
+  ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+
+  // This functionality requires a feature flag.
+  CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      "--enable-override-bookmarks-ui",
+      "1");
+
+  ASSERT_TRUE(RunExtensionTest("keybinding/overwrite_bookmark_shortcut"))
+      << message_;
+
+  ui_test_utils::NavigateToURL(browser(),
+      test_server()->GetURL("files/extensions/test_file.txt"));
+
+  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(tab);
+
+  // Activate the shortcut (Ctrl+D) to make the page green.
+  {
+    ResultCatcher catcher;
+#if defined(OS_MACOSX)
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_D, false, false, false, true));
+#else
+    ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_D, true, false, false, false));
+#endif
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+
+  bool result = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      tab,
+      "setInterval(function() {"
+      "  if (document.body.bgColor == 'green') {"
+      "    window.domAutomationController.send(true)}}, 100)",
+      &result));
+  ASSERT_TRUE(result);
+}
+
 #if defined(OS_WIN)
 // Currently this feature is implemented on Windows only.
 #define MAYBE_AllowDuplicatedMediaKeys AllowDuplicatedMediaKeys

chrome/browser/ui/accelerator_utils.h

@@ -18,6 +18,8 @@ namespace chrome {
 bool IsChromeAccelerator(const ui::Accelerator& accelerator,
                          Profile* profile);
 
+ui::Accelerator GetPrimaryChromeAcceleratorForCommandId(int command_id);
+
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_UI_ACCELERATOR_UTILS_H_

chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm

@@ -50,4 +50,11 @@ bool IsChromeAccelerator(const ui::Accelerator& accelerator, Profile* profile) {
   return id != -1;
 }
 
+ui::Accelerator GetPrimaryChromeAcceleratorForCommandId(int command_id) {
+  const ui::Accelerator* accelerator =
+      AcceleratorsCocoa::GetInstance()->GetAcceleratorForCommand(command_id);
+
+  return accelerator ? *accelerator : ui::Accelerator();
+}
+
 }  // namespace chrome

chrome/browser/ui/gtk/accelerator_utils_gtk.cc

@@ -20,4 +20,12 @@ bool IsChromeAccelerator(const ui::Accelerator& accelerator, Profile* profile) {
   return false;
 }
 
+ui::Accelerator GetPrimaryChromeAcceleratorForCommandId(int command_id) {
+  AcceleratorsGtk* accelerators = AcceleratorsGtk::GetInstance();
+  const ui::Accelerator* accelerator =
+      accelerators->GetPrimaryAcceleratorForCommand(command_id);
+
+  return accelerator ? *accelerator : ui::Accelerator();
+}
+
 }  // namespace chrome

chrome/browser/ui/omnibox/location_bar.cc

@@ -30,9 +30,11 @@ bool LocationBar::IsBookmarkStarHiddenByExtension() const {
       extension_service->extensions();
   for (extensions::ExtensionSet::const_iterator i = extension_set->begin();
        i != extension_set->end(); ++i) {
-    const extensions::SettingsOverrides* settings_overrides =
-        extensions::SettingsOverrides::Get(i->get());
-    if (settings_overrides && settings_overrides->RemovesBookmarkButton() &&
+    using extensions::SettingsOverrides;
+    const SettingsOverrides* settings_overrides =
+        SettingsOverrides::Get(i->get());
+    if (settings_overrides &&
+        SettingsOverrides::RemovesBookmarkButton(*settings_overrides) &&
         (extensions::PermissionsData::HasAPIPermission(
             *i,
             extensions::APIPermission::kBookmarkManagerPrivate) ||

chrome/browser/ui/views/accelerator_utils_aura.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/views/accelerator_table.h"
 #include "ui/base/accelerators/accelerator.h"
 
@@ -35,4 +36,28 @@ bool IsChromeAccelerator(const ui::Accelerator& accelerator, Profile* profile) {
   return false;
 }
 
+ui::Accelerator GetPrimaryChromeAcceleratorForCommandId(int command_id) {
+  ui::Accelerator accelerator;
+  // GetAshAcceleratorForCommandId with HOST_DESKTOP_TYPE_ASH is used so we can
+  // find Ash accelerators if Ash is supported on this platform, even if it's
+  // not currently in use.
+  if (GetStandardAcceleratorForCommandId(command_id, &accelerator) ||
+      GetAshAcceleratorForCommandId(command_id,
+                                    HOST_DESKTOP_TYPE_ASH,
+                                    &accelerator)) {
+    return accelerator;
+  }
+
+  std::vector<chrome::AcceleratorMapping> accelerators =
+      chrome::GetAcceleratorList();
+  for (size_t i = 0; i < accelerators.size(); ++i) {
+    if (accelerators[i].command_id == command_id) {
+      return ui::Accelerator(accelerators[i].keycode,
+                             accelerators[i].modifiers);
+    }
+  }
+
+  return ui::Accelerator();
+}
+
 }  // namespace chrome

chrome/common/extensions/api/manifest_types.json

@@ -53,6 +53,11 @@
                 "description": "If <code>true</code>, the built-in bookmark button will be removed from the user interface.",
                 "optional": true
               },
+              "remove_bookmark_shortcut": {
+                "type": "boolean",
+                "description": "If <code>true</code>, the built-in \"Bookmark this page...\" shortcut key is removed and the extension is permitted to override the shortcut by binding it in the commands section of the manifest.",
+                "optional": true
+              },
               // TODO(wittman): Remove for M36.
               "hide_bookmark_button": {
                 "type": "boolean",

chrome/common/extensions/docs/templates/articles/settings_override.html

+<h1>Settings Overrides</h1>
+
+<p>
+Settings overrides are a way for extensions to override selected Chrome settings
+and user interface properties.
+</p>
+
+<p>
+An extension can override one or more of the following properties:
+</p>
+
+<p>
+<b>Bookmarks User Interface:</b>
+<ul>
+  <li>
+    Bookmark button: the "star" button that is used to bookmark
+    pages. Extensions may remove this button using the settings overrides, and
+    optionally replace it with a browser action or page action.
+  </li>
+
+  <li>
+    Bookmark shortcut: the shortcut key that is used to bookmark a page (Ctrl-D
+    on Windows). Extensions may remove this shortcut via the settings overrides,
+    and optionally bind their own command to it using the <code>commands</code>
+    section of the manifest.
+  </li>
+</ul>
+</p>
+
+<p class="note">
+<b>Note:</b> Settings overrides are only enabled in the Chrome Dev release, and
+Chrome must be started with the <code>--enable-override-bookmarks-ui=1</code>
+command line flag to enable bookmarks user interface overrides.</p>
+
+<h2 id="manifest">Manifest</h2>
+
+<p>
+Register settings overrides in the
+<a href="manifest.html">extension manifest</a> like this:
+</p>
+
+<pre>{
+  "name": "My extension",
+  ...
+<b>
+  "chrome_settings_overrides" : {
+    "bookmarks_ui": {
+      "remove_button": "true",
+      "remove_bookmark_shortcut": "true"
+    }
+  }</b>,
+  ...
+}</pre>

chrome/common/extensions/docs/templates/json/manifest.json

@@ -21,6 +21,10 @@
     "example": {},
     "level": "only_one"
   },
+  "chrome_settings_overrides": {
+    "documentation": "settings_override.html",
+    "example": {}
+  },
   "chrome_url_overrides": {
     "documentation": "override.html",
     "example": {}

chrome/common/extensions/docs/templates/public/extensions/settings_override.html

+{{+partials.standard_extensions_article article:intros.settings_override/}}

chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc

@@ -220,9 +220,18 @@ const SettingsOverrides* SettingsOverrides::Get(
       extension->GetManifestData(manifest_keys::kSettingsOverride));
 }
 
-bool SettingsOverrides::RemovesBookmarkButton() const {
-  return bookmarks_ui && bookmarks_ui->remove_button &&
-      *bookmarks_ui->remove_button;
+bool SettingsOverrides::RemovesBookmarkButton(
+    const SettingsOverrides& settings_overrides) {
+  return settings_overrides.bookmarks_ui &&
+      settings_overrides.bookmarks_ui->remove_button &&
+      *settings_overrides.bookmarks_ui->remove_button;
+}
+
+bool SettingsOverrides::RemovesBookmarkShortcut(
+    const SettingsOverrides& settings_overrides) {
+  return settings_overrides.bookmarks_ui &&
+      settings_overrides.bookmarks_ui->remove_bookmark_shortcut &&
+      *settings_overrides.bookmarks_ui->remove_bookmark_shortcut;
 }
 
 SettingsOverridesHandler::SettingsOverridesHandler() {}
@@ -257,7 +266,7 @@ bool SettingsOverridesHandler::Parse(Extension* extension,
     return false;
   }
   info->manifest_permission.reset(new ManifestPermissionImpl(
-      info->RemovesBookmarkButton()));
+      SettingsOverrides::RemovesBookmarkButton(*info)));
 
   APIPermissionSet* permission_set =
       PermissionsData::GetInitialAPIPermissions(extension);

chrome/common/extensions/manifest_handlers/settings_overrides_handler.h

@@ -22,7 +22,10 @@ struct SettingsOverrides : public Extension::ManifestData {
 
   static const SettingsOverrides* Get(const Extension* extension);
 
-  bool RemovesBookmarkButton() const;
+  static bool RemovesBookmarkButton(
+      const SettingsOverrides& settings_overrides);
+  static bool RemovesBookmarkShortcut(
+      const SettingsOverrides& settings_overrides);
 
   scoped_ptr<api::manifest_types::ChromeSettingsOverrides::Bookmarks_ui>
       bookmarks_ui;

chrome/test/data/extensions/api_test/keybinding/dont_overwrite_system/manifest.json

@@ -11,6 +11,10 @@
       "suggested_key": "Ctrl+F",
       "description": "Make the page red"
     },
+    "green": {
+      "suggested_key": "Ctrl+D",
+      "description": "Make the page green"
+    },
     "blue": {
       "suggested_key": "Alt+Shift+F",
       "description": "Make the page blue"

chrome/test/data/extensions/api_test/keybinding/overwrite_bookmark_shortcut/background.js

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Called when the user activates the command.
+chrome.commands.onCommand.addListener(function(command) {
+  chrome.tabs.executeScript(null, {
+      code: "document.body.bgColor='" + command + "'" });
+  chrome.test.notifyPass();
+});
+
+chrome.test.notifyPass();

chrome/test/data/extensions/api_test/keybinding/overwrite_bookmark_shortcut/manifest.json

+{
+  "name": "An extension that overwrites permitted system keybindings",
+  "version": "1.0",
+  "manifest_version": 2,
+  "background": {
+    "scripts": ["background.js"]
+  },
+  "permissions": ["activeTab"],
+  "chrome_settings_overrides": {
+    "bookmarks_ui": {
+      "remove_bookmark_shortcut": true
+    }
+  },
+  "commands": {
+    "green": {
+      "suggested_key": "Ctrl+D",
+      "description": "Make the page green"
+    }
+  }
+}

ui/base/accelerators/accelerator.cc

@@ -68,14 +68,15 @@ bool Accelerator::operator <(const Accelerator& rhs) const {
 }
 
 bool Accelerator::operator ==(const Accelerator& rhs) const {
-  if (platform_accelerator_.get() != rhs.platform_accelerator_.get() &&
-      ((!platform_accelerator_.get() || !rhs.platform_accelerator_.get()) ||
-       !platform_accelerator_->Equals(*rhs.platform_accelerator_))) {
-    return false;
-  }
+  if ((key_code_ == rhs.key_code_) && (type_ == rhs.type_) &&
+      (modifiers_ == rhs.modifiers_))
+    return true;
+
+  bool platform_equal =
+      platform_accelerator_.get() && rhs.platform_accelerator_.get() &&
+      platform_accelerator_.get() == rhs.platform_accelerator_.get();
 
-  return (key_code_ == rhs.key_code_) && (type_ == rhs.type_) &&
-      (modifiers_ == rhs.modifiers_);
+  return platform_equal;
 }
 
 bool Accelerator::operator !=(const Accelerator& rhs) const {