Begin whitelisting specific extensions for global key registration.

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_keybinding_registry.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/extension_constants.h"
 #include "chrome/common/extensions/manifest_handlers/ui_overrides_handler.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "extensions/browser/extension_function_registry.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/feature_switch.h"
@@ skipping 525 matching lines @@
   // changed from the previous value.
   if (IsCommandShortcutUserModified(extension, command.command_name()))
     return false;
 
   if (command.global()) {
     using namespace extensions;
     if (command.command_name() == manifest_values::kBrowserActionCommandEvent ||
         command.command_name() == manifest_values::kPageActionCommandEvent)
       return false;  // Browser and page actions are not global in nature.
 
+    if (extension->id() == extension_misc::kChromeVoxExtensionId) {
+      return true;
+    }

[Finnur, 2014/07/17 11:00:52]

This seems a bit hacky. We already have a permission system for extensions, with
the ability to restrict to certain platforms/extension IDs. That seems like a
better fit for this.

[David Tseng, 2014/07/18 22:54:50]

Are you referring to _permission_features(.json)?

If so, which permission did you have in mind?

[Finnur, 2014/08/11 14:01:07]

Yes, for example, you can check in code whether an extension has certain
permissions, such as fileSystem.write by doing:

extension->permissions_data()->HasAPIPermission(APIPermission::kFileSystemWrite);

For your purposes, you could add a section to _permission_features.json
called...

commands.accessibility
(or something)

... and whitelist your extension for that permission (and then check for that
permission here instead of the id). That way it is really easy to update the
list if we need to add other extensions to it. Furthermore, there might be more
places you need to add this check...

So, I can see three rule restrictions that, depending on the use case, one would
want relaxed:

1) Allow overwriting of already taken shortcuts.
2) Allow overwriting of *global* shortcuts other than Ctrl+Shift+[0-9].
Remember: Global shortcuts work when Chrome does not have focus.
3) Relax the restriction on the number of keys a developer can assign to one
extension (currently maxed out at 4, not counting MediaKeys).

So, your CL is relaxing only 2. Remind me again why it is important for
accessibility shortcuts to work when Chrome does _not_ have focus?

[David Tseng, 2014/08/19 22:47:49]

A few cases. Suppose focus (in Chrome OS) is on the views-based toolbar or if
there are no browser windows open at all. We still want an accessibility
extension to receive keyboard commands (e.g. to read the current time).

As for the points above, I think we will need all three relaxations.

[Finnur, 2014/08/20 12:41:14]

OK, then that's definitely something you'd want to add an API permission for,
because you'd otherwise have to spread if checks for the extension_id around the
codebase and we'd need to update each location when adding/removing extensions.
The HasAPIPermission makes us only have to update a single file to give/take
permission to/from an extension.

+
     // 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;
@@ skipping 281 matching lines @@
   return true;
 }
 
 template <>
 void
 BrowserContextKeyedAPIFactory<CommandService>::DeclareFactoryDependencies() {
   DependsOn(ExtensionCommandsGlobalRegistry::GetFactoryInstance());
 }
 
 }  // namespace extensions