[Windows] Finish global and non-global media keys support on Windows.

[Windows] Finish global and non-global media keys support on Windows.

This patch make sure that Media keys go to all apps/extensions that register for
them.

BUG=131612, 302437
TEST=interactive_ui_tests --gtest_filter=CommandsApiTest.AllowDuplicatedMediaKeys
TEST=interactive_ui_tests --gtest_filter=GlobalCommandsApiTest.GlobalDuplicatedMediaKey

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236707

[zhchbin, 2013-11-12 14:29:29]

PTAL :)

[Finnur, 2013-11-12 16:27:53]

Great to see progress here. Thank you for working on this.

A few questions/comments...

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:118: // staitc
s/staitc/static/

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:212: if
(IsMediaKey(accelerator))
What happens if I assign a Media key to a browser action? Shouldn't we exclude
page action and browser action commands from getting this prefix?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:213: key += (":" +
base::IntToString(media_keys_command_counter_++));
nit: Do you need the parens around the whole expression?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:312: shortcut =
tokens[1];  // The second token is the shortcut that assigned.
nit: Drop the word 'that'

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:338:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
I think we always want the ChromeAccelerator check, in case MediaKey was one of
those. Remember, this is to avoid having Extension Developers steal Chrome
Accelerators. It is less of a concern for MediaKeys, but we can always relax the
restriction, if we need to.

How about:

if (!chrome::IsChromeAccelerator(iter->second.accelerator(), profile_) &&
    (IsWhitelistedGlobalShortcut(iter->second) ||
        extensions::CommandService::IsMediaKey(iter->second.accelerator()))) {

Does that work?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:72: // key.
You don't mention that e.g. a Ctrl+Next Track returns false...

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:192: // A counter that
used to append to the key of keybindings map when adding
s/that/that is/, but see below.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:195: int
media_keys_command_counter_;
I don't think this is going to work. For example, when you restart the browser,
you'll reset the number and start overwriting your previous bindings. Also,
think of sync -- you might be setting media keybindings on two different
computers and both might be going for the same counter value.

Can't we make the key unique by using extension id + command name instead?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry.cc:31: }
I'm not too keen on the iter -> eit/mit change here (and elsewhere).

Can you switch these two for loops to look like this instead? 

for (EventTargets::const_iterator iter = event_targets_.begin(); iter !=
event_targets_.end(); ++iter)

Then both can use iter as the name (iterator scoped within the for loop).

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:80: }
Why do we need to copy this function?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
I think this is not the right approach.
1) I'm not entirely sure why this function works but the one in ui_test_utils
does not. I'd like to understand that better.
2) If we absolutely need to do this, then we should split the function up into
two (like you have done) but have them both live there (and one call the other)
and not have one live here (and be almost an exact copy of the other one).

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

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:63: TargetList
&target_list = mit->second;
nit: Prefer Foo* bar over Foo *bar.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:64:
TargetList::iterator target_list_it = target_list.begin();
s/target_list_it/target/

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:68: target_list_it =
target_list.erase(target_list_it);
nit: Weird indentation.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:105:
MediaKeysEventTargets::iterator targets_iterator =
s/targets_iterator/targets/

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:115: }
nit: You don't need braces for single line if clauses.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.h:99: // whereas on
other platforms it does not.
Document here that this is used for all keys except MediaKeys, and that that is
because MediaKeys have a 1 to many relationship with commands. But see below
first.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.h:108:
MediaKeysEventTargets media_keys_event_targets_;
I think you should just use only the map you create here for all keys (not just
MediaKeys)? Non-MediaKeys would then just be like a MediaKey that has only one
target in the list, right? Then it looks like you can do away with some amount
of code and complexity from the .cc file (Perhaps you can then make the
ismediakey function private, although that's low priority compared to the other
things I mentioned).

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/global_shortcut_listener_win.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/global_shortcut_listener_win.cc:76: return;  // The
shortcut has been registered.
Perhaps document that this is because some shortcuts, such as MediaKeys can have
multiple targets, all keyed off of the same accelerator.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:41: }
nit: Single line if clause, you can skip the braces.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:90:
accelerator);
It is good to consolidate this in the base class. Thanks.

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/keybinding/global_media_keys_1/manifest.json
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/keybinding/global_media_keys_1/manifest.json:14:
"description": "Test passes if called."
nit: "Description" before "global" (alphabetical order). Also, don't you want
the stop event here too, or?

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/keybinding/non_global_media_keys_0/manifest.json
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/keybinding/non_global_media_keys_0/manifest.json:17:
"default": "MediaStop"
Hmmm... An extension can register for the same shortcut twice. I don't think
that would be useful for extension developers and is likely to be the result of
an error (e.g. copy paste). I would actually enforce one command per shortcut by
throwing an error (and test for it).

[zhchbin, 2013-11-13 05:01:57]

Updated.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:118: // staitc
On 2013/11/12 16:27:54, Finnur wrote:
> s/staitc/static/

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:212: if
(IsMediaKey(accelerator))
On 2013/11/12 16:27:54, Finnur wrote:
> What happens if I assign a Media key to a browser action? Shouldn't we exclude
> page action and browser action commands from getting this prefix?

1. How can it happen? We only parse Media Keys for named command, (Maybe I miss
out on something), so we didn't need to do that. 
2. But from the perspective of this function, we should exclude for someone may
change the parse code.

I choose the first one. WDYH?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:213: key += (":" +
base::IntToString(media_keys_command_counter_++));
On 2013/11/12 16:27:54, Finnur wrote:
> nit: Do you need the parens around the whole expression?

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:312: shortcut =
tokens[1];  // The second token is the shortcut that assigned.
On 2013/11/12 16:27:54, Finnur wrote:
> nit: Drop the word 'that'

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:338:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
On 2013/11/12 16:27:54, Finnur wrote:
> I think we always want the ChromeAccelerator check, in case MediaKey was one
of
> those. Remember, this is to avoid having Extension Developers steal Chrome
> Accelerators. It is less of a concern for MediaKeys, but we can always relax
the
> restriction, if we need to.
> 
> How about:
> 
> if (!chrome::IsChromeAccelerator(iter->second.accelerator(), profile_) &&
>     (IsWhitelistedGlobalShortcut(iter->second) ||
>         extensions::CommandService::IsMediaKey(iter->second.accelerator()))) {
> 
> Does that work?

Not. !chrome::IsChromeAccelerator will be false for Media Keys on aura build.
See the implementation of this function: [0]

[0]:https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/accelerator_utils_aura.cc

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:72: // key.
On 2013/11/12 16:27:54, Finnur wrote:
> You don't mention that e.g. a Ctrl+Next Track returns false...

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:192: // A counter that
used to append to the key of keybindings map when adding
On 2013/11/12 16:27:54, Finnur wrote:
> s/that/that is/, but see below.

Done. Removed.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:195: int
media_keys_command_counter_;
On 2013/11/12 16:27:54, Finnur wrote:
> I don't think this is going to work. For example, when you restart the
browser,
> you'll reset the number and start overwriting your previous bindings. Also,
> think of sync -- you might be setting media keybindings on two different
> computers and both might be going for the same counter value.
> 
> Can't we make the key unique by using extension id + command name instead?

Done. Now using extension id for make the key quique. I make a stupid mistake
here. Sigh..

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry.cc:31: }
On 2013/11/12 16:27:54, Finnur wrote:
> I'm not too keen on the iter -> eit/mit change here (and elsewhere).
> 
> Can you switch these two for loops to look like this instead? 
> 
> for (EventTargets::const_iterator iter = event_targets_.begin(); iter !=
> event_targets_.end(); ++iter)
> 
> Then both can use iter as the name (iterator scoped within the for loop).

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
On 2013/11/12 16:27:54, Finnur wrote:
> I think this is not the right approach.
> 1) I'm not entirely sure why this function works but the one in ui_test_utils
> does not. I'd like to understand that better.
> 2) If we absolutely need to do this, then we should split the function up into
> two (like you have done) but have them both live there (and one call the
other)
> and not have one live here (and be almost an exact copy of the other one).

You can do an experiment: 
1. Change the SendNativeKeyEvent in my test case to ui_test_utils one. Then you
can find it fail.
2. Now add a little trick patch to /ui/base/test/ui_controls_internal_win.cc

diff --git a/ui/base/test/ui_controls_internal_win.cc
b/ui/base/test/ui_controls_internal_win.cc
index 45b8f45..9e264bd 100644
--- a/ui/base/test/ui_controls_internal_win.cc
+++ b/ui/base/test/ui_controls_internal_win.cc
@@ -246,6 +246,11 @@ bool SendKeyPressImpl(HWND window,
     i++;
   }

+  if (!control && !shift && !alt) {
+    FillKeyboardInput(ui::VKEY_CONTROL, &input[i++], false);
+    FillKeyboardInput(ui::VKEY_CONTROL, &input[i++], true);
+  }
+
   if (::SendInput(i, input, sizeof(INPUT)) != i)
     return false;

With this trick way, you can find it can work now.

The difference between my custom function and the one in ui_test_utils is:
ui_test_utils one will wait the WM_KEYUP event in its message loop.

The purpose I copied the function here is want to show you the difference and
asking for advice.

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

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:63: TargetList
&target_list = mit->second;
On 2013/11/12 16:27:54, Finnur wrote:
> nit: Prefer Foo* bar over Foo *bar.

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:64:
TargetList::iterator target_list_it = target_list.begin();
On 2013/11/12 16:27:54, Finnur wrote:
> s/target_list_it/target/

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:68: target_list_it =
target_list.erase(target_list_it);
On 2013/11/12 16:27:54, Finnur wrote:
> nit: Weird indentation.

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:105:
MediaKeysEventTargets::iterator targets_iterator =
On 2013/11/12 16:27:54, Finnur wrote:
> s/targets_iterator/targets/

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.cc:115: }
On 2013/11/12 16:27:54, Finnur wrote:
> nit: You don't need braces for single line if clauses.

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.h:108:
MediaKeysEventTargets media_keys_event_targets_;
On 2013/11/12 16:27:54, Finnur wrote:
> I think you should just use only the map you create here for all keys (not
just
> MediaKeys)? Non-MediaKeys would then just be like a MediaKey that has only one
> target in the list, right? Then it looks like you can do away with some amount
> of code and complexity from the .cc file (Perhaps you can then make the
> ismediakey function private, although that's low priority compared to the
other
> things I mentioned).

Yes. I have this idea before. My concern is that I didn't have development
device that can support OS X. Change this need some work in
extension_keybinding_registry_cocoa.mm, though it can be trivial, I still afraid
that I would make some stupid mistake.

Let's make sure other things work first of all. Then I would refactor this one.
:)

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/global_shortcut_listener_win.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/global_shortcut_listener_win.cc:76: return;  // The
shortcut has been registered.
On 2013/11/12 16:27:54, Finnur wrote:
> Perhaps document that this is because some shortcuts, such as MediaKeys can
have
> multiple targets, all keyed off of the same accelerator.

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:41: }
On 2013/11/12 16:27:54, Finnur wrote:
> nit: Single line if clause, you can skip the braces.

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/keybinding/global_media_keys_1/manifest.json
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/keybinding/global_media_keys_1/manifest.json:14:
"description": "Test passes if called."
On 2013/11/12 16:27:54, Finnur wrote:
> nit: "Description" before "global" (alphabetical order). Also, don't you want
> the stop event here too, or?

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/keybinding/non_global_media_keys_0/manifest.json
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/keybinding/non_global_media_keys_0/manifest.json:17:
"default": "MediaStop"
On 2013/11/12 16:27:54, Finnur wrote:
> Hmmm... An extension can register for the same shortcut twice. I don't think
> that would be useful for extension developers and is likely to be the result
of
> an error (e.g. copy paste). I would actually enforce one command per shortcut
by
> throwing an error (and test for it).

Since I have changed the key by appending extension id (instead of my stupid
counter) when adding media key binding preference, so now it's one command per
shortcut when installing an extension. I would try to add this rule when parsing
commands from manifest.

[Finnur, 2013-11-13 10:22:24]

There's a typo in the CL description: "non-gloabl"

Apart from that, here are my comments:

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:212: if
(IsMediaKey(accelerator))
That's fine. Perhaps just add a DCHECK inside the if clause that command_name is
not a page action/browser action (when IsMediaKey is true)?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:338:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
So, I would actually expect IsChromeAccelerator to return false for media keys
on all platforms. And that's good, because then the code works on all platforms.

This is more so that if we the Chrome UI guys decide in the future to make
MediaKeys into reserved ChromeAccelerators that extension devs can't steal them
from them anymore. You know, like it works today with Ctrl+F (it's taken by
FindInPage so extension devs can't set it, but the user can override it).

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.h:195: int
media_keys_command_counter_;
Ah, don't beat yourself too hard. :)

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.h:108:
MediaKeysEventTargets media_keys_event_targets_;
How about this...

You make the change for Windows and Linux in this patch. I'll patch this
changelist into my Mac box, make the corresponding change there and give you
what is needed to include in this patch?

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

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:12: #include
"base/strings/string_split.h"
Can either of this include be deleted now?

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:211: if
(IsMediaKey(accelerator))
Please document why we do this. Perhaps something along the lines of:

Media keys have a 1-to-many relationship with targets, unlike regular shortcut
(1-to-1 relationship). That means two or more extensions can register for the
same media key so the extension ID needs to be added to the key to make sure the
key is unique.

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_win.cc (right):

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_win.cc:76: // The shortcut
has been registered. Some shortcuts, such as MediaKeys can
nit: add the word 'already' in front of 'been'.

[zhchbin, 2013-11-13 16:05:10]

PTAL.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:212: if
(IsMediaKey(accelerator))
On 2013/11/13 10:22:24, Finnur wrote:
> That's fine. Perhaps just add a DCHECK inside the if clause that command_name
is
> not a page action/browser action (when IsMediaKey is true)?

Done.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:338:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
On 2013/11/13 10:22:24, Finnur wrote:
> So, I would actually expect IsChromeAccelerator to return false for media keys
> on all platforms. And that's good, because then the code works on all
platforms.
> 
> This is more so that if we the Chrome UI guys decide in the future to make
> MediaKeys into reserved ChromeAccelerators that extension devs can't steal
them
> from them anymore. You know, like it works today with Ctrl+F (it's taken by
> FindInPage so extension devs can't set it, but the user can override it).

Do you mean remove media keys in the reserved ChromeAccelerators to make it
return false for now? Then in the future maybe we will keep them reserved again?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
On 2013/11/13 05:01:57, zhchbin wrote:
> On 2013/11/12 16:27:54, Finnur wrote:
> > I think this is not the right approach.
> > 1) I'm not entirely sure why this function works but the one in
ui_test_utils
> > does not. I'd like to understand that better.
> > 2) If we absolutely need to do this, then we should split the function up
into
> > two (like you have done) but have them both live there (and one call the
> other)
> > and not have one live here (and be almost an exact copy of the other one).
> 
> You can do an experiment: 
> 1. Change the SendNativeKeyEvent in my test case to ui_test_utils one. Then
you
> can find it fail.
> 2. Now add a little trick patch to /ui/base/test/ui_controls_internal_win.cc
> 
> diff --git a/ui/base/test/ui_controls_internal_win.cc
> b/ui/base/test/ui_controls_internal_win.cc
> index 45b8f45..9e264bd 100644
> --- a/ui/base/test/ui_controls_internal_win.cc
> +++ b/ui/base/test/ui_controls_internal_win.cc
> @@ -246,6 +246,11 @@ bool SendKeyPressImpl(HWND window,
>      i++;
>    }
> 
> +  if (!control && !shift && !alt) {
> +    FillKeyboardInput(ui::VKEY_CONTROL, &input[i++], false);
> +    FillKeyboardInput(ui::VKEY_CONTROL, &input[i++], true);
> +  }
> +
>    if (::SendInput(i, input, sizeof(INPUT)) != i)
>      return false;
> 
> With this trick way, you can find it can work now.
> 
> The difference between my custom function and the one in ui_test_utils is:
> ui_test_utils one will wait the WM_KEYUP event in its message loop.
> 
> The purpose I copied the function here is want to show you the difference and
> asking for advice.

Maybe you ignore this comment?

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_keybinding_registry.h:108:
MediaKeysEventTargets media_keys_event_targets_;
On 2013/11/13 10:22:24, Finnur wrote:
> How about this...
> 
> You make the change for Windows and Linux in this patch. I'll patch this
> changelist into my Mac box, make the corresponding change there and give you
> what is needed to include in this patch?

Done.

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

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:12: #include
"base/strings/string_split.h"
On 2013/11/13 10:22:24, Finnur wrote:
> Can either of this include be deleted now?

Done.

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:211: if
(IsMediaKey(accelerator))
On 2013/11/13 10:22:24, Finnur wrote:
> Please document why we do this. Perhaps something along the lines of:
> 
> Media keys have a 1-to-many relationship with targets, unlike regular shortcut
> (1-to-1 relationship). That means two or more extensions can register for the
> same media key so the extension ID needs to be added to the key to make sure
the
> key is unique.

Done.

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_win.cc (right):

https://codereview.chromium.org/64273008/diff/180001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_win.cc:76: // The shortcut
has been registered. Some shortcuts, such as MediaKeys can
On 2013/11/13 10:22:24, Finnur wrote:
> nit: add the word 'already' in front of 'been'.

Done.

[Finnur, 2013-11-15 21:10:10]

Sorry for the lack of updates on this; I started reviewing, then got distracted
with another very important task. Then merged your CL on my Mac, made the
changes necessary and sent the CL for a try but for some reason never got the
email results back -- so I forgot to check on it (and now I don't even see my CL
on Rietveld so I don't know the status) :s

Anyway, just came here to let you know you are not forgotten. :)

Might as well send the two comments I had amassed...

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.cc (left):

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:57: break;
Don't we still need this?

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.h:96: // Maps an
accelerator to a list of string pair (extension id, command name)
s/pair/pairs/

[zhchbin, 2013-11-16 01:33:30]

> Sorry for the lack of updates on this; I started reviewing, then got
distracted with another very important task. Then merged your CL on my Mac, made
the changes necessary and sent the CL for a try but for some reason never got
the email results back -- so I forgot to check on it (and now I don't even see
my CL on Rietveld so I don't know the status) :s
> Anyway, just came here to let you know you are not forgotten. :)

Aha, I can see your patch Set 7 "Cocoa Support". I uploaded a patch set to fix a
DCHECK fail on register_manager.cc (as well as including your patch on cocoa),
so maybe my patch set is misleading you.

I'm waiting for code review patiently always, suffering from the time zone
difference.

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.cc (left):

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:57: break;
On 2013/11/15 21:10:10, Finnur wrote:
> Don't we still need this?

Done. I had noticed this, but seems I'm wrong at that time.

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/330001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.h:96: // Maps an
accelerator to a list of string pair (extension id, command name)
On 2013/11/15 21:10:10, Finnur wrote:
> s/pair/pairs/

Done.

[Finnur, 2013-11-18 11:16:13]

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:338:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
Hmm... Are they already reserved? OK, then that won't work. Since we're not
exclusive on the media key shortcuts we can probably make an exception on this
and keep it as is.

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
I didn't have a good advice for you because I don't feel like I understand the
problem well. I'd like to take a look at the message loop that handles it as it
seems like that loop is wrong. Where is it exactly? Specifically, there is a
keyup message sent (when the non-modifier key is set in the input stream) -- but
I presume the loop assume the keyup must be present on a non-modifier key)? 

Maybe we should change the messageloop (and the test code) to send a custom
WM_USER message (or something) to let it know that no further processing is to
be expected?

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

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:217: // Media Keys are
allowed using in named command only.
nit: s/using in/for use by/

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:223: }
This logic should be moved to GetPlatformKeybindingKeyForAccelerator.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:322: shortcut =
tokens[1];  // The second token is the shortcut assigned.
nit: Instead of this comment, I would have a comment on line 318 (or 317):

// Format stored in Preferences is: "Platform:Shortcut[:ExtensionId]".

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:348:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
Add a comment on line 346:

Make sure registered Chrome shortcuts cannot be automatically assigned
(overwritten) by extension developers. Media keys are an exception here.

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

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:48: target =
target_list.erase(target);
Um, how does this work? Don't you need to advance |target| before erasing?

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:51: }
Nit: Both of these are single-line so no braces needed (but address comment
above first).

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:63: break;
I think this should be outside the if clause that ends on line 64. If we found
the right command name we can quit, it doesn't matter whether we erase all the
targets, we won't find the same |command_name| in the rest of the map.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:97: return true;
nit: Line break above.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.h:90: bool
NotifyEventTargetsByAccelerator(const ui::Accelerator& accelerator);
You can probably skip the 'ByAccelerator' from the name.
Also, in the comment: s/accelerator/|accelerator|/ to indicate you are talking
about a param to the function.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_win.cc (right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_win.cc:80: }
I presume we'll need this in other platform-specific code?

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:72: //
type != named command, so we need to dispatch this event directly.
nit: Capitalize 'type'.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:67:
continue;   // We have register it to the focus manager.
Comment-nit: It is already registered with the focus manager.

[zhchbin, 2013-11-18 13:10:13]

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:338:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
On 2013/11/18 11:16:14, Finnur wrote:
> Hmm... Are they already reserved? OK, then that won't work. Since we're not
> exclusive on the media key shortcuts we can probably make an exception on this
> and keep it as is.

Yes, you can find the reserved shortcuts on aura here:
ash/accelerators/accelerator_table.cc

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
On 2013/11/18 11:16:14, Finnur wrote:
> I didn't have a good advice for you because I don't feel like I understand the
> problem well. I'd like to take a look at the message loop that handles it as
it
> seems like that loop is wrong. Where is it exactly? 

The message loop is:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...

> Specifically, there is a
> keyup message sent (when the non-modifier key is set in the input stream) --
but
> I presume the loop assume the keyup must be present on a non-modifier key)? 
> 
> Maybe we should change the messageloop (and the test code) to send a custom
> WM_USER message (or something) to let it know that no further processing is to
> be expected?

And the detail of windows:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/ui_co....

I have no idea how we can fix it yet.

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

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:217: // Media Keys are
allowed using in named command only.
On 2013/11/18 11:16:14, Finnur wrote:
> nit: s/using in/for use by/

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:223: }
On 2013/11/18 11:16:14, Finnur wrote:
> This logic should be moved to GetPlatformKeybindingKeyForAccelerator.

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:322: shortcut =
tokens[1];  // The second token is the shortcut assigned.
On 2013/11/18 11:16:14, Finnur wrote:
> nit: Instead of this comment, I would have a comment on line 318 (or 317):
> 
> // Format stored in Preferences is: "Platform:Shortcut[:ExtensionId]".

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:348:
extensions::CommandService::IsMediaKey(iter->second.accelerator())) {
On 2013/11/18 11:16:14, Finnur wrote:
> Add a comment on line 346:
> 
> Make sure registered Chrome shortcuts cannot be automatically assigned
> (overwritten) by extension developers. Media keys are an exception here.

Done.

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

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:48: target =
target_list.erase(target);
On 2013/11/18 11:16:14, Finnur wrote:
> Um, how does this work? Don't you need to advance |target| before erasing?

The return value is an iterator pointing to the new location of the element that
followed the last element erased by the function call. This is the container end
if the operation erased the last element in the sequence.

Reference: http://www.cplusplus.com/reference/list/list/erase/

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:51: }
On 2013/11/18 11:16:14, Finnur wrote:
> Nit: Both of these are single-line so no braces needed (but address comment
> above first).

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:63: break;
On 2013/11/18 11:16:14, Finnur wrote:
> I think this should be outside the if clause that ends on line 64. If we found
> the right command name we can quit, it doesn't matter whether we erase all the
> targets, we won't find the same |command_name| in the rest of the map.

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:97: return true;
On 2013/11/18 11:16:14, Finnur wrote:
> nit: Line break above.

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.h:90: bool
NotifyEventTargetsByAccelerator(const ui::Accelerator& accelerator);
On 2013/11/18 11:16:14, Finnur wrote:
> You can probably skip the 'ByAccelerator' from the name.
> Also, in the comment: s/accelerator/|accelerator|/ to indicate you are talking
> about a param to the function.

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_win.cc (right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_win.cc:80: }
On 2013/11/18 11:16:14, Finnur wrote:
> I presume we'll need this in other platform-specific code?

Done. Have added to the linux implementation.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:72: //
type != named command, so we need to dispatch this event directly.
On 2013/11/18 11:16:14, Finnur wrote:
> nit: Capitalize 'type'.

Done.

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:67:
continue;   // We have register it to the focus manager.
On 2013/11/18 11:16:14, Finnur wrote:
> Comment-nit: It is already registered with the focus manager.

Done.

[Finnur, 2013-11-18 14:14:53]

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
I think this is the one remaining issue before you can check this in (apart from
the DCHECK). There's still some confusion on this issue; might be with me -- but
we need to get to the bottom of this. :)

How did you figure out that KeyDown+KeyUp of Ctrl was enough to get the loop to
stop processing? I was hoping to see where that decision is made (to stop on
KeyUp) but I didn't see it from the links. Unfortunately, I don't have the time
right now to patch in your CL and start debugging, as I'm a few days behind on
the schedule I promised for another task, so I'll need to try to understand this
based on the information you give me.

One thing that I don't understand is that the code below does not send
KeyDown+KeyUp of Ctrl, yet it works better than the helper function provided in
the util class. Why is that?

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

https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.cc:48: target =
target_list.erase(target);
Oh, I missed the assignment there.

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

https://codereview.chromium.org/64273008/diff/700001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:215: // Media Keys are
allowed used by named command only.
s/used/for use/

https://codereview.chromium.org/64273008/diff/700001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:216:
DCHECK(IsMediaKey(accelerator) &&
That can't be right...
What happens when you add Ctrl+Shift+0?

[zhchbin, 2013-11-18 14:54:43]

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/extension_commands_global_registry_apitest.cc
(right):

https://codereview.chromium.org/64273008/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_commands_global_registry_apitest.cc:97: //
TODO(zhchbin): Clear up the comment.
Em... first of all, sorry for my poor English that couldn't help you understand
what's happening.

I had tried my best to explain the problem in the comment of the code. Let me
try to explain again.

1. |SendKeyPressSync| is a wrapper of |SendKeyPressToWindowSync|.
2. |SendKeyPressToWindowSync| is an synchronized function, it will wait the
result whether the specified window has receive the keys. (If timeout happen,
the test case will fail.)
3. On windows, the media keys has been registered by our API, so the
InputDispatcher didn't get notified when only media keys are sent.
InputDispatcher is using Keyboard Hook to listen for a keyboard event
(WM_KEYUP). So adding a irrelevant keyup/down, like what I have shown you, can
help in this situation. Also take a look again my comments: 

A single key: "MediaNextTrack" down -> "MediaNextTrack" up (for example).

Key Combination: Sending "Ctrl+Shift+1", we will send "Ctrl" down -> "Shift"
down -> "1" down, (here the hot key is activated.), then 1 "up" (here
InputDispatcher will receive an key up event, then the message loop in
|SendKeyPressToWindowSync| could stop running normally) -> "Shift" up -> "Ctrl"
up.

4. Last question: Because the bellow code just send the keys out and ignore who
will receive the keys. We didn't wait for any result.

Maybe you can read the links I have gave first of all. Anyway, deal with this
this issue only when you are available for it. I'm OK all the times. Thanks! :)

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

https://codereview.chromium.org/64273008/diff/700001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:215: // Media Keys are
allowed used by named command only.
On 2013/11/18 14:14:54, Finnur wrote:
> s/used/for use/

Done. Change to: "Media Keys are allowed to be used by named command only.",
which I think is more accurate.

https://codereview.chromium.org/64273008/diff/700001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:216:
DCHECK(IsMediaKey(accelerator) &&
On 2013/11/18 14:14:54, Finnur wrote:
> That can't be right...
> What happens when you add Ctrl+Shift+0?

I beat my head again....

[zhchbin, 2013-11-18 14:59:43]

I will "gclient sync" the code as soon as I can.

[Finnur, 2013-11-19 13:48:09]

OK, I think I have grok'ed the problem now. 

What happens if you use SendKeyPressImpl but specify NULL for the |task|
parameter? Doesn't that take the InputDispatcher out of the equation and get rid
of the timeout?

[zhchbin, 2013-11-19 14:02:03]

On 2013/11/19 13:48:09, Finnur wrote:
> OK, I think I have grok'ed the problem now. 
> 
> What happens if you use SendKeyPressImpl but specify NULL for the |task|
> parameter? Doesn't that take the InputDispatcher out of the equation and get
rid
> of the timeout?

Hm... SendKeyPressImpl is in the ui_controls::internal namespace, you mean that
we call it directly?

[Finnur, 2013-11-19 14:27:47]

Um, no. Probably easier to temporarily modify (purely for testing purposes) the
relevant SendKeyPressFoo function (SendKeyPressToWindowSync?) and have it
specify no task (last param in the call to SendKeyPressNotifyWhenDone).

We can't check that modification in, but if that works for this particular test,
then we know what we need to do (next step).

[zhchbin, 2013-11-19 14:54:22]

On 2013/11/19 14:27:47, Finnur wrote:
> Um, no. Probably easier to temporarily modify (purely for testing purposes)
the
> relevant SendKeyPressFoo function (SendKeyPressToWindowSync?) and have it
> specify no task (last param in the call to SendKeyPressNotifyWhenDone).
> 
> We can't check that modification in, but if that works for this particular
test,
> then we know what we need to do (next step).

A good news, thanks for the reminding that the |task| can be null -> I try to
find out when it can be null -> I find out ui::SendKeyPress can work for us.

Now it's updated. PTAL. :)

[Finnur, 2013-11-19 15:29:47]

Excellent! LGTM, with one nit.

https://codereview.chromium.org/64273008/diff/1070001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/1070001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:219: command_name !=
manifest_values::kScriptBadgeCommandEvent);
nit: Probably preferable to do
DCHECK(!IsMediaKey(...) || (...what the DCHECK does now...))
Then the if statement around it isn't needed.

[zhchbin, 2013-11-19 15:49:10]

@finnur, thanks for the review!
@sky, ping for an owner approve: chrome/browser/ui/*.

Due to the change:
https://codereview.chromium.org/64273008/patch/1280001/1290007, this CL need to
include extension_keybinding_registry_*.cc under chrome/browser/ui/*.

https://codereview.chromium.org/64273008/diff/1070001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/64273008/diff/1070001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:219: command_name !=
manifest_values::kScriptBadgeCommandEvent);
On 2013/11/19 15:29:47, Finnur wrote:
> nit: Probably preferable to do
> DCHECK(!IsMediaKey(...) || (...what the DCHECK does now...))
> Then the if statement around it isn't needed.

Done.

[sky, 2013-11-19 16:54:46]

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:53: if
(first_target == targets->second.end())
Is there a reason to allow an empty TargetList?

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:80:
DCHECK(++first_target == targets->second.end());
Having side effects like this in a DCHECK is error prone. I realize it makes no
difference here, but would prefer side effect free DCHECK.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:107:
DCHECK(event_targets_[iter->second.accelerator()].size() == 1);
DCHECK_EQ where appropriate.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:136:
DCHECK(event_targets_[accelerator].size() == 1);  // Ditto.
Ditto ~15 lines away from a comment is not clear.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:149:
DCHECK(event_targets_[accelerator].size() == 1);  // Ditto.
Even more so here.

[zhchbin, 2013-11-20 02:05:40]

Ping Finnur for a double check about cocoa's change.

@sky, I didn't have MAC development environment so I have to rely on Finnur. :)

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:53: if
(first_target == targets->second.end())
On 2013/11/19 16:54:47, sky wrote:
> Is there a reason to allow an empty TargetList?

Done.

Ping Finnur here, I have change it to DCHECK, do you mind have a double check?
The same as below.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:80:
DCHECK(++first_target == targets->second.end());
On 2013/11/19 16:54:47, sky wrote:
> Having side effects like this in a DCHECK is error prone. I realize it makes
no
> difference here, but would prefer side effect free DCHECK.

Done.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:107:
DCHECK(event_targets_[iter->second.accelerator()].size() == 1);
On 2013/11/19 16:54:47, sky wrote:
> DCHECK_EQ where appropriate.

Done.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:136:
DCHECK(event_targets_[accelerator].size() == 1);  // Ditto.
On 2013/11/19 16:54:47, sky wrote:
> Ditto ~15 lines away from a comment is not clear.

Done.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:149:
DCHECK(event_targets_[accelerator].size() == 1);  // Ditto.
On 2013/11/19 16:54:47, sky wrote:
> Even more so here.

Done.

[Finnur, 2013-11-20 13:26:38]

Agree with the changes you made to the .mm.

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/1280001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:53: if
(first_target == targets->second.end())
No, there is no reason to allow it. 

On 2013/11/20 02:05:40, zhchbin wrote:
> On 2013/11/19 16:54:47, sky wrote:
> > Is there a reason to allow an empty TargetList?
> 
> Done.
> 
> Ping Finnur here, I have change it to DCHECK, do you mind have a double check?
> The same as below.

https://codereview.chromium.org/64273008/diff/1320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/1320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:79:
DCHECK_EQ(targets->second.size(), 1u);
The syntax is:

DCHECK_EQ(expected, actual), not
DCHECK_EQ(actual, expected)

... so this should be:

DCHECK_EQ(1u, targets->second.size());

[zhchbin, 2013-11-20 15:03:02]

https://codereview.chromium.org/64273008/diff/1320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm
(right):

https://codereview.chromium.org/64273008/diff/1320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:79:
DCHECK_EQ(targets->second.size(), 1u);
On 2013/11/20 13:26:39, Finnur wrote:
> The syntax is:
> 
> DCHECK_EQ(expected, actual), not
> DCHECK_EQ(actual, expected)
> 
> ... so this should be:
> 
> DCHECK_EQ(1u, targets->second.size());

Done. I will remember next time.

[zhchbin, 2013-11-21 13:48:06]

Ping @sky again. PTAL if you are available.

[sky, 2013-11-21 16:46:59]

LGTM

[commit-bot: I haz the power, 2013-11-21 16:47:38]

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

[commit-bot: I haz the power, 2013-11-21 19:06:51]

Failed to apply patch for
chrome/browser/extensions/extension_keybinding_registry.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_keybinding_registry.cc
  Hunk #1 FAILED at 7.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_keybinding_registry.cc.rej

Patch:       chrome/browser/extensions/extension_keybinding_registry.cc
Index: chrome/browser/extensions/extension_keybinding_registry.cc
diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc
b/chrome/browser/extensions/extension_keybinding_registry.cc
index
dc675658e4eab5db9db1374d32f0292674acb75e..abc1bf84c0a60332aa003df8f6069ae42834f686
100644
--- a/chrome/browser/extensions/extension_keybinding_registry.cc
+++ b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -7,6 +7,7 @@
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
@@ -37,22 +38,27 @@ ExtensionKeybindingRegistry::~ExtensionKeybindingRegistry()
{
 void ExtensionKeybindingRegistry::RemoveExtensionKeybinding(
     const Extension* extension,
     const std::string& command_name) {
-  EventTargets::iterator iter = event_targets_.begin();
-  while (iter != event_targets_.end()) {
-    if (iter->second.first != extension->id() ||
-        (!command_name.empty() && (iter->second.second != command_name))) {
-      ++iter;
-      continue;  // Not the extension or command we asked for.
+  EventTargets::iterator it = event_targets_.begin();
+  while (it != event_targets_.end()) {
+    TargetList& target_list = it->second;
+    TargetList::iterator target = target_list.begin();
+    while (target != target_list.end()) {
+      if (target->first == extension->id() &&
+          (command_name.empty() || command_name == target->second))
+        target = target_list.erase(target);
+      else
+        target++;
     }
 
-    // Let each platform-specific implementation get a chance to clean up.
-    RemoveExtensionKeybindingImpl(iter->first, command_name);
-
-    EventTargets::iterator old = iter++;
-    event_targets_.erase(old);
+    EventTargets::iterator old = it++;
+    if (target_list.empty()) {
+      // Let each platform-specific implementation get a chance to clean up.
+      RemoveExtensionKeybindingImpl(old->first, command_name);
+      event_targets_.erase(old);
+    }
 
-    // If a specific command_name was requested, it has now been deleted so
-    // no further work is required.
+    // If a specific command_name was requested, it has now been deleted so no
+    // further work is required.
     if (!command_name.empty())
       break;
   }
@@ -78,6 +84,19 @@ bool ExtensionKeybindingRegistry::ShouldIgnoreCommand(
          command == manifest_values::kScriptBadgeCommandEvent;
 }
 
+bool ExtensionKeybindingRegistry::NotifyEventTargets(
+    const ui::Accelerator& accelerator) {
+  EventTargets::iterator targets = event_targets_.find(accelerator);
+  if (targets == event_targets_.end() || targets->second.empty())
+    return false;
+
+  for (TargetList::const_iterator it = targets->second.begin();
+       it != targets->second.end(); it++)
+    CommandExecuted(it->first, it->second);
+
+  return true;
+}
+
 void ExtensionKeybindingRegistry::CommandExecuted(
     const std::string& extension_id, const std::string& command) {
   ExtensionService* service =

[commit-bot: I haz the power, 2013-11-22 03:12:55]

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

[commit-bot: I haz the power, 2013-11-22 04:56:51]

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

[commit-bot: I haz the power, 2013-11-22 06:15:54]

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

[commit-bot: I haz the power, 2013-11-22 07:20:04]

Change committed as 236707