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

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

 // Copyright (c) 2013 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/common/extensions/api/commands/commands_handler.h"
 
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "extensions/common/error_utils.h"
@@ skipping 72 matching lines @@
           manifest_errors::kInvalidKeyBindingDictionary,
           base::IntToString(command_index));
       return false;
     }
 
     scoped_ptr<extensions::Command> binding(new Command());
     if (!binding->Parse(command, iter.key(), command_index, 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));
         return false;
       }
     }
 
     std::string command_name = binding->command_name();
     if (command_name == manifest_values::kBrowserActionCommandEvent) {
       commands_info->browser_action_command.reset(binding.release());
@@ skipping 29 matching lines @@
                     std::string(),
                     false));
   }
 }
 
 const std::vector<std::string> CommandsHandler::Keys() const {
   return SingleKey(keys::kCommands);
 }
 
 }  // namespace extensions