Media Keys should not count towards the max of four shortcuts per extension.

chrome/common/extensions/api/commands/commands_handler.cc

Index: chrome/common/extensions/api/commands/commands_handler.cc
diff --git a/chrome/common/extensions/api/commands/commands_handler.cc b/chrome/common/extensions/api/commands/commands_handler.cc
index d49a1056240f4ad8f3c57c01532116b8c1eb1b25..ed28d8dddad53904a2f8654ba46805370bd99a49 100644
--- a/chrome/common/extensions/api/commands/commands_handler.cc
+++ b/chrome/common/extensions/api/commands/commands_handler.cc
@@ -90,7 +90,19 @@ bool CommandsHandler::Parse(Extension* extension, base::string16* error) {
       return false;  // |error| already set.
 
     if (binding->accelerator().key_code() != ui::VKEY_UNKNOWN) {
-      if (++keybindings_found > kMaxCommandsWithKeybindingPerExtension) {
+      if (binding->accelerator().modifiers() != 0) {
+        ++keybindings_found;
+      } else {
+        // We only allow media keys to work without modifiers, and they should

[Finnur, 2014/02/28 11:02:26]

s/We only allow media keys/Only media keys are allowed/

[zhchbin, 2014/02/28 11:45:22]

Done.

+        // not count towards the max of four shotcuts per extension. See

[Finnur, 2014/02/28 11:02:26]

s/shotcuts/shortcuts/

[zhchbin, 2014/02/28 11:45:22]

Done.

+        // http://crbug.com/329870.

[Finnur, 2014/02/28 11:02:26]

The bug doesn't add much to the understanding of the issue so I would leave it
out of the comment. Instead I would just add what is between the underscores:
"... and _because media keys aren't registered exclusively_ they should not
count towards the max ..."

[zhchbin, 2014/02/28 11:45:22]

Done.

+        DCHECK(binding->accelerator().key_code() == ui::VKEY_MEDIA_NEXT_TRACK ||
+               binding->accelerator().key_code() == ui::VKEY_MEDIA_PREV_TRACK ||
+               binding->accelerator().key_code() == ui::VKEY_MEDIA_PLAY_PAUSE ||
+               binding->accelerator().key_code() == ui::VKEY_MEDIA_STOP);

[Finnur, 2014/02/28 11:02:26]

DCHECK(CommandService::IsMediaKey(binding->accelerator())) ?

[zhchbin, 2014/02/28 11:45:22]

I didn't use CommandService::IsMediaKey because it needs an additional header
file. And we can comment here for readability. WDYT?

And I also thinking about we should move IsMediaKey to somewhere more suitable?
But haven't find it yet.

[Finnur, 2014/02/28 12:31:00]

I don't see a problem with one additional header include, as long as there's no
include-rule that prevents you from including it here. Better than having
duplicate code, in my opinion.

[zhchbin, 2014/02/28 12:44:21]

Actually, I got the following message when 'git cl upload'

You added one or more #includes that violate checkdeps rules.
  chrome/common/extensions/api/commands/commands_handler.cc
      Illegal include:
"chrome/browser/extensions/api/commands/command_service.h"
    Because of "-chrome" from chrome's include_rules.

+      }
+
+      if (keybindings_found > kMaxCommandsWithKeybindingPerExtension) {
         *error = ErrorUtils::FormatErrorMessageUTF16(
             manifest_errors::kInvalidKeyBindingTooMany,
             base::IntToString(kMaxCommandsWithKeybindingPerExtension));