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..be46c20be8e43170b935be4b740d23cf593e4573 100644
--- a/chrome/common/extensions/command.cc
+++ b/chrome/common/extensions/command.cc
@@ -29,10 +29,25 @@ 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;
+}
+
+bool DoesRequireModifier(const std::string& accelerator) {
+  return accelerator != values::kKeyMediaNextTrack &&
+         accelerator != values::kKeyMediaPlayPause &&
+         accelerator != values::kKeyMediaPrevTrack &&
+         accelerator != values::kKeyMediaStop;
+}
+
 ui::Accelerator ParseImpl(const std::string& accelerator,
                           const std::string& platform_key,
                           int index,
+                          bool should_parse_media_keys,
                           string16* error) {
+  error->clear();
   if (platform_key != values::kKeybindingPlatformWin &&
       platform_key != values::kKeybindingPlatformMac &&
       platform_key != values::kKeybindingPlatformChromeOs &&
@@ -47,7 +62,9 @@ ui::Accelerator ParseImpl(const std::string& accelerator,
 
   std::vector<std::string> tokens;
   base::SplitString(accelerator, '+', &tokens);
-  if (tokens.size() < 2 || tokens.size() > 3) {
+  if (tokens.size() == 0 ||
+      (tokens.size() == 1 && DoesRequireModifier(accelerator)) ||
+      tokens.size() > 3) {
     *error = ErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
@@ -99,7 +116,11 @@ ui::Accelerator ParseImpl(const std::string& accelerator,
                tokens[i] == values::kKeyEnd ||
                tokens[i] == values::kKeyPgUp ||
                tokens[i] == values::kKeyPgDwn ||
-               tokens[i] == values::kKeyTab) {
+               tokens[i] == values::kKeyTab ||
+               tokens[i] == values::kKeyMediaNextTrack ||
+               tokens[i] == values::kKeyMediaPlayPause ||
+               tokens[i] == values::kKeyMediaPrevTrack ||
+               tokens[i] == values::kKeyMediaStop) {
       if (key != ui::VKEY_UNKNOWN) {
         // Multiple key assignments.
         key = ui::VKEY_UNKNOWN;
@@ -132,6 +153,18 @@ ui::Accelerator ParseImpl(const std::string& accelerator,
         key = ui::VKEY_NEXT;
       } else if (tokens[i] == values::kKeyTab) {
         key = ui::VKEY_TAB;
+      } else if (tokens[i] == values::kKeyMediaNextTrack &&
+                 should_parse_media_keys) {
+        key = ui::VKEY_MEDIA_NEXT_TRACK;
+      } else if (tokens[i] == values::kKeyMediaPlayPause &&
+                 should_parse_media_keys) {
+        key = ui::VKEY_MEDIA_PLAY_PAUSE;
+      } else if (tokens[i] == values::kKeyMediaPrevTrack &&
+                 should_parse_media_keys) {
+        key = ui::VKEY_MEDIA_PREV_TRACK;
+      } else if (tokens[i] == values::kKeyMediaStop &&
+                 should_parse_media_keys) {
+        key = ui::VKEY_MEDIA_STOP;
       } else if (tokens[i].size() == 1 &&
                  tokens[i][0] >= 'A' && tokens[i][0] <= 'Z') {
         key = static_cast<ui::KeyboardCode>(ui::VKEY_A + (tokens[i][0] - 'A'));
@@ -151,6 +184,7 @@ ui::Accelerator ParseImpl(const std::string& accelerator,
       return ui::Accelerator();
     }
   }
+
   bool command = (modifiers & ui::EF_COMMAND_DOWN) != 0;
   bool ctrl = (modifiers & ui::EF_CONTROL_DOWN) != 0;
   bool alt = (modifiers & ui::EF_ALT_DOWN) != 0;
@@ -172,6 +206,19 @@ ui::Accelerator ParseImpl(const std::string& accelerator,
     return ui::Accelerator();
   }
 
+  if ((key == ui::VKEY_MEDIA_NEXT_TRACK ||
+       key == ui::VKEY_MEDIA_PREV_TRACK ||
+       key == ui::VKEY_MEDIA_PLAY_PAUSE ||
+       key == ui::VKEY_MEDIA_STOP) &&
+      (shift || ctrl || alt || command)) {
+    *error = ErrorUtils::FormatErrorMessageUTF16(
+        errors::kInvalidKeyBindingMediaKeyWithModifier,
+        base::IntToString(index),
+        platform_key,
+        accelerator);
+    return ui::Accelerator();
+  }
+
   return ui::Accelerator(key, modifiers);
 }
 
@@ -214,7 +261,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 +283,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 +361,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 +387,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,13 +478,16 @@ 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,
-            base::IntToString(index),
-            iter->first,
-            iter->second);
+        if (error->empty()) {

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

Note: Check whether the error message is empty before override it in order to
expose the error of ParseImpl.

[Finnur, 2013/08/29 14:46:28]

Can you elaborate on why this is needed...
I don't follow...

[zhchbin, 2013/08/29 14:54:51]

If ParseImpl have some error, then the error message will be set. For example:
"kInvalidKeyBindingUnknownPlatform". We check only the return accelerator, the
key_code will be ui::VKEY_UNKNOWN no matter what error it encountered. Then we
override the detailed error, so we can only see errors::kInvalidKeyBinding in
this situation.

+          *error = ErrorUtils::FormatErrorMessageUTF16(
+              errors::kInvalidKeyBinding,
+              base::IntToString(index),
+              iter->first,
+              iter->second);
+        }
         return false;
       }
     }