Parse media keys for named command in the manifest.

chrome/common/extensions/command.cc

Index: chrome/common/extensions/command.cc
diff --git a/chrome/common/extensions/command.cc b/chrome/common/extensions/command.cc
index fffc55ef29dcd8a33363a4d73bbf5c81206a9616..1bbe360f4d30fbadfa2cc16504c2fa5a96e65350 100644
--- a/chrome/common/extensions/command.cc
+++ b/chrome/common/extensions/command.cc
@@ -29,9 +29,16 @@ static const char kMissing[] = "Missing";
 static const char kCommandKeyNotSupported[] =
     "Command key is not supported. Note: Ctrl means Command on Mac";
 
+bool IsNamedCommand(const std::string& command_name) {
+  return command_name != values::kPageActionCommandEvent &&
+         command_name != values::kBrowserActionCommandEvent &&
+         command_name != values::kScriptBadgeCommandEvent;
+}
+
 ui::Accelerator ParseImpl(const std::string& accelerator,
                           const std::string& platform_key,
                           int index,
+                          bool should_parse_media_keys,
                           string16* error) {
   if (platform_key != values::kKeybindingPlatformWin &&
       platform_key != values::kKeybindingPlatformMac &&
@@ -45,6 +52,17 @@ ui::Accelerator ParseImpl(const std::string& accelerator,
     return ui::Accelerator();
   }
 
+  if (should_parse_media_keys) {
+    if (accelerator == values::kKeyMediaNextTrack)
+      return ui::Accelerator(ui::VKEY_MEDIA_NEXT_TRACK, ui::EF_NONE);
+    if (accelerator == values::kKeyMediaPlayPause)
+      return ui::Accelerator(ui::VKEY_MEDIA_PLAY_PAUSE, ui::EF_NONE);
+    if (accelerator == values::kKeyMediaPrevTrack)
+      return ui::Accelerator(ui::VKEY_MEDIA_PREV_TRACK, ui::EF_NONE);
+    if (accelerator == values::kKeyMediaStop)
+      return ui::Accelerator(ui::VKEY_MEDIA_STOP, ui::EF_NONE);

[Finnur, 2013/08/28 14:15:16]

This seems a bit ad-hoc. Also, I think we need to make sure modifiers don't work
(such as Ctrl+Shift+MediaKeys). I would delete this code block and implement it
below.

+  }
+
   std::vector<std::string> tokens;
   base::SplitString(accelerator, '+', &tokens);
   if (tokens.size() < 2 || tokens.size() > 3) {

[zhchbin, 2013/08/29 06:20:14]

Note: if the size of tokens is 1, it will fail and return. Our case:
"MediaNextTrack", there is no '+' in the accelerator and the size of tokens is
1.

[Finnur, 2013/08/29 11:20:52]

Yes, change this if statement to become:

if (tokens.size() == 0 ||
    (tokens.size() == 1 && DoesRequireModifier(accelerator)) ||
    tokens.size() > 3) {
  ... do what it does now (show error) ...
}

Then implement DoesRequireModifier to return true for all accelerators but the
Media Keys.

[zhchbin, 2013/08/29 14:27:11]

Done.

@@ -214,7 +232,8 @@ Command::Command(const std::string& command_name,
     : command_name_(command_name),
       description_(description) {
   string16 error;
-  accelerator_ = ParseImpl(accelerator, CommandPlatform(), 0, &error);
+  accelerator_ = ParseImpl(accelerator, CommandPlatform(), 0,
+                           IsNamedCommand(command_name), &error);
 }
 
 Command::~Command() {}
@@ -235,11 +254,12 @@ std::string Command::CommandPlatform() {
 }
 
 // static
-ui::Accelerator Command::StringToAccelerator(const std::string& accelerator) {
+ui::Accelerator Command::StringToAccelerator(const std::string& accelerator,
+                                             const std::string& command_name) {
   string16 error;
-  Command command;
   ui::Accelerator parsed =
-      ParseImpl(accelerator, Command::CommandPlatform(), 0, &error);
+      ParseImpl(accelerator, Command::CommandPlatform(), 0,
+                IsNamedCommand(command_name), &error);
   return parsed;
 }
 
@@ -312,6 +332,18 @@ std::string Command::AcceleratorToString(const ui::Accelerator& accelerator) {
       case ui::VKEY_TAB:
         shortcut += values::kKeyTab;
         break;
+      case ui::VKEY_MEDIA_NEXT_TRACK:
+        shortcut += values::kKeyMediaNextTrack;
+        break;
+      case ui::VKEY_MEDIA_PLAY_PAUSE:
+        shortcut += values::kKeyMediaPlayPause;
+        break;
+      case ui::VKEY_MEDIA_PREV_TRACK:
+        shortcut += values::kKeyMediaPrevTrack;
+        break;
+      case ui::VKEY_MEDIA_STOP:
+        shortcut += values::kKeyMediaStop;
+        break;
       default:
         return "";
     }
@@ -326,9 +358,7 @@ bool Command::Parse(const base::DictionaryValue* command,
   DCHECK(!command_name.empty());
 
   string16 description;
-  if (command_name != values::kPageActionCommandEvent &&
-      command_name != values::kBrowserActionCommandEvent &&
-      command_name != values::kScriptBadgeCommandEvent) {
+  if (IsNamedCommand(command_name)) {
     if (!command->GetString(keys::kDescription, &description) ||
         description.empty()) {
       *error = ErrorUtils::FormatErrorMessageUTF16(
@@ -419,7 +449,8 @@ bool Command::Parse(const base::DictionaryValue* command,
     if (!iter->second.empty()) {
       // Note that we pass iter->first to pretend we are on a platform we're not
       // on.
-      accelerator = ParseImpl(iter->second, iter->first, index, error);
+      accelerator = ParseImpl(iter->second, iter->first, index,
+                              IsNamedCommand(command_name), error);
       if (accelerator.key_code() == ui::VKEY_UNKNOWN) {
         *error = ErrorUtils::FormatErrorMessageUTF16(
             errors::kInvalidKeyBinding,