Allow extensions to remove and override the bookmark shortcut key

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

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/extensions/api/commands/command_service.h"
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #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"
 #include "chrome/browser/extensions/extension_function_registry.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/extensions/extension_service.h"
 #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"
 #include "content/public/browser/notification_service.h"
 #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;
 
 namespace {
 
 const char kExtension[] = "extension";
 const char kCommandName[] = "command_name";
 const char kGlobal[] = "global";
 
@@ skipping 31 matching lines @@
     const ExtensionPrefs* prefs, const std::string& extension_id) {
   bool assigned = false;
   if (!prefs || !prefs->ReadPrefAsBoolean(extension_id,
                                           kInitialBindingsHaveBeenAssigned,
                                           &assigned))
     return false;
 
   return assigned;
 }
 
+bool IsReservedChromeAccelerator(const ui::Accelerator& accelerator,
+                                 const Extension* extension,
+                                 Profile* profile) {
+  if (!chrome::IsChromeAccelerator(accelerator, profile))
+    return false;
+
+  using extensions::SettingsOverrides;
+  using extensions::FeatureSwitch;
+  if (SettingsOverrides::Get(extension)->RemovesBookmarkShortcut() &&

[Finnur, 2014/02/05 13:37:40]

I mentioned this in the design doc, but worth repeating here: It is a bit
confusing to be able to assign a shortcut based on the fact that a
remove_shortcut permission is granted. This should really be
AllowBookmarkShortcutOverride(), or something.

[Mike Wittman, 2014/02/05 19:17:07]

I'll update this as necessary pending agreement in the design doc.

+      (extensions::PermissionsData::HasAPIPermission(
+          extension,
+          extensions::APIPermission::kBookmarkManagerPrivate) ||

[Finnur, 2014/02/05 13:37:40]

You mentioned you didn't want a dependency on the bookmarks permission, but you
have a dependency on the bookmarkManager permission instead. Should that be
listed in the design doc?

[Mike Wittman, 2014/02/05 19:17:07]

The check for kBookmarkManagerPrivate API permission is done as a means of
whitelisting Chrome-internal bookmarks extensions to use this functionality. It
has no effect on authors of general extensions, so I don't see a reason to
mention it in the design doc.

+       FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled())) {
+    return accelerator !=
+        chrome::GetChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE);
+  }
+
+  return true;
+}
+
 bool IsWhitelistedGlobalShortcut(const extensions::Command& command) {
   // Non-global shortcuts are always allowed.
   if (!command.global())
     return true;
   // Global shortcuts must be (Ctrl|Command)-Shift-[0-9].
 #if defined OS_MACOSX
   if (!command.accelerator().IsCmdDown())
     return false;
 #else
   if (!command.accelerator().IsCtrlDown())
@@ skipping 266 matching lines @@
       ExtensionSystem::Get(profile_)->extension_service();
   ExtensionPrefs* extension_prefs = extension_service->extension_prefs();
   if (InitialBindingsHaveBeenAssigned(extension_prefs, extension->id()))
     return;
   SetInitialBindingsHaveBeenAssigned(extension_prefs, extension->id());
 
   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)) ||
+    if ((!IsReservedChromeAccelerator(
+            iter->second.accelerator(), extension, profile_) &&
+         IsWhitelistedGlobalShortcut(iter->second)) ||
         extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
       AddKeybindingPref(iter->second.accelerator(),
                         extension->id(),
                         iter->second.command_name(),
                         false,  // Overwriting not allowed.

[Finnur, 2014/02/05 13:37:40]

Note: If the user has reassigned Ctrl+D to something else, then the bookmark
extension won't be allowed to assign it (since this is false). I'm leaning
towards that being correct, because the user should be in control of the
shortcuts, but could be convinced otherwise as in this case it is a bit of a
gray area since they just installed an extension that said it is going to take
over the shortcut and even has a special permission to do so. The counterpoint
is that the shortcut is no longer assigned to the bookmark feature...

[Mike Wittman, 2014/02/05 19:17:07]

I tend to agree that user-created shortcuts should not be automatically
overridden by extensions with this permission. Presumably if the user has taken
the effort to reassign the key once, they will be able to reassign again to this
extension if they want.

                         iter->second.global());

[Finnur, 2014/02/05 13:37:40]

Commands can be global/non-global in scope and a global command works even when
Chrome does not have focus (it is considered system-wide). 

Before your change, Ctrl+D was reserved and could not be overwritten by an
extension (only through user interaction). Now, however, the only thing stopping
the extension from making Ctrl+D available globally (which means Ctrl+D will
stop working in other apps since your extension eats it) is the fact that we are
super restrictive about the things you can auto-assign globally (see
IsWhitelistedGlobalShortcut). 

Initially I thought we should perhaps pass in bool global to
IsReservedChromeAccelerator and only consult the overwrite bookmarks flag if
global == false. But then I thought, this is probably (and should be) covered by
IsWhitelistedGlobalShortcut. If we decide to relax the restriction on
auto-assigning global shortcuts, then we should be able to auto-assign Ctrl+D,
even globally.

Perhaps the best course of action is simply to add a comment at the top of
IsWhitelistedGlobalShortcut, saying something like:

"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".

[Mike Wittman, 2014/02/05 19:17:07]

SGTM. I've added the comment.

     }
   }
 
   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(),
                         false,   // Overwriting not allowed.
                         false);  // Browser actions can't be global.
     }
   }
 
   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(),
                         false,   // Overwriting not allowed.
                         false);  // Page actions can't be global.
     }
   }
 }
 
 void CommandService::RemoveKeybindingPrefs(const std::string& extension_id,
@@ skipping 92 matching lines @@
 
   return true;
 }
 
 template <>
 void ProfileKeyedAPIFactory<CommandService>::DeclareFactoryDependencies() {
   DependsOn(ExtensionCommandsGlobalRegistry::GetFactoryInstance());
 }
 
 }  // namespace extensions