Continue key propagation for extensions without any listeners to chrome.commands.onCommand.

Allow key events to continue propagation for commands without any event listeners.

Rather than complicating the API with proposals of key re injection, or runtime registration/deregistration (which we might want for other reasons in the future), here's a very simple change that would probably give us the same functionality. If an extension remove all chrome.commands.onCommand listeners, then they will no longer receive the onCommand event and thereafter, the browser should continue propagating the key event and not drop it. I think this is actually the correct behavior, but also allows extension authors to have a little more control over when a command should be processed.

BUG=407163
Committed: https://crrev.com/fa5d9da251c9aa5333ab5a22ddd0bb51f44d1db2
Cr-Commit-Position: refs/heads/master@{#292997}

[David Tseng, 2014-08-26 19:33:16]

dtseng@chromium.org changed reviewers:
+ finnur@chromium.org

[David Tseng, 2014-08-26 19:33:16]

Publishing since it's a relatively trivial change.

Rather than complicating the API with proposals of key re injection, or runtime
registration/deregistration (which we might want for other reasons in the
future), here's a very simple change that would probably give us the same
functionality. If an extension removes all chrome.commands.onCommand listeners,
then they will no longer receive the onCommand event and thereafter, the browser
should continue propagating the key event and not drop it. I think this is
actually the correct behavior, but also allows extension authors to have a
little more control over when a command should be processed.

[Finnur, 2014-08-27 11:28:31]

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.cc:237: if
(!extensions::EventRouter::Get(browser_context_)
Be careful here: extension_id can be empty (see NotifyEventTargets above), and
I'm guessing that might cause this to return early, breaking some codepaths.

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.cc:240:
"commands.onCommand"))
This string literal also appears on line 118, so a const might be warranted at
the top.

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.cc:242: 
I'd like to see tests added for this functionality, both for global and
non-global extension. Also would have been good to run this through the try
servers. I'll add a few bots, just to see what happens.

[David Tseng, 2014-08-27 14:13:30]

W.I.P.

On Wed, Aug 27, 2014 at 4:28 AM, <finnur@chromium.org> wrote:

>
> https://codereview.chromium.org/504183004/diff/1/chrome/
> browser/extensions/extension_keybinding_registry.cc
> File chrome/browser/extensions/extension_keybinding_registry.cc (right):
>
> https://codereview.chromium.org/504183004/diff/1/chrome/
> browser/extensions/extension_keybinding_registry.cc#newcode237
> chrome/browser/extensions/extension_keybinding_registry.cc:237: if
> (!extensions::EventRouter::Get(browser_context_)
> Be careful here: extension_id can be empty (see NotifyEventTargets
> above), and I'm guessing that might cause this to return early, breaking
> some codepaths.
>
> https://codereview.chromium.org/504183004/diff/1/chrome/
> browser/extensions/extension_keybinding_registry.cc#newcode240
> chrome/browser/extensions/extension_keybinding_registry.cc:240:
> "commands.onCommand"))
> This string literal also appears on line 118, so a const might be
> warranted at the top.
>
> https://codereview.chromium.org/504183004/diff/1/chrome/
> browser/extensions/extension_keybinding_registry.cc#newcode242
> chrome/browser/extensions/extension_keybinding_registry.cc:242:
> I'd like to see tests added for this functionality, both for global and
> non-global extension. Also would have been good to run this through the
> try servers. I'll add a few bots, just to see what happens.
>
> Sorry, I wanted to sanity check the approach before proceeding; will mark
as w.I.P. in the future.

> https://codereview.chromium.org/504183004/
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2014-08-27 15:48:03]

> > Sorry, I wanted to sanity check the approach before proceeding; will mark
> as w.I.P. in the future.

No problem. And yes, would be helpful in the future.

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

w.i.p.

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.cc:237: if
(!extensions::EventRouter::Get(browser_context_)
On 2014/08/27 11:28:31, Finnur wrote:
> Be careful here: extension_id can be empty (see NotifyEventTargets above), and
> I'm guessing that might cause this to return early, breaking some codepaths.

You're totally right; this broke the majority of the [global]commands tests.

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.cc:240:
"commands.onCommand"))
On 2014/08/27 11:28:31, Finnur wrote:
> This string literal also appears on line 118, so a const might be warranted at
> the top.

Done.

https://codereview.chromium.org/504183004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.cc:242: 
On 2014/08/27 11:28:31, Finnur wrote:
> I'd like to see tests added for this functionality, both for global and
> non-global extension. Also would have been good to run this through the try
> servers. I'll add a few bots, just to see what happens.

Working on some tests now; the existing tests appear to pass in patchset 2 (at
least locally on Chrome OS).

[David Tseng, 2014-08-27 21:10:47]

Ready for review.

Includes a CommandsApiTest. I'm not sure it adds any value to add a test to
GlobalCommandsApiTest as it would largely duplicate the current test. Let me
know otherwise and I'd be happy to add it.

[David Tseng, 2014-08-27 22:09:44]

Patchset #4 (id:60001) has been deleted

[David Tseng, 2014-08-27 22:09:55]

Patchset #3 (id:40001) has been deleted

[Finnur, 2014-08-28 11:15:39]

Your new test seems to have failed on Mac...

> I'm not sure it adds any value to add a test to
> GlobalCommandsApiTest as it would largely duplicate
> the current test. 

Yeah, both messages go down the same funnel
(ExtensionKeybindingRegistry::NotifyEventTargets) so I'm not sure it would add
much coverage.

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_apitest.cc:713: // and notify the
background page.
I think it would help the casual reader to explain that the test doesn't define
an onCommand handler from the start, so the keystroke gets sent to the page
instead of the background page.

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:21: const char
kCommandsOnCommandEventName[] = "commands.onCommand";
nit: Why not simply kOnCommandEventName or kCommandHandlerEvent?

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js
(right):

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js:5:
var stage = 'DOM';
Add:
// Keeps track of who should be receiving keystrokes sent: 
// The web page (DOM) or the background page (onCommand).

And perhaps consider using more descriptive names.

Pseudo code:   expectedListener = [ "WebPage" | "BackgroundPage" ]

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js:13:
chrome.test.notifyFail('Unexpected call to onCommand');
nit: Change error to something like:
'Webpage expecting keystroke, but sent to extension instead'.

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js:24:
chrome.test.notifyFail('Unexpected key event on page');
nit: Change error to something like:
'Extension expecting keystroke, but sent to webpage instead'.

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js:27:
}    );
nit: Extra spaces.

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/keybinding/continue_propagation/manifest.json
(right):

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/manifest.json:16:
"CtrlShiftD": {
CtrlShiftD?

[David Tseng, 2014-08-28 23:37:11]

Not sure why the test fails on Mac, but hoping bringing the browser window to
front helps.

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_apitest.cc:713: // and notify the
background page.
On 2014/08/28 11:15:38, Finnur wrote:
> I think it would help the casual reader to explain that the test doesn't
define
> an onCommand handler from the start, so the keystroke gets sent to the page
> instead of the background page.

Done.

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:21: const char
kCommandsOnCommandEventName[] = "commands.onCommand";
On 2014/08/28 11:15:39, Finnur wrote:
> nit: Why not simply kOnCommandEventName or kCommandHandlerEvent?

Done. (Was trying to follow the "new" format generated from idl's
namespace::kFooEventName).

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js
(right):

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js:5:
var stage = 'DOM';
On 2014/08/28 11:15:39, Finnur wrote:
> Add:
> // Keeps track of who should be receiving keystrokes sent: 
> // The web page (DOM) or the background page (onCommand).
> 
> And perhaps consider using more descriptive names.
> 
> Pseudo code:   expectedListener = [ "WebPage" | "BackgroundPage" ]

Done.

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/background.js:13:
chrome.test.notifyFail('Unexpected call to onCommand');
On 2014/08/28 11:15:39, Finnur wrote:
> nit: Change error to something like:
> 'Webpage expecting keystroke, but sent to extension instead'.

Done.

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/keybinding/continue_propagation/manifest.json
(right):

https://codereview.chromium.org/504183004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/continue_propagation/manifest.json:16:
"CtrlShiftD": {
On 2014/08/28 11:15:39, Finnur wrote:
> CtrlShiftD?

Earlier keystroke I used which for some reason, doesn't seem to be assignable.
Changed.

[Finnur, 2014-08-29 12:17:01]

Looks like the test is still failing, but the good news is that it fails on the
try servers, so you can add trace statements and iterate without having to check
in.

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:21: const char
kCommandsOnCommandEventName[] = "commands.onCommand";
Do we do that elsewhere?

https://codereview.chromium.org/504183004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/504183004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:713: // Activate the
shortcut (Ctrl+Shift+F). The page should capture the keydown
s/The page/The page (as opposed to the extension)/

https://codereview.chromium.org/504183004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:714: // and notify the
background page because |onCommands| has no event listeners
s/onCommands/onCommand/
s/listeners/listener/

[David Tseng, 2014-08-29 17:18:46]

Answering questions; still looking at the try failure.

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/504183004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:21: const char
kCommandsOnCommandEventName[] = "commands.onCommand";
On 2014/08/29 12:17:00, Finnur wrote:
> Do we do that elsewhere?

Not sure what you mean; an example is location::OnLocationError::kEventName
which gets generated from an idl.

In this instance, I'm fine with keeping this constant in an empty namespace. The
real fix would be to transition the json api declaration into idl.

https://codereview.chromium.org/504183004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/504183004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:713: // Activate the
shortcut (Ctrl+Shift+F). The page should capture the keydown
On 2014/08/29 12:17:00, Finnur wrote:
> s/The page/The page (as opposed to the extension)/

Way too wordy now...rephrased.

https://codereview.chromium.org/504183004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:714: // and notify the
background page because |onCommands| has no event listeners
On 2014/08/29 12:17:00, Finnur wrote:
> s/onCommands/onCommand/
> s/listeners/listener/

Done.

[Finnur, 2014-09-01 10:46:10]

LGTM, once the test failure has been figured out.

[David Tseng, 2014-09-02 17:18:45]

Patchset #6 (id:180001) has been deleted

[David Tseng, 2014-09-02 17:54:59]

Patchset #6 (id:260001) has been deleted

[David Tseng, 2014-09-02 17:55:44]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-09-02 17:56:48]

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

[commit-bot: I haz the power, 2014-09-02 19:46:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-02 21:09:25]

Committed patchset #6 (id:280001) as 0cc73280f29c2745c8b571d8df0dc110a933cb22

[commit-bot: I haz the power, 2014-09-10 03:20:45]

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