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

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_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"
 #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/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 58 matching lines @@
 
 // static
 void CommandService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(
       prefs::kExtensionCommands,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 }
 
 CommandService::CommandService(Profile* profile)
-    : profile_(profile) {
+    : media_keys_command_counter_(0),
+      profile_(profile) {
   ExtensionFunctionRegistry::GetInstance()->
       RegisterFunction<GetAllCommandsFunction>();
 
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
       content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
       content::Source<Profile>(profile));
 }
 
 CommandService::~CommandService() {
 }
 
 static base::LazyInstance<ProfileKeyedAPIFactory<CommandService> >
 g_factory = LAZY_INSTANCE_INITIALIZER;
 
 // static
 ProfileKeyedAPIFactory<CommandService>* CommandService::GetFactoryInstance() {
   return &g_factory.Get();
 }
 
 // static
 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,
     extensions::Command* command,
     bool* active) {
   return GetExtensionActionCommand(
       extension_id, type, command, active, BROWSER_ACTION);
 }
 
 bool CommandService::GetPageActionCommand(
@@ skipping 63 matching lines @@
     bool allow_overrides,
     bool global) {
   if (accelerator.key_code() == ui::VKEY_UNKNOWN)
     return false;
 
   DictionaryPrefUpdate updater(profile_->GetPrefs(),
                                prefs::kExtensionCommands);
   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.
 
   base::DictionaryValue* keybinding = new base::DictionaryValue();
   keybinding->SetString(kExtension, extension_id);
   keybinding->SetString(kCommandName, command_name);
   keybinding->SetBoolean(kGlobal, global);
 
   bindings->Set(key, keybinding);
@@ skipping 74 matching lines @@
       continue;
     std::string command_name;
     item->GetString(kCommandName, &command_name);
     if (command != command_name)
       continue;
     bool global = false;
     if (FeatureSwitch::global_commands()->IsEnabled())
       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);
   }
 
   return Command();
 }
 
 void CommandService::AssignInitialKeybindings(const Extension* extension) {
   const extensions::CommandMap* commands =
       CommandsInfo::GetNamedCommands(extension);
   if (!commands)
     return;
 
   ExtensionService* extension_service =
       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) {
-    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(),
                         false,  // Overwriting not allowed.
                         iter->second.global());
     }
   }
 
   const extensions::Command* browser_action_command =
       CommandsInfo::GetBrowserActionCommand(extension);
@@ skipping 130 matching lines @@
 
   return true;
 }
 
 template <>
 void ProfileKeyedAPIFactory<CommandService>::DeclareFactoryDependencies() {
   DependsOn(ExtensionCommandsGlobalRegistry::GetFactoryInstance());
 }
 
 }  // namespace extensions