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 (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;
   // Global shortcuts must be (Ctrl|Command)-Shift-[0-9].
 #if defined OS_MACOSX
   if (!command.accelerator().IsCmdDown())
     return false;
 #else
   if (!command.accelerator().IsCtrlDown())
     return false;
 #endif
   if (!command.accelerator().IsShiftDown())
     return false;
   return (command.accelerator().key_code() >= ui::VKEY_0 &&
           command.accelerator().key_code() <= ui::VKEY_9);
 }

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

I just realized (coming in this morning with a clear head) that the thing that
is really bothering me with this is that we don't have a single funnel for
determining whether we can override a key. Instead we have three functions that
decide whether you can override the shortcut or not (pre-existing condition, as
they'd call it).

I'm therefore proposing we delete the comment you added to this function and
simply merge the two functions above into what I've written below (haven't
compiled it, but I think this should do what we want):

bool CanAutoAssign(const extensions::Command& command,
                   const Extension* extension,
                   Profile* profile,
                   bool is_named_command) {
  // Media Keys are non-exclusive, so allow auto-assigning them.
  if (extensions::CommandService::IsMediaKey(command.accelerator())
    return true;

  if (command.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())
      return false;
#else
    if (!command.accelerator().IsCtrlDown())
      return false;
#endif
    if (!command.accelerator().IsShiftDown())
      return false;
    return (command.accelerator().key_code() >= ui::VKEY_0 &&
            command.accelerator().key_code() <= ui::VKEY_9);
  } else {
    // Not a global command, check if Chrome shortcut and whether
    // we can override it.
    if (command.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(command.accelerator(), profile));
        return true;
      }
    }
  
    return !chrome::IsChromeAccelerator(command.accelerator(), profile);
  }
}

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

SGTM. This is clearer, and I believe implements the proper checking logic. I've
changed the interface to be a bit clearer and more explicit (IMO).

 
 }  // namespace
 
 namespace extensions {
 
 // static
 void CommandService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(
       prefs::kExtensionCommands,
@@ skipping 249 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)) ||

[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(),
                         iter->second.command_name(),
                         false,  // Overwriting not allowed.
                         iter->second.global());
     }
   }
 
   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