[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..4d091979d9ba39730726e7207f7430c811098524 100644
--- a/chrome/browser/extensions/api/commands/command_service.cc
+++ b/chrome/browser/extensions/api/commands/command_service.cc
@@ -4,8 +4,12 @@
 
 #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_number_conversions.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"
@@ -84,7 +88,8 @@ void CommandService::RegisterProfilePrefs(
 }
 
 CommandService::CommandService(Profile* profile)
-    : profile_(profile) {
+    : media_keys_command_counter_(0),
+      profile_(profile) {
   ExtensionFunctionRegistry::GetInstance()->
       RegisterFunction<GetAllCommandsFunction>();
 
@@ -110,6 +115,17 @@ CommandService* CommandService::Get(Profile* profile) {
   return ProfileKeyedAPIFactory<CommandService>::GetForProfile(profile);
 }
 
+// staitc

[Finnur, 2013/11/12 16:27:54]

s/staitc/static/

[zhchbin, 2013/11/13 05:01:57]

Done.

+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,
@@ -193,6 +209,8 @@ bool CommandService::AddKeybindingPref(
   base::DictionaryValue* bindings = updater.Get();
 
   std::string key = GetPlatformKeybindingKeyForAccelerator(accelerator);
+  if (IsMediaKey(accelerator))

[Finnur, 2013/11/12 16:27:54]

What happens if I assign a Media key to a browser action? Shouldn't we exclude
page action and browser action commands from getting this prefix?

[zhchbin, 2013/11/13 05:01:57]

1. How can it happen? We only parse Media Keys for named command, (Maybe I miss
out on something), so we didn't need to do that. 
2. But from the perspective of this function, we should exclude for someone may
change the parse code.

I choose the first one. WDYH?

[Finnur, 2013/11/13 10:22:24]

That's fine. Perhaps just add a DCHECK inside the if clause that command_name is
not a page action/browser action (when IsMediaKey is true)?

[zhchbin, 2013/11/13 16:05:10]

Done.

+    key += (":" + base::IntToString(media_keys_command_counter_++));

[Finnur, 2013/11/12 16:27:54]

nit: Do you need the parens around the whole expression?

[zhchbin, 2013/11/13 05:01:57]

Done.

 
   if (!allow_overrides && bindings->HasKey(key))
     return false;  // Already taken.
@@ -287,8 +305,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 that assigned.

[Finnur, 2013/11/12 16:27:54]

nit: Drop the word 'that'

[zhchbin, 2013/11/13 05:01:57]

Done.

+    }
 
     return Command(command_name, string16(), shortcut, global);
   }
@@ -311,9 +333,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/12 16:27:54]

I think we always want the ChromeAccelerator check, in case MediaKey was one of
those. Remember, this is to avoid having Extension Developers steal Chrome
Accelerators. It is less of a concern for MediaKeys, but we can always relax the
restriction, if we need to.

How about:

if (!chrome::IsChromeAccelerator(iter->second.accelerator(), profile_) &&
    (IsWhitelistedGlobalShortcut(iter->second) ||
        extensions::CommandService::IsMediaKey(iter->second.accelerator()))) {

Does that work?

[zhchbin, 2013/11/13 05:01:57]

Not. !chrome::IsChromeAccelerator will be false for Media Keys on aura build.
See the implementation of this function: [0]

[0]:https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/accelerator_utils_aura.cc

[Finnur, 2013/11/13 10:22:24]

So, I would actually expect IsChromeAccelerator to return false for media keys
on all platforms. And that's good, because then the code works on all platforms.

This is more so that if we the Chrome UI guys decide in the future to make
MediaKeys into reserved ChromeAccelerators that extension devs can't steal them
from them anymore. You know, like it works today with Ctrl+F (it's taken by
FindInPage so extension devs can't set it, but the user can override it).

[zhchbin, 2013/11/13 16:05:10]

Do you mean remove media keys in the reserved ChromeAccelerators to make it
return false for now? Then in the future maybe we will keep them reserved again?

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

Hmm... Are they already reserved? OK, then that won't work. Since we're not
exclusive on the media key shortcuts we can probably make an exception on this
and keep it as is.

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

Yes, you can find the reserved shortcuts on aura here:
ash/accelerators/accelerator_table.cc

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