Begin whitelisting specific extensions for global key registration.

Begin whitelisting specific extensions for global key registration.

Adds whitelist to commands API to relax restrictions on:
- global accelerator keys
- maximum number of commands
- override previously taken accelerators

This cl also involved fixing the use of the search key while using the global commands listener. The 'command' modifier was being dropped while making its way to the renderer and back.

BUG=404768
TEST=With a sample extension binding
"Search+Shift+L" and
"global": true

press and verify callback when
focus is inside of web content
focus is on the status tray

Committed: https://crrev.com/28ba7f5f7ba661275a7bf6081b2e21a2270f25eb
Cr-Commit-Position: refs/heads/master@{#291628}

[Finnur, 2014-07-17 11:00:52]

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:570: }
This seems a bit hacky. We already have a permission system for extensions, with
the ability to restrict to certain platforms/extension IDs. That seems like a
better fit for this.

[David Tseng, 2014-07-18 22:54:51]

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:570: }
On 2014/07/17 11:00:52, Finnur wrote:
> This seems a bit hacky. We already have a permission system for extensions,
with
> the ability to restrict to certain platforms/extension IDs. That seems like a
> better fit for this.
> 

Are you referring to _permission_features(.json)?

If so, which permission did you have in mind?

[Finnur, 2014-08-11 14:01:07]

BUG=none

I believe we want to file a bug for all work items, so they can be tracked, for
example by QA.

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:570: }
Yes, for example, you can check in code whether an extension has certain
permissions, such as fileSystem.write by doing:

extension->permissions_data()->HasAPIPermission(APIPermission::kFileSystemWrite);

For your purposes, you could add a section to _permission_features.json
called...

commands.accessibility
(or something)

... and whitelist your extension for that permission (and then check for that
permission here instead of the id). That way it is really easy to update the
list if we need to add other extensions to it. Furthermore, there might be more
places you need to add this check...

So, I can see three rule restrictions that, depending on the use case, one would
want relaxed:

1) Allow overwriting of already taken shortcuts.
2) Allow overwriting of *global* shortcuts other than Ctrl+Shift+[0-9].
Remember: Global shortcuts work when Chrome does not have focus.
3) Relax the restriction on the number of keys a developer can assign to one
extension (currently maxed out at 4, not counting MediaKeys).

So, your CL is relaxing only 2. Remind me again why it is important for
accessibility shortcuts to work when Chrome does _not_ have focus?

[David Tseng, 2014-08-19 22:47:50]

Just answering a few questions.

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:570: }
On 2014/08/11 14:01:07, Finnur wrote:
> Yes, for example, you can check in code whether an extension has certain
> permissions, such as fileSystem.write by doing:
> 
>
extension->permissions_data()->HasAPIPermission(APIPermission::kFileSystemWrite);
> 
> For your purposes, you could add a section to _permission_features.json
> called...
> 
> commands.accessibility
> (or something)
> 
> ... and whitelist your extension for that permission (and then check for that
> permission here instead of the id). That way it is really easy to update the
> list if we need to add other extensions to it. Furthermore, there might be
more
> places you need to add this check...
> 
> So, I can see three rule restrictions that, depending on the use case, one
would
> want relaxed:
> 
> 1) Allow overwriting of already taken shortcuts.
> 2) Allow overwriting of *global* shortcuts other than Ctrl+Shift+[0-9].
> Remember: Global shortcuts work when Chrome does not have focus.
> 3) Relax the restriction on the number of keys a developer can assign to one
> extension (currently maxed out at 4, not counting MediaKeys).
> 
> So, your CL is relaxing only 2. Remind me again why it is important for
> accessibility shortcuts to work when Chrome does _not_ have focus?


A few cases. Suppose focus (in Chrome OS) is on the views-based toolbar or if
there are no browser windows open at all. We still want an accessibility
extension to receive keyboard commands (e.g. to read the current time).

As for the points above, I think we will need all three relaxations.

[Finnur, 2014-08-20 12:41:14]

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:570: }
OK, then that's definitely something you'd want to add an API permission for,
because you'd otherwise have to spread if checks for the extension_id around the
codebase and we'd need to update each location when adding/removing extensions.
The HasAPIPermission makes us only have to update a single file to give/take
permission to/from an extension.

[David Tseng, 2014-08-20 23:24:26]

PTAL; added a new permission "commands.accessibility" which contains a whitelist
of all extensions that have the previously discussed restrictions lifted.

[Finnur, 2014-08-21 11:18:11]

https://codereview.chromium.org/399783002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:570: }
nit: No need for braces (single line if clause).

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:307: },
See comments at the top of the file for additional steps.

I also discovered an email thread that is relevant. Let me add you to it.

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

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/commands/commands_handler.cc:13: #include
"extensions/common/permissions//permissions_data.h"
nit: double slash.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command.cc:81: tokens[i] == values::kKeySearch) {
This isn't right. This makes Search map over to Command on Mac and allows
Command to be mapped to Search on ChromeOS, which we don't want.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command.cc:116: }
Is this copy-pasted from an old version of a CL I recently reviewed?

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command.cc:327: #endif
Yeah, it seems like this CL could use a sync up with master...

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/command_unittest.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command_unittest.cc:83: }
Same here. Version we reviewed before is more up-to-date.

[David Tseng, 2014-08-21 16:03:04]

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command.cc:81: tokens[i] == values::kKeySearch) {
On 2014/08/21 11:18:11, Finnur wrote:
> This isn't right. This makes Search map over to Command on Mac and allows
> Command to be mapped to Search on ChromeOS, which we don't want.

Hmm...this cl was branched off of the other change. Let me rebase and upload
again.

[David Tseng, 2014-08-21 16:17:57]

https://codereview.chromium.org/399783002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:570: }
On 2014/08/21 11:18:10, Finnur wrote:
> nit: No need for braces (single line if clause).

Mixed on this one. The conditional is multiple lines, so generally braces are
also required? I'm ok either way. Changed to stay consistent with this file.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:307: },
On 2014/08/21 11:18:10, Finnur wrote:
> See comments at the top of the file for additional steps.

Was previously done; other than making note of the mapping of SHA-1 back to id,
was there anything else?

> 
> I also discovered an email thread that is relevant. Let me add you to it.
Got it; thanks.

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

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/commands/commands_handler.cc:13: #include
"extensions/common/permissions//permissions_data.h"
On 2014/08/21 11:18:10, Finnur wrote:
> nit: double slash.

Done.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command.cc:116: }
On 2014/08/21 11:18:10, Finnur wrote:
> Is this copy-pasted from an old version of a CL I recently reviewed?

Acknowledged.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command.cc:327: #endif
On 2014/08/21 11:18:10, Finnur wrote:
> Yeah, it seems like this CL could use a sync up with master...

Acknowledged.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/command_unittest.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/command_unittest.cc:83: }
On 2014/08/21 11:18:11, Finnur wrote:
> Same here. Version we reviewed before is more up-to-date.

Acknowledged.

[Finnur, 2014-08-21 16:23:33]

https://codereview.chromium.org/399783002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:570: }
Yup. It is a common misunderstanding of the style guide that because the
*conditional* is multi-line then single-line if and else clauses that follow
need braces. They don't.

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/399783002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:307: },
I'm not sure if more is needed. Lets see what comes out of the email thread.

https://codereview.chromium.org/399783002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/399783002/diff/40001/chrome/common/extensions...
chrome/common/extensions/command.cc:83: platform_key ==
values::kKeybindingPlatformChromeOs) {
This still makes Search work as Command on Mac and Command as Search on
ChromeOS. That's not what we want. Isn't this taken care of by the other CL?
(can we drop these changes)

[David Tseng, 2014-08-21 17:24:51]

https://codereview.chromium.org/399783002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/command.cc (right):

https://codereview.chromium.org/399783002/diff/40001/chrome/common/extensions...
chrome/common/extensions/command.cc:83: platform_key ==
values::kKeybindingPlatformChromeOs) {
On 2014/08/21 16:23:32, Finnur wrote:
> This still makes Search work as Command on Mac and Command as Search on
> ChromeOS. That's not what we want. Isn't this taken care of by the other CL?
> (can we drop these changes)

Bad rebase; done.

[Finnur, 2014-08-21 17:54:40]

One nit, then LGTM.

https://codereview.chromium.org/399783002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/399783002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:281:
"75C7F4B720314B6CB1B5817CD86089DB95CD2461"
Don't you need the link to the bug you created here?

[David Tseng, 2014-08-21 19:43:55]

https://codereview.chromium.org/399783002/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/399783002/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:281:
"75C7F4B720314B6CB1B5817CD86089DB95CD2461"
On 2014/08/21 17:54:40, Finnur wrote:
> Don't you need the link to the bug you created here?

Done.

[David Tseng, 2014-08-22 16:47:34]

+ sky, kalman for OWNERS.

[not at google - send to devlin, 2014-08-22 16:55:19]

lgtm

https://codereview.chromium.org/399783002/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/399783002/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:278: "extension_types":
["extension", "legacy_packaged_app", "platform_app"],
We should stop granting APIs to legacy_packaged_app.

[sky, 2014-08-22 19:31:58]

content/browser/renderer_host/native_web_keyboard_event.cc LGTM

[David Tseng, 2014-08-24 18:39:42]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-08-24 18:40:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtseng@chromium.org/399783002/100001

[commit-bot: I haz the power, 2014-08-24 19:34:28]

Committed patchset #6 (100001) as 528ed1e6ad55a15100a8b6c0cb5bfc36a3f6d9f2

[commit-bot: I haz the power, 2014-09-10 02:32:41]

Patchset 6 (id:??) landed as
https://crrev.com/28ba7f5f7ba661275a7bf6081b2e21a2270f25eb
Cr-Commit-Position: refs/heads/master@{#291628}