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;

[Finnur, 2014/02/11 09:35:01]

Ooops! Not LGTM.

This is incorrect for Mac at the moment. As issue 342484 points out,
we need to fix IsChromeAccelerator on Mac before we can check this in.

[Mike Wittman, 2014/02/11 23:09:45]

OK. I will ping this review when the Mac changes are in.

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

Thank you.

+
+  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())) {
+    return accelerator !=
+        chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE);
+  }
+
+  return true;
+}

[Finnur, 2014/02/11 09:50:05]

We need to flip this logic. Instead of current implementation of (pseudo):

if (!reserved(accelerator))
  return false;
if (hasRemoveBookmarkPermission())
  return accelerator != BOOKMARK
return true;

... we need something like ...

if (accelerator == BOOKMARK)
  return !hasRemoveBookmarkPermission();
return reserved(accelerator);

This will ensure that if reserved() is incorrectly implemented (like it is now)
then still only extensions with permission to override the key can do so.

[Mike Wittman, 2014/02/11 23:09:45]

Done.

+
 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;
@@ skipping 265 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.
                         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