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

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

R=finnur@chromium.org
TEST=unit_tests --gtest_filter=CommandsManifestTest.CommandManifestShouldNotCountMediaKeys
BUG=329870
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254176

[zhchbin, 2014-02-28 05:31:48]

Finnur, PTAL. :)

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

Change the CL description:
s/shotcut/shortcut/

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/commands/commands_handler.cc (right):

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:96: // We only allow
media keys to work without modifiers, and they should
s/We only allow media keys/Only media keys are allowed/

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:97: // not count
towards the max of four shotcuts per extension. See
s/shotcuts/shortcuts/

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:98: //
http://crbug.com/329870.
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 ..."

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:102:
binding->accelerator().key_code() == ui::VKEY_MEDIA_STOP);
DCHECK(CommandService::IsMediaKey(binding->accelerator())) ?

https://codereview.chromium.org/180783012/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json
(right):

https://codereview.chromium.org/180783012/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json:2:
"name": "Command test - mediaKeys should not counts towards the max of four
shotcuts per extension",
s/shotcuts/shortcuts/

https://codereview.chromium.org/180783012/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json:4:
"version": "2",
nit: version 1 ? :)

[Finnur, 2014-02-28 11:02:55]

Forgot:
... apart from this, LGTM.

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

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/commands/commands_handler.cc (right):

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:96: // We only allow
media keys to work without modifiers, and they should
On 2014/02/28 11:02:26, Finnur wrote:
> s/We only allow media keys/Only media keys are allowed/

Done.

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:97: // not count
towards the max of four shotcuts per extension. See
On 2014/02/28 11:02:26, Finnur wrote:
> s/shotcuts/shortcuts/

Done.

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:98: //
http://crbug.com/329870.
On 2014/02/28 11:02:26, Finnur wrote:
> 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 ..."

Done.

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:102:
binding->accelerator().key_code() == ui::VKEY_MEDIA_STOP);
On 2014/02/28 11:02:26, Finnur wrote:
> DCHECK(CommandService::IsMediaKey(binding->accelerator())) ?

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.

https://codereview.chromium.org/180783012/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json
(right):

https://codereview.chromium.org/180783012/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json:2:
"name": "Command test - mediaKeys should not counts towards the max of four
shotcuts per extension",
On 2014/02/28 11:02:26, Finnur wrote:
> s/shotcuts/shortcuts/

I shouldn't copy and paste without check. My bad.

https://codereview.chromium.org/180783012/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/manifest_tests/command_should_not_count_media_keys.json:4:
"version": "2",
On 2014/02/28 11:02:26, Finnur wrote:
> nit: version 1 ? :)

Done.

Copy from: "command_too_many.json".

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

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/commands/commands_handler.cc (right):

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:102:
binding->accelerator().key_code() == ui::VKEY_MEDIA_STOP);
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:20]

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/commands/commands_handler.cc (right):

https://codereview.chromium.org/180783012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/commands/commands_handler.cc:102:
binding->accelerator().key_code() == ui::VKEY_MEDIA_STOP);
On 2014/02/28 12:31:00, Finnur wrote:
> 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.

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.

[Finnur, 2014-02-28 12:55:41]

Yeah, I suspected as much. 

Actually, this IsMediaKey function should really live in
chrome/common/extensions, not c/b/extensions. 

Can you move it to chrome/common/extensions/command.h & .cc ?

[zhchbin, 2014-02-28 13:32:36]

On 2014/02/28 12:55:41, Finnur wrote:
> Yeah, I suspected as much. 
> 
> Actually, this IsMediaKey function should really live in
> chrome/common/extensions, not c/b/extensions. 
> 
> Can you move it to chrome/common/extensions/command.h & .cc ?

Done.

Thanks! Better location for IsMediaKey now, cheer!

[Finnur, 2014-02-28 14:00:54]

Yes, this is better.

Still LGTM.

[zhchbin, 2014-02-28 14:02:13]

The CQ bit was checked by zhchbin@gmail.com

[commit-bot: I haz the power, 2014-02-28 14:02:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/180783012/40001

[commit-bot: I haz the power, 2014-02-28 15:58:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-28 15:58:06]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[zhchbin, 2014-02-28 15:59:51]

The CQ bit was checked by zhchbin@gmail.com

[commit-bot: I haz the power, 2014-02-28 15:59:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/180783012/40001

[commit-bot: I haz the power, 2014-02-28 18:33:38]

Change committed as 254176