Media Keys should not count towards the max of four shortcuts per extension.

R=finnur@chromium.org
TEST=unit_tests --gtest_filter=CommandsManifestTest.CommandManifestShouldNotCountMediaKeys
BUG=329870

Review URL: https://codereview.chromium.org/180783012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@254176 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 71 insertions, 28 deletions

diff --git a/chrome/browser/extensions/api/commands/command_service.cc b/chrome/browser/extensions/api/commands/command_service.cc
index 7beb4e5..a4344c7 100644
--- a/chrome/browser/extensions/api/commands/command_service.cc
+++ b/chrome/browser/extensions/api/commands/command_service.cc
@@ -54,7 +54,7 @@ std::string GetPlatformKeybindingKeyForAccelerator(
   // shortcut (1-to-1 relationship). That means two or more extensions can
   // register for the same media key so the extension ID needs to be added to
   // the key to make sure the key is unique.
-  if (extensions::CommandService::IsMediaKey(accelerator))
+  if (extensions::Command::IsMediaKey(accelerator))
     key += ":" + extension_id;
 
   return key;
@@ -90,7 +90,7 @@ bool CanAutoAssign(const ui::Accelerator& accelerator,
                    bool is_named_command,
                    bool is_global) {
   // Media Keys are non-exclusive, so allow auto-assigning them.
-  if (extensions::CommandService::IsMediaKey(accelerator))
+  if (extensions::Command::IsMediaKey(accelerator))
     return true;
 
   if (is_global) {
@@ -169,17 +169,6 @@ CommandService* CommandService::Get(content::BrowserContext* context) {
 }
 
 // static
-bool CommandService::IsMediaKey(const ui::Accelerator& accelerator) {
-  if (accelerator.modifiers() != 0)
-    return false;
-
-  return (accelerator.key_code() == ui::VKEY_MEDIA_NEXT_TRACK ||
-          accelerator.key_code() == ui::VKEY_MEDIA_PREV_TRACK ||
-          accelerator.key_code() == ui::VKEY_MEDIA_PLAY_PAUSE ||
-          accelerator.key_code() == ui::VKEY_MEDIA_STOP);
-}
-
-// static
 bool CommandService::RemovesBookmarkShortcut(
     const extensions::Extension* extension) {
   using extensions::SettingsOverrides;
@@ -261,7 +250,7 @@ bool CommandService::AddKeybindingPref(
     return false;
 
   // Media Keys are allowed to be used by named command only.
-  DCHECK(!IsMediaKey(accelerator) ||
+  DCHECK(!Command::IsMediaKey(accelerator) ||
          (command_name != manifest_values::kPageActionCommandEvent &&
           command_name != manifest_values::kBrowserActionCommandEvent));
 
diff --git a/chrome/browser/extensions/api/commands/command_service.h b/chrome/browser/extensions/api/commands/command_service.h
index bd27746..41c288f 100644
--- a/chrome/browser/extensions/api/commands/command_service.h
+++ b/chrome/browser/extensions/api/commands/command_service.h
@@ -78,11 +78,6 @@ class CommandService : public ProfileKeyedAPI,
   // Convenience method to get the CommandService for a profile.
   static CommandService* Get(content::BrowserContext* context);
 
-  // Return true if the specified accelerator is one of the following multimedia
-  // keys: Next Track key, Previous Track key, Stop Media key, Play/Pause Media
-  // key, without any modifiers.
-  static bool IsMediaKey(const ui::Accelerator& accelerator);
-
   // Returns true if |extension| is permitted to and does remove the bookmark
   // shortcut key.
   static bool RemovesBookmarkShortcut(const extensions::Extension* extension);
diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc b/chrome/browser/extensions/extension_keybinding_registry.cc
index 24b80a3..5a62d00 100644
--- a/chrome/browser/extensions/extension_keybinding_registry.cc
+++ b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -7,9 +7,9 @@
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
-#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/command.h"
 #include "content/public/browser/browser_context.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_system.h"
@@ -144,7 +144,7 @@ void ExtensionKeybindingRegistry::AddEventTarget(
       std::make_pair(extension_id, command_name));
   // Shortcuts except media keys have only one target in the list. See comment
   // about |event_targets_|.
-  if (!extensions::CommandService::IsMediaKey(accelerator))
+  if (!extensions::Command::IsMediaKey(accelerator))
     DCHECK_EQ(1u, event_targets_[accelerator].size());
 }
 
diff --git a/chrome/browser/extensions/global_shortcut_listener_mac.mm b/chrome/browser/extensions/global_shortcut_listener_mac.mm
index 43a36d4..7df8fba 100644
--- a/chrome/browser/extensions/global_shortcut_listener_mac.mm
+++ b/chrome/browser/extensions/global_shortcut_listener_mac.mm
@@ -9,7 +9,7 @@
 #include <IOKit/hidsystem/ev_keymap.h>
 
 #import "base/mac/foundation_util.h"
-#include "chrome/browser/extensions/api/commands/command_service.h"
+#include "chrome/common/extensions/command.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/events/event.h"
@@ -125,7 +125,7 @@ bool GlobalShortcutListenerMac::RegisterAcceleratorImpl(
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(accelerator_ids_.find(accelerator) == accelerator_ids_.end());
 
-  if (CommandService::IsMediaKey(accelerator)) {
+  if (Command::IsMediaKey(accelerator)) {
     if (!IsAnyMediaKeyRegistered()) {
       // If this is the first media key registered, start the event tap.
       StartWatchingMediaKeys();
@@ -153,7 +153,7 @@ void GlobalShortcutListenerMac::UnregisterAcceleratorImpl(
   DCHECK(accelerator_ids_.find(accelerator) != accelerator_ids_.end());
 
   // Unregister the hot_key if it's a keyboard shortcut.
-  if (!CommandService::IsMediaKey(accelerator))
+  if (!Command::IsMediaKey(accelerator))
     UnregisterHotKey(accelerator);
 
   // Remove hot_key from the mappings.
@@ -161,7 +161,7 @@ void GlobalShortcutListenerMac::UnregisterAcceleratorImpl(
   id_accelerators_.erase(key_id);
   accelerator_ids_.erase(accelerator);
 
-  if (CommandService::IsMediaKey(accelerator)) {
+  if (Command::IsMediaKey(accelerator)) {
     // If we unregistered a media key, and now no media keys are registered,
     // stop the media key tap.
     if (!IsAnyMediaKeyRegistered())
@@ -283,7 +283,7 @@ bool GlobalShortcutListenerMac::IsAnyMediaKeyRegistered() {
   // Iterate through registered accelerators, looking for media keys.
   AcceleratorIdMap::iterator it;
   for (it = accelerator_ids_.begin(); it != accelerator_ids_.end(); ++it) {
-    if (CommandService::IsMediaKey(it->first))
+    if (Command::IsMediaKey(it->first))
       return true;
   }
   return false;
@@ -292,7 +292,7 @@ bool GlobalShortcutListenerMac::IsAnyMediaKeyRegistered() {
 bool GlobalShortcutListenerMac::IsAnyHotKeyRegistered() {
   AcceleratorIdMap::iterator it;
   for (it = accelerator_ids_.begin(); it != accelerator_ids_.end(); ++it) {
-    if (!CommandService::IsMediaKey(it->first))
+    if (!Command::IsMediaKey(it->first))
       return true;
   }
   return false;
diff --git a/chrome/common/extensions/api/commands/commands_handler.cc b/chrome/common/extensions/api/commands/commands_handler.cc
index d49a105..8d0788b 100644
--- a/chrome/common/extensions/api/commands/commands_handler.cc
+++ b/chrome/common/extensions/api/commands/commands_handler.cc
@@ -7,6 +7,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/common/extensions/command.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/manifest_constants.h"
 
@@ -90,7 +91,13 @@ bool CommandsHandler::Parse(Extension* extension, base::string16* error) {
       return false;  // |error| already set.
 
     if (binding->accelerator().key_code() != ui::VKEY_UNKNOWN) {
-      if (++keybindings_found > kMaxCommandsWithKeybindingPerExtension) {
+      // Only media keys are allowed to work without modifiers, and because
+      // media keys aren't registered exclusively they should not count towards
+      // the max of four shortcuts per extension.
+      if (!Command::IsMediaKey(binding->accelerator()))
+        ++keybindings_found;
+
+      if (keybindings_found > kMaxCommandsWithKeybindingPerExtension) {
         *error = ErrorUtils::FormatErrorMessageUTF16(
             manifest_errors::kInvalidKeyBindingTooMany,
             base::IntToString(kMaxCommandsWithKeybindingPerExtension));
diff --git a/chrome/common/extensions/api/commands/commands_manifest_unittest.cc b/chrome/common/extensions/api/commands/commands_manifest_unittest.cc
index a0eab03..b753003 100644
--- a/chrome/common/extensions/api/commands/commands_manifest_unittest.cc
+++ b/chrome/common/extensions/api/commands/commands_manifest_unittest.cc
@@ -132,4 +132,9 @@ TEST_F(CommandsManifestTest, ChannelTests) {
   }
 }
 
+TEST_F(CommandsManifestTest, CommandManifestShouldNotCountMediaKeys) {
+  scoped_refptr<Extension> extension =
+      LoadAndExpectSuccess("command_should_not_count_media_keys.json");
+}
+
 }  // namespace extensions
diff --git a/chrome/common/extensions/command.cc b/chrome/common/extensions/command.cc
index faed130..5ebf3fc 100644
--- a/chrome/common/extensions/command.cc
+++ b/chrome/common/extensions/command.cc
@@ -380,6 +380,17 @@ std::string Command::AcceleratorToString(const ui::Accelerator& accelerator) {
   return shortcut;
 }
 
+// static
+bool Command::IsMediaKey(const ui::Accelerator& accelerator) {
+  if (accelerator.modifiers() != 0)
+    return false;
+
+  return (accelerator.key_code() == ui::VKEY_MEDIA_NEXT_TRACK ||
+          accelerator.key_code() == ui::VKEY_MEDIA_PREV_TRACK ||
+          accelerator.key_code() == ui::VKEY_MEDIA_PLAY_PAUSE ||
+          accelerator.key_code() == ui::VKEY_MEDIA_STOP);
+}
+
 bool Command::Parse(const base::DictionaryValue* command,
                     const std::string& command_name,
                     int index,
diff --git a/chrome/common/extensions/command.h b/chrome/common/extensions/command.h
index 0ea3fe4..41ec35a 100644
--- a/chrome/common/extensions/command.h
+++ b/chrome/common/extensions/command.h
@@ -42,6 +42,11 @@ class Command {
   // shortcut text (like accelerator::GetShortcutText() does).
   static std::string AcceleratorToString(const ui::Accelerator& accelerator);
 
+  // Return true if the specified accelerator is one of the following multimedia
+  // keys: Next Track key, Previous Track key, Stop Media key, Play/Pause Media
+  // key, without any modifiers.
+  static bool IsMediaKey(const ui::Accelerator& accelerator);
+
   // Parse the command.
   bool Parse(const base::DictionaryValue* command,
              const std::string& command_name,
diff --git a/chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json b/chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json
new file mode 100644
index 0000000..01cbfaa
--- /dev/null
+++ b/chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json
@@ -0,0 +1,31 @@
+{
+  "name": "Command test - mediaKeys should not counts towards the max of four shortcuts per extension",
+  "manifest_version": 2,
+  "version": "1",
+  "commands": {
+    "MediaNextTrack": {
+      "suggested_key": "MediaNextTrack",
+      "description": "MediaNextTrack"
+    },
+    "MediaPlayPause": {
+      "suggested_key": "MediaPlayPause",
+      "description": "MediaPlayPause"
+    },
+    "MediaPrevTrack": {
+      "suggested_key": "MediaPrevTrack",
+      "description": "MediaPrevTrack"
+    },
+    "MediaStop": {
+      "suggested_key": "MediaStop",
+      "description": "MediaStop"
+    },
+    "feature1": {
+      "suggested_key": "Ctrl+A",
+      "description": "feature1"
+    },
+    "feature2": {
+      "suggested_key": "Ctrl+B",
+      "description": "feature2"
+    }
+  }
+}