Allow extensions to remove and override the bookmark shortcut key

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

Index: chrome/browser/extensions/api/commands/command_service.cc
diff --git a/chrome/browser/extensions/api/commands/command_service.cc b/chrome/browser/extensions/api/commands/command_service.cc
index 4c2906def1022f85f6542f1a33c54f2c0ef9dbdf..5e92f683e58a90dc61231756fdb541884e6387b8 100644
--- a/chrome/browser/extensions/api/commands/command_service.cc
+++ b/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,7 +81,33 @@ bool InitialBindingsHaveBeenAssigned(
   return assigned;
 }
 
+bool IsReservedChromeAccelerator(const ui::Accelerator& accelerator,
+                                 const Extension* extension,
+                                 Profile* profile) {
+  if (accelerator ==
+      chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE)) {
+    using extensions::SettingsOverrides;
+    using extensions::FeatureSwitch;
+    const SettingsOverrides* settings_overrides =
+        SettingsOverrides::Get(extension);
+    return !(settings_overrides &&
+             SettingsOverrides::RemovesBookmarkShortcut(*settings_overrides) &&
+             (extensions::PermissionsData::HasAPIPermission(
+                 extension,
+                 extensions::APIPermission::kBookmarkManagerPrivate) ||
+              FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled()));

[Finnur, 2014/02/12 12:51:14]

It is hard to wrap one's head around verifying the correctness of allowing
extensions to override shortcuts. The ! here doesn't help, but neither does the
logic being split up into checks in three different places (here,
IsWhitelistedGlobalShortcut and IsMediaKey way down this file).

More on this in my next comment.

[Mike Wittman, 2014/02/12 19:55:58]

Agreed.

+  }
+
+  return chrome::IsChromeAccelerator(accelerator, profile);
+}
+
 bool IsWhitelistedGlobalShortcut(const extensions::Command& command) {
+  // NOTE(wittman): If the restrictions on auto-assigning global shortcuts are
+  // ever relaxed in the future, then keys such as the bookmarking shortcut
+  // (IDC_BOOKMARK_PAGE) need to be evaluated on a case-by-case basis. Such keys
+  // can be auto-assigned by extensions (with the right permission), even though
+  // the shortcut is already in use by Chrome.
+
   // Non-global shortcuts are always allowed.
   if (!command.global())
     return true;
@@ -364,8 +393,9 @@ void CommandService::AssignInitialKeybindings(const Extension* extension) {
   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)) ||
+    if ((!IsReservedChromeAccelerator(
+            iter->second.accelerator(), extension, profile_) &&
+         IsWhitelistedGlobalShortcut(iter->second)) ||

[Finnur, 2014/02/12 12:51:14]

This would turn into one simple if statement:
  if (CanOverride(iter->second, extension, profile, true))
... and same applies to below, except false for last param (for both).

[Mike Wittman, 2014/02/12 19:55:58]

Done.

         extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
       AddKeybindingPref(iter->second.accelerator(),
                         extension->id(),
@@ -378,8 +408,8 @@ void CommandService::AssignInitialKeybindings(const Extension* extension) {
   const extensions::Command* browser_action_command =
       CommandsInfo::GetBrowserActionCommand(extension);
   if (browser_action_command) {
-    if (!chrome::IsChromeAccelerator(
-        browser_action_command->accelerator(), profile_)) {
+    if (!IsReservedChromeAccelerator(
+        browser_action_command->accelerator(), extension, profile_)) {
       AddKeybindingPref(browser_action_command->accelerator(),
                         extension->id(),
                         browser_action_command->command_name(),
@@ -391,8 +421,8 @@ void CommandService::AssignInitialKeybindings(const Extension* extension) {
   const extensions::Command* page_action_command =
       CommandsInfo::GetPageActionCommand(extension);
   if (page_action_command) {
-    if (!chrome::IsChromeAccelerator(
-        page_action_command->accelerator(), profile_)) {
+    if (!IsReservedChromeAccelerator(
+        page_action_command->accelerator(), extension, profile_)) {
       AddKeybindingPref(page_action_command->accelerator(),
                         extension->id(),
                         page_action_command->command_name(),