[Windows] Finish global and non-global media keys support on Windows.

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

Index: chrome/browser/extensions/api/commands/command_service.cc
diff --git a/chrome/browser/extensions/api/commands/command_service.cc b/chrome/browser/extensions/api/commands/command_service.cc
index 9bff7a572ad6b3dfc860973b97a532ed4069ecb4..eafbb86ceea11f3aff8f7c98b50a001226be841f 100644
--- a/chrome/browser/extensions/api/commands/command_service.cc
+++ b/chrome/browser/extensions/api/commands/command_service.cc
@@ -4,8 +4,11 @@
 
 #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/browser/chrome_notification_types.h"
@@ -23,6 +26,7 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "extensions/common/feature_switch.h"
+#include "extensions/common/manifest_constants.h"
 
 using extensions::Extension;
 using extensions::ExtensionPrefs;
@@ -110,6 +114,17 @@ CommandService* CommandService::Get(Profile* profile) {
   return ProfileKeyedAPIFactory<CommandService>::GetForProfile(profile);
 }
 
+// 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);
+}
+
 bool CommandService::GetBrowserActionCommand(
     const std::string& extension_id,
     QueryType type,
@@ -194,6 +209,19 @@ bool CommandService::AddKeybindingPref(
 
   std::string key = GetPlatformKeybindingKeyForAccelerator(accelerator);
 
+  // Media keys have a 1-to-many relationship with targets, unlike regular
+  // 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 (IsMediaKey(accelerator)) {
+    // Media Keys are allowed using in named command only.

[Finnur, 2013/11/18 11:16:14]

nit: s/using in/for use by/

[zhchbin, 2013/11/18 13:10:13]

Done.

+    DCHECK(command_name != manifest_values::kPageActionCommandEvent &&
+           command_name != manifest_values::kBrowserActionCommandEvent &&
+           command_name != manifest_values::kScriptBadgeCommandEvent);
+
+    key += ":" + extension_id;
+  }

[Finnur, 2013/11/18 11:16:14]

This logic should be moved to GetPlatformKeybindingKeyForAccelerator.

[zhchbin, 2013/11/18 13:10:13]

Done.

+
   if (!allow_overrides && bindings->HasKey(key))
     return false;  // Already taken.
 
@@ -287,8 +315,12 @@ Command CommandService::FindCommandByName(
       item->GetBoolean(kGlobal, &global);
 
     std::string shortcut = it.key();
-    if (StartsWithASCII(shortcut, Command::CommandPlatform() + ":", true))
-      shortcut = shortcut.substr(Command::CommandPlatform().length() + 1);
+    if (StartsWithASCII(shortcut, Command::CommandPlatform() + ":", true)) {
+      std::vector<std::string> tokens;
+      base::SplitString(shortcut, ':', &tokens);
+      CHECK(tokens.size() >= 2);
+      shortcut = tokens[1];  // The second token is the shortcut assigned.

[Finnur, 2013/11/18 11:16:14]

nit: Instead of this comment, I would have a comment on line 318 (or 317):

// Format stored in Preferences is: "Platform:Shortcut[:ExtensionId]".

[zhchbin, 2013/11/18 13:10:13]

Done.

+    }
 
     return Command(command_name, string16(), shortcut, global);
   }
@@ -311,9 +343,9 @@ void CommandService::AssignInitialKeybindings(const Extension* extension) {
 
   extensions::CommandMap::const_iterator iter = commands->begin();
   for (; iter != commands->end(); ++iter) {
-    if (!chrome::IsChromeAccelerator(
-            iter->second.accelerator(), profile_) &&
-        IsWhitelistedGlobalShortcut(iter->second)) {
+    if ((!chrome::IsChromeAccelerator(iter->second.accelerator(), profile_) &&
+        IsWhitelistedGlobalShortcut(iter->second)) ||
+        extensions::CommandService::IsMediaKey(iter->second.accelerator())) {

[Finnur, 2013/11/18 11:16:14]

Add a comment on line 346:

Make sure registered Chrome shortcuts cannot be automatically assigned
(overwritten) by extension developers. Media keys are an exception here.

[zhchbin, 2013/11/18 13:10:13]

Done.

       AddKeybindingPref(iter->second.accelerator(),
                         extension->id(),
                         iter->second.command_name(),