Implement first part of supporting global extension commands.

Implement first part of supporting global extension commands.

This first part simply reads the optional "global" flag from the manifest and registers the shortcut as global if it is set to true. The intent is to let developers register Ctrl+Shift+[0..9] (and nothing else) as global shortcuts but let users override that -- delete the shortcut, change it to whatever other combination (not restricted to Ctrl+Shift+[0..9] or even make it non-global. There's no UI at the moment to do so, but that is a separate CL that is almost ready.

This CL implements this functionality for Windows and stubs out the other platforms. The GTK stub has been fleshed out in parallel, but that's also another CL.

BUG=302437
R=erg@chromium.org, sky@chromium.org, thakis@chromium.org, yoz@chromium.org

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

[zhchbin, 2013-09-20 07:49:23]

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener.cc (right):

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.cc:30: }
On linux I need to know which accelerator is registering or unregistering, so
that we can listen global hotkey via "XGrabKey". I think it's same on MAC, via
"RegisterEventHotKey".

AFAICS, On linux and mac we didn't have an easy way that can listen for all
keyboard event.

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener.h (right):

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.h:9: #include
"ui/base/keycodes/keyboard_codes.h"
This file has been moved from base to events. s/base/events.

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.h:34: virtual void
StopHooking() = 0;
Hooking seems a little window-specific. How about StartListening &
StopListening.

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

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_win.cc:11: #include
"ui/base/keycodes/keyboard_code_conversion_win.h"
This file has been moved from base to events. s/base/events.

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_win.cc:45: keyboard_hook_ =
SetWindowsHookEx(WH_KEYBOARD_LL, &KeyboardHook, NULL, 0L);
Do you mind explaining the reason that you chose using hook procedure that
monitors low-level keyboard input events, instead of "RegisterHotKey" [0]?

On of my concerns is that it's too powerful. For example, suppose an native
application (Not Chrome) is using "RegisterHotKey" to register "Ctrl+Shift+Z"
and the API return true. In the meantime, an extension suggesting using global
"Ctrl+Shift+Z" is installed, suppose the command can register successfully. Now
what's happening? The hotkey of native application stops working anymore because
we "stolen" the keyboard event currently. Also thinking about a developer who is
developing a native application that using "RegisterHotKey" to enable his hotkey
"Ctrl+Shift+Z" functionality, the API return true, but why it didn't work?

By the way using hook procedure will make enabling global media keys will be
easier.

[0]:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms646309(v=vs.85).aspx

[Finnur, 2013-09-20 13:01:11]

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener.cc (right):

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.cc:30: }
I'm not sure I understand this comment. You have both the |accelerator| and
|observer| here. What am I missing?

On 2013/09/20 07:49:23, zhchbin wrote:
> On linux I need to know which accelerator is registering or unregistering, so
> that we can listen global hotkey via "XGrabKey". I think it's same on MAC, via
> "RegisterEventHotKey".
> 
> AFAICS, On linux and mac we didn't have an easy way that can listen for all
> keyboard event.

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener.h (right):

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.h:34: virtual void
StopHooking() = 0;
Good point.

On 2013/09/20 07:49:23, zhchbin wrote:
> Hooking seems a little window-specific.

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

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_win.cc:45: keyboard_hook_ =
SetWindowsHookEx(WH_KEYBOARD_LL, &KeyboardHook, NULL, 0L);
I haven't fully made up my mind yet (and not fully thought it through either). 

This approach is dead simple and fits well in the current implementation. 

Using RegisterHotKey, on the other hand, requires a window and a messageloop,
which probably means shunting the work to another thread unless we want to have
the handler logic live outside these classes (capture WM_HOTKEY in the main
window handler code). :/

There's a clear benefit (with RegisterHotkey) to getting an error if another app
has registered the same hotkey. But I need to figure out if I can do this
without too much surgery...

On 2013/09/20 07:49:23, zhchbin wrote:
> Do you mind explaining the reason that you chose using hook procedure that
> monitors low-level keyboard input events, instead of "RegisterHotKey" [0]?
> 
> On of my concerns is that it's too powerful. For example, suppose an native
> application (Not Chrome) is using "RegisterHotKey" to register "Ctrl+Shift+Z"
> and the API return true. In the meantime, an extension suggesting using global
> "Ctrl+Shift+Z" is installed, suppose the command can register successfully.
Now
> what's happening? The hotkey of native application stops working anymore
because
> we "stolen" the keyboard event currently. Also thinking about a developer who
is
> developing a native application that using "RegisterHotKey" to enable his
hotkey
> "Ctrl+Shift+Z" functionality, the API return true, but why it didn't work?
> 
> By the way using hook procedure will make enabling global media keys will be
> easier.
> 
> [0]:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms646309%28v=vs.85%29...

[Finnur, 2013-09-20 13:15:56]

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

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_win.cc:45: keyboard_hook_ =
SetWindowsHookEx(WH_KEYBOARD_LL, &KeyboardHook, NULL, 0L);
Well, I take that back: a window isn't required. But we'll still need a separate
thread as we don't want to run the messageloop on the UI thread.

On 2013/09/20 13:01:12, Finnur wrote:
> I haven't fully made up my mind yet (and not fully thought it through either).

> 
> This approach is dead simple and fits well in the current implementation. 
> 
> Using RegisterHotKey, on the other hand, requires a window and a messageloop,
> which probably means shunting the work to another thread unless we want to
have
> the handler logic live outside these classes (capture WM_HOTKEY in the main
> window handler code). :/
> 
> There's a clear benefit (with RegisterHotkey) to getting an error if another
app
> has registered the same hotkey. But I need to figure out if I can do this
> without too much surgery...
> 
> On 2013/09/20 07:49:23, zhchbin wrote:
> > Do you mind explaining the reason that you chose using hook procedure that
> > monitors low-level keyboard input events, instead of "RegisterHotKey" [0]?
> > 
> > On of my concerns is that it's too powerful. For example, suppose an native
> > application (Not Chrome) is using "RegisterHotKey" to register
"Ctrl+Shift+Z"
> > and the API return true. In the meantime, an extension suggesting using
global
> > "Ctrl+Shift+Z" is installed, suppose the command can register successfully.
> Now
> > what's happening? The hotkey of native application stops working anymore
> because
> > we "stolen" the keyboard event currently. Also thinking about a developer
who
> is
> > developing a native application that using "RegisterHotKey" to enable his
> hotkey
> > "Ctrl+Shift+Z" functionality, the API return true, but why it didn't work?
> > 
> > By the way using hook procedure will make enabling global media keys will be
> > easier.
> > 
> > [0]:
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms646309%2528v=vs.85%...
>

[zhchbin, 2013-09-21 02:07:50]

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener.cc (right):

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.cc:30: }
I mean to said that this function should add some logic that is implemented
platform specified. Something in my mind:

GlobalShortcutListener::RegisterAccelerator(
    const ui::Accelerator& accelerator, Observer* observer) {
  if (RegisterAcceleratorImpl(accelerator)) {
    ...  // Cross platform code.
  }
}

Then on windows:

GlobalShortcutListenerWin::RegisterAcceleratorImpl(
  const ui::Accelerator& accelerator) {
  // Register using "RegisterHotkey"
}

On Linux

GlobalShortcutListenerLinux::RegisterAcceleratorImpl(
  const ui::Accelerator& accelerator) {
  // Register using "XGrabKey"
}

On Mac

GlobalShortcutListenerCocoa::RegisterAcceleratorImpl(
  const ui::Accelerator& accelerator) {
  // Register using "RegisterEventHotKey"
}

Just the picture about how to implement it on different platforms.


On 2013/09/20 13:01:12, Finnur wrote:
> I'm not sure I understand this comment. You have both the |accelerator| and
> |observer| here. What am I missing?
> 
> On 2013/09/20 07:49:23, zhchbin wrote:
> > On linux I need to know which accelerator is registering or unregistering,
so
> > that we can listen global hotkey via "XGrabKey". I think it's same on MAC,
via
> > "RegisterEventHotKey".
> > 
> > AFAICS, On linux and mac we didn't have an easy way that can listen for all
> > keyboard event.
>

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

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_win.cc:45: keyboard_hook_ =
SetWindowsHookEx(WH_KEYBOARD_LL, &KeyboardHook, NULL, 0L);
We can use base::win::MessageWindow to subscribe to WM_HOTKEY messages here. Or
gfx::SingletonHwnd. It seems to fit our requirement here.

On 2013/09/20 13:15:56, Finnur wrote:
> Well, I take that back: a window isn't required. But we'll still need a
separate
> thread as we don't want to run the messageloop on the UI thread.
> 
> On 2013/09/20 13:01:12, Finnur wrote:
> > I haven't fully made up my mind yet (and not fully thought it through
either).
> 
> > 
> > This approach is dead simple and fits well in the current implementation. 
> > 
> > Using RegisterHotKey, on the other hand, requires a window and a
messageloop,
> > which probably means shunting the work to another thread unless we want to
> have
> > the handler logic live outside these classes (capture WM_HOTKEY in the main
> > window handler code). :/
> > 
> > There's a clear benefit (with RegisterHotkey) to getting an error if another
> app
> > has registered the same hotkey. But I need to figure out if I can do this
> > without too much surgery...
> > 
> > On 2013/09/20 07:49:23, zhchbin wrote:
> > > Do you mind explaining the reason that you chose using hook procedure that
> > > monitors low-level keyboard input events, instead of "RegisterHotKey" [0]?
> > > 
> > > On of my concerns is that it's too powerful. For example, suppose an
native
> > > application (Not Chrome) is using "RegisterHotKey" to register
> "Ctrl+Shift+Z"
> > > and the API return true. In the meantime, an extension suggesting using
> global
> > > "Ctrl+Shift+Z" is installed, suppose the command can register
successfully.
> > Now
> > > what's happening? The hotkey of native application stops working anymore
> > because
> > > we "stolen" the keyboard event currently. Also thinking about a developer
> who
> > is
> > > developing a native application that using "RegisterHotKey" to enable his
> > hotkey
> > > "Ctrl+Shift+Z" functionality, the API return true, but why it didn't work?
> > > 
> > > By the way using hook procedure will make enabling global media keys will
be
> > > easier.
> > > 
> > > [0]:
> > >
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms646309%252528v=vs.8...
> > 
>

[Finnur, 2013-09-21 12:23:50]

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener.cc (right):

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener.cc:30: }
I see. I'll take a look on Monday.

On 2013/09/21 02:07:50, zhchbin wrote:
> I mean to said that this function should add some logic that is implemented
> platform specified. Something in my mind:
> 
> GlobalShortcutListener::RegisterAccelerator(
>     const ui::Accelerator& accelerator, Observer* observer) {
>   if (RegisterAcceleratorImpl(accelerator)) {
>     ...  // Cross platform code.
>   }
> }
> 
> Then on windows:
> 
> GlobalShortcutListenerWin::RegisterAcceleratorImpl(
>   const ui::Accelerator& accelerator) {
>   // Register using "RegisterHotkey"
> }
> 
> On Linux
> 
> GlobalShortcutListenerLinux::RegisterAcceleratorImpl(
>   const ui::Accelerator& accelerator) {
>   // Register using "XGrabKey"
> }
> 
> On Mac
> 
> GlobalShortcutListenerCocoa::RegisterAcceleratorImpl(
>   const ui::Accelerator& accelerator) {
>   // Register using "RegisterEventHotKey"
> }
> 
> Just the picture about how to implement it on different platforms.
> 
> 
> On 2013/09/20 13:01:12, Finnur wrote:
> > I'm not sure I understand this comment. You have both the |accelerator| and
> > |observer| here. What am I missing?
> > 
> > On 2013/09/20 07:49:23, zhchbin wrote:
> > > On linux I need to know which accelerator is registering or unregistering,
> so
> > > that we can listen global hotkey via "XGrabKey". I think it's same on MAC,
> via
> > > "RegisterEventHotKey".
> > > 
> > > AFAICS, On linux and mac we didn't have an easy way that can listen for
all
> > > keyboard event.
> > 
>

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

https://codereview.chromium.org/23812010/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_win.cc:45: keyboard_hook_ =
SetWindowsHookEx(WH_KEYBOARD_LL, &KeyboardHook, NULL, 0L);
Yes, I came to the conclusion after responding to you yesterday that
RegisterHotkey is probably the way to go. SingletonHwnd is what we need, I
believe, and I got it working on Friday just before leaving. Need to polish it
up on Monday.

[Finnur, 2013-09-23 16:29:58]

See what you think of this last version.

[zhchbin, 2013-09-24 02:41:22]

Wonderful!

The problem from mek@ in the API proposal:
"If there are multiple chrome profiles, extensions in each profile are allowed
to register the same shortcuts. How will you deal with this with global
shortcuts?"

I tested it but only worked on the first profile. Should we make sure that it
works as non-global command? From my point of view, we can only allow the first
one.

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
We should handle *global* named commands only. Currently we will deal with
non-global command and traverse the event_targets_.

https://codereview.chromium.org/23812010/diff/61001/chrome/common/extensions/...
File chrome/common/extensions/extension_manifest_constants.cc (right):

https://codereview.chromium.org/23812010/diff/61001/chrome/common/extensions/...
chrome/common/extensions/extension_manifest_constants.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
I think you may commit this file by accident.

[Finnur, 2013-09-24 10:53:23]

> The problem from mek@ in the API proposal:
> "If there are multiple chrome profiles, extensions in each profile are allowed
to
> register the same shortcuts. How will you deal with this with global
shortcuts?"

> I tested it but only worked on the first profile. Should we make sure that it
> works as non-global command? From my point of view, we can only allow the
first
> one.

I'll take a look at the API proposal, but I'm not too concerned about this. The
global shortcut should only work in one profile and with the implementation I
have envisioned you won't be able to configure the same shortcut in two
profiles.

There are two ways to set a shortcut:
1) Automatically assigned at install time using manifest suggestion. This will
only work if the shortcut is free already (if another profile/app has taken the
shortcut, this shortcut registration does not succeed). Now that we are using
RegisterHotkey we can get this signal.

2) User sets the shortcut through the UI. The UI is a bit confusing at the
moment because I haven't made an effort to make it clear that one shortcut is
local and the other global, but when this is done, there are two options.
a) The user gets to set the shortcut, but it takes the shortcut away from the
other profile.
OR
b) The user is barred from setting the shortcut because another profile is using
it. This is probably the safer option, in case the user does not have full
control of both profiles (think password protected profiles, if we ever
implement that).

It certainly doesn't make sense to me to have the same shortcut bound to
separate actions in two profiles at the same time.

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
I don't see the code here as being incorrect. The event targets are populated by
AddExtensionKeybinding where we do ask for only named commands of GLOBAL scope
(see line 73 above). Therefore only global targets should be in this list when
we start removing... Unless am I missing something?

https://codereview.chromium.org/23812010/diff/61001/chrome/common/extensions/...
File chrome/common/extensions/extension_manifest_constants.cc (right):

https://codereview.chromium.org/23812010/diff/61001/chrome/common/extensions/...
chrome/common/extensions/extension_manifest_constants.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Good catch.

[zhchbin, 2013-09-24 11:25:26]

> b) The user is barred from setting the shortcut because another profile is
> using it. This is probably the safer option, in case the user does not have
> full control of both profiles (think password protected profiles, if we ever
> implement that).

Yes! I also think this will be better AFAICS.

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
On 2013/09/24 10:53:23, Finnur wrote:
> I don't see the code here as being incorrect. The event targets are populated
by
> AddExtensionKeybinding where we do ask for only named commands of GLOBAL scope
> (see line 73 above). Therefore only global targets should be in this list when
> we start removing... Unless am I missing something?

ExtensionKeybindingRegistry is observing
chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED, when a user remove a
shortcut(global/non-global) from the ui, these lines of code will be called. The
code here is just check whether the command is named. We should check it whether
is global and prevent the unnecessary loop. :).

[Finnur, 2013-09-25 13:11:40]

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
I see. I am currently investigating a crash (not related to commands) but I plan
to take a look at this after that.

Do you want to branch off of this version and take a stab at implementing the
GlobalKeyboardShortcutLinux class in the meantime, to see if this modularization
works for that?


On 2013/09/24 11:25:27, zhchbin wrote:
> On 2013/09/24 10:53:23, Finnur wrote:
> > I don't see the code here as being incorrect. The event targets are
populated
> by
> > AddExtensionKeybinding where we do ask for only named commands of GLOBAL
scope
> > (see line 73 above). Therefore only global targets should be in this list
when
> > we start removing... Unless am I missing something?
> 
> ExtensionKeybindingRegistry is observing
> chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED, when a user remove a
> shortcut(global/non-global) from the ui, these lines of code will be called.
The
> code here is just check whether the command is named. We should check it
whether
> is global and prevent the unnecessary loop. :).

[Finnur, 2013-09-25 13:12:38]

Huh, I could have sworn that I added Boris to this list. Guess not. Hmmm
(fixed).

[zhchbin, 2013-09-25 13:27:13]

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
> Do you want to branch off of this version and take a stab at implementing the
> GlobalKeyboardShortcutLinux class in the meantime, to see if this
modularization
> works for that?

I would begin as soon as I can! So many homework for me currently. :) I estimate
it could be done within one week, based on your awesome work.

[Finnur, 2013-09-26 13:53:07]

Updated just a bit. Now working on providing a stub implementation for Linux
(just the class, not the implementation).

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
> We should handle *global* named commands only.

OK, found some time to think about this some more.

I presume you are suggesting we use GetNamedCommands, like we do in
AddExtensionKeybinding? The problem with that is that by the time we get this
event the command has already been removed from the map in CommandService, so
GetNamedCommands no longer includes the command we are about to unregister.

Therefore we look it up by the information we have about it. Yes, it is slightly
more work (in a non-performance critical code handling a rare event) but there
is an additional benefit in that we don't try to delete unless we managed to
register the shortcut with AddExtensionKeybinding.

However, it was good that you mentioned this, because it made me realize we
don't need to continue looping after we've deleted the command in the case where
the caller specified a certain command to delete (as opposed to all).

[Finnur, 2013-09-26 14:34:15]

OK, Linux stub implementation provided (global_shortcut_listener_linux.cc and
.h) for your perusal. I'm hoping if it works on your Linux box it'll work in
ChromeOS in general, but we'll cross that bridge when we get to it.

[zhchbin, 2013-09-26 14:44:58]

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
On 2013/09/26 13:53:08, Finnur wrote:
> > We should handle *global* named commands only.
> 
> OK, found some time to think about this some more.
> 
> I presume you are suggesting we use GetNamedCommands, like we do in
> AddExtensionKeybinding? The problem with that is that by the time we get this
> event the command has already been removed from the map in CommandService, so
> GetNamedCommands no longer includes the command we are about to unregister.
> 
> Therefore we look it up by the information we have about it. Yes, it is
slightly
> more work (in a non-performance critical code handling a rare event) but there
> is an additional benefit in that we don't try to delete unless we managed to
> register the shortcut with AddExtensionKeybinding.
> 
> However, it was good that you mentioned this, because it made me realize we
> don't need to continue looping after we've deleted the command in the case
where
> the caller specified a certain command to delete (as opposed to all).

Hm... I mean to said that we can check whether the given command_name is global
before we do the loop. AFAICS, we can gain this info from CommandService, it
seems that we didn't remove any command from the command map.

Actually, what's in my mind is adding a function like: IsCommandGloabl in the
CommandService, 

bool CommandService::IsCommandGlobal(const Extension* extension,
                                     const std::string command_name) const {
  const extensions::CommandMap* commands =
      CommandsInfo::GetNamedCommands(extension);
  if (!commands)
    return false;

  extensions::CommandMap::const_iterator iter = commands->find(command_name);
  if (iter != commands->end()) {
    return iter->second.global();
  }

  return false;
}

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:87: // TODO(finnur): find a
better place for this.
How about make the ExtensionCommandsGlobalRegistryViews cross platform,
something like ExtensionCommandsGlobalRegistry and put it on the constructor of
CommandService?

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.h
(right):

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.h:25:
// ExtensionCommandsGlobalRegistryViews is a class that handles the
Does this need to be views-specific? I'm working on linux this afternoon and
find the code here can cross platform.

[zhchbin, 2013-09-26 14:54:04]

On 2013/09/26 14:34:15, Finnur wrote:
> OK, Linux stub implementation provided (global_shortcut_listener_linux.cc and
> .h) for your perusal. I'm hoping if it works on your Linux box it'll work in
> ChromeOS in general, but we'll cross that bridge when we get to it.

Thanks. I'm working on it already this afternoon. In my implementation, it needs
gtk support, which is annoying me. We are using _linux, this kind of code will
be a build fail on linux_aura I think. Anyway, I would like to make it work on
GTK+ first of all!

[Finnur, 2013-09-26 15:00:59]

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
No, that's what I'm saying: The Command in question has already been deleted
from CommandService by the time we get this event so that function would always
return false. 

I actually implemented something similar to what you are describing and when it
didn't work I started investigating why. Inspecting the code shows that we
delete the command before RemoveExtensionKeybinding runs.

Now, it is possible to do some surgery to change that, but I don't see enough
benefit to warrant it. Removing a command is a very rare event and the map we
are iterating will contain very few entries (0 for most people). Possibly dozens
of entries for heavy users.

I don't think this is worth pursuing.

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:87: // TODO(finnur): find a
better place for this.
Haven't revisited this TODO, but now is a good time to do so.

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.h
(right):

https://codereview.chromium.org/23812010/diff/76001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.h:25:
// ExtensionCommandsGlobalRegistryViews is a class that handles the
I think you may be right. I'll try to move it to a platform indendant location.

[zhchbin, 2013-09-26 15:13:05]

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
File
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc
(right):

https://codereview.chromium.org/23812010/diff/61001/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/extension_commands_global_registry_views.cc:100:
// This object only handles named commands, not browser/page actions.
On 2013/09/26 15:00:59, Finnur wrote:
> No, that's what I'm saying: The Command in question has already been deleted
> from CommandService by the time we get this event so that function would
always
> return false. 
> 
> I actually implemented something similar to what you are describing and when
it
> didn't work I started investigating why. Inspecting the code shows that we
> delete the command before RemoveExtensionKeybinding runs.
> 
> Now, it is possible to do some surgery to change that, but I don't see enough
> benefit to warrant it. Removing a command is a very rare event and the map we
> are iterating will contain very few entries (0 for most people). Possibly
dozens
> of entries for heavy users.
> 
> I don't think this is worth pursuing.

:) OK let's stop talking about this and go on. 

(The code works fine in my side, but as you said, "Removing a command is a very
rare event", so it's meaningless currently. Maybe I will revisit this problem
when it is worth pursuing.)

Thanks for your kindly explanation.

[Finnur, 2013-09-26 15:21:42]

ExtensionCommandsGlobalRegistry no longer is tied to Views (see latest CL) and
seems to work fine in a cross-platform location. I think we're getting close.

[zhchbin, 2013-09-26 15:44:33]

On 2013/09/26 15:21:42, Finnur wrote:
> ExtensionCommandsGlobalRegistry no longer is tied to Views (see latest CL) and
> seems to work fine in a cross-platform location. I think we're getting close.

Great! That's enough for me now! Is your next step: add mac & chromeos stub and
go through formal review now? By the way, should we put this feature under a
flag?

[Finnur, 2013-09-26 15:51:33]

Mac stub: yes. In fact I already uploaded it.

ChromeOS stub: I was planning on adding it if we needed it. But ChromeOS is
actually a special-case, because the regular shortcuts actually work as global
shortcuts. Not sure how to tackle that.

Flag: Yes, that's the next thing I was thinking of doing.

Formal review: Perhaps. There are more things required (like changing the UI to
show which shortcuts are global), but maybe if the thing is behind a flag we can
get away with not completing it in this round.

[Finnur, 2013-09-26 15:56:57]

Re: ChromeOS: Oh, right... You mentioned you need GTK, so it won't work on
ChromeOS. I'll change the stub to be GTK instead.

[zhchbin, 2013-09-27 09:30:06]

On 2013/09/26 15:56:57, Finnur wrote:
> Re: ChromeOS: Oh, right... You mentioned you need GTK, so it won't work on
> ChromeOS. I'll change the stub to be GTK instead.

I have uploaded gtk part: https://codereview.chromium.org/25007002. Change gtk
stub only.

[Finnur, 2013-09-27 17:01:55]

Elliot,

This is not a formal review request (at least not yet) :)

But I have an architecture question for you.

We are extending the Commands API to support global shortcuts (this changelist).
I implemented this for Windows and added empty stubs for Mac, GTK and ChromeOS
(to my changelist). In parallel (another changelist) Chaobin Zhang took the GTK
stub and implemented it, seen here:
https://codereview.chromium.org/23812010/diff/137001/chrome/browser/extension...
https://codereview.chromium.org/23812010/diff/137001/chrome/browser/extension...

I'm thinking of how to split the stub files up correctly so that each toolkit/os
flavor is covered but there's no duplicate code.

Specific questions related to Chaobin's code:

The stubs were added using suffixes on implementation files, such as _cocoa.cc,
_gtk.cc and _chromeos.cc. That leaves Linux Aura, which currently fails to
compile. Will the XGrabKey approach you see in GTK work on Linux Aura and/or
ChromeOS?

If not, will Linux Aura need an _aura.cc stub (where we no-op everything if
OS_WIN is defined)?

Or am I going about this all wrong? :)

Please advise. :)

[Elliot Glaysher, 2013-09-27 20:27:36]

+piman

I don't actually know if XGrabKey is the appropriate thing to do here in a pure
X11 implementation. Maybe piman@ has opinions here?

The suffix that you'll want to use for x11 code shared between Gtk and aura is
_x11.*

[zhchbin, 2013-09-28 03:33:10]

On 2013/09/27 17:01:55, Finnur wrote:
> Elliot,
> 
> This is not a formal review request (at least not yet) :)
> 
> But I have an architecture question for you.
> 
> We are extending the Commands API to support global shortcuts (this
changelist).
> I implemented this for Windows and added empty stubs for Mac, GTK and ChromeOS
> (to my changelist). In parallel (another changelist) Chaobin Zhang took the
GTK
> stub and implemented it, seen here:
>
https://codereview.chromium.org/23812010/diff/137001/chrome/browser/extension...
>
https://codereview.chromium.org/23812010/diff/137001/chrome/browser/extension...
> 
> I'm thinking of how to split the stub files up correctly so that each
toolkit/os
> flavor is covered but there's no duplicate code.
> 
> Specific questions related to Chaobin's code:
> 
> The stubs were added using suffixes on implementation files, such as
_cocoa.cc,
> _gtk.cc and _chromeos.cc. That leaves Linux Aura, which currently fails to
> compile. Will the XGrabKey approach you see in GTK work on Linux Aura and/or
> ChromeOS?
> 
> If not, will Linux Aura need an _aura.cc stub (where we no-op everything if
> OS_WIN is defined)?
> 
> Or am I going about this all wrong? :)
> 
> Please advise. :)

I see one kind of suffix: _aurax11.*, is it for Linux Aura? (I think the answer
is yes, and I can continue working on investigating how it can be implemented.)
XGrabkey is an API from Xlib, but my current approach needs
gdk_window_add_filter to add a window event filter to the root window, which can
listen for the KeyPress event globally.

A pure X11 implementation will be better from my point of view. But it needs
some time to investigate. Please put it as a TODO for me. So in order to solve
the Linux Aura compile problem, maybe _aurax11.* is a good stub.

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

https://codereview.chromium.org/23812010/diff/137001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:134:
ExtensionSystem::Get(profile_)->extension_service()->extensions();
Because we add
"ExtensionCommandsGlobalRegistry::GetFactoryInstance()->GetForProfile(profile);"
in the CommandService, now we are observing NOTIFICATION_EXTENSION_LOADED, so
this function will be called when running test case:
UserScriptListenerTest.MultiProfile. And extension_service will be NULL during
testing, See
http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/483...
for more info.

[Elliot Glaysher, 2013-09-30 21:26:40]

aurax11 is any aura configuration that runs on X11, including chromeos. You can
also just filter the aurax11 implementation out in the gyp file in the chromeos
case.

Is there a launch bug for this? Has it been through UX review? I'm slightly
scared of grabbing keys in other applications.

Finally, I'm slightly iffy about the approach. There's no usage of XGrabKey() in
GTK (other than a single one-off, limited time use in the drag and drop
implementation to handle hitting escape). There are a few uses in the metacity
window manager; I'm not sure what sort of conflicts need to be handled here...

[Finnur, 2013-10-02 11:38:11]

Sorry, forgot to reply.

The feature bug is here (and it points to the launch bug):
https://code.google.com/p/chromium/issues/detail?id=302437

The conflict with other apps' shortcuts did not come up during review, actually.
People did express concerns about conflicts with other _global_ shotcuts
registered, but since that creates a conflict on registration we shouldn't have
a problem (we simply won't steal that shortcut). The problem of overriding
*non-global* shortcuts (that the app is using) with global shortcuts in Chrome
is something we need to focus on carefully as we won't detect that conflict on
registration.

We are discussing ways of mitigating this, such as some UI letting the user know
the shortcut was sent to Chrome or by restricting the combination to something
that's not as likely to conflict (for example, requiring Alt+Shift as modifiers,
or something). All of the solutions have drawbacks, though, but nothing has been
decided yet.

> Finally, I'm slightly iffy about the approach. There's no usage of XGrabKey()
in GTK 

I wouldn't expect that function to be used much in Chrome today, since we don't
have much of a need for monitoring specific keystrokes like this (usage of
RegisterHotkey is also new for the Windows codebase). But is there another
approach you were thinking of that is more appropriate?

[Elliot Glaysher, 2013-10-02 19:42:22]

On 2013/10/02 11:38:11, Finnur wrote:
> We are discussing ways of mitigating this, such as some UI letting the user
know
> the shortcut was sent to Chrome or by restricting the combination to something
> that's not as likely to conflict (for example, requiring Alt+Shift as
modifiers,
> or something). All of the solutions have drawbacks, though, but nothing has
been
> decided yet.

Are these keyboard shortcuts on by default? I assume they're opt-out instead of
opt-in?

> I wouldn't expect that function to be used much in Chrome today, since we
don't
> have much of a need for monitoring specific keystrokes like this (usage of
> RegisterHotkey is also new for the Windows codebase). But is there another
> approach you were thinking of that is more appropriate?

I don't have any better answers.

If you're going to do a global XGrabKey implementation, you should share it
between desktop aura and gtk, but probably(?) not ash, but I'd double check with
them about it. I would assume that ash has its own system for handling global
key events.

You'll probably be able to reuse the same XGrabKey implementation between
desktop aura and gtk, so use a _x11.* suffix and filter out chromeos. (It's
possible, but unlikely that you won't be able to reuse the XGrabKey
implementation in gtk due to mismatches between XEvents and GdkEvents. If that
happens, I'd urge you to just implement the aura version. There's a chance we'll
get linux_aura out the door in M32, and if not, it will very likely be M33. I'm
not sure how much effort you should really spend on the gtk version.)

[zhchbin, 2013-10-03 00:27:59]

> If you're going to do a global XGrabKey implementation, you should share it
> between desktop aura and gtk, but probably(?) not ash, but I'd double check
with
> them about it. I would assume that ash has its own system for handling global
> key events.
> 
> You'll probably be able to reuse the same XGrabKey implementation between
> desktop aura and gtk, so use a _x11.* suffix and filter out chromeos. (It's
> possible, but unlikely that you won't be able to reuse the XGrabKey
> implementation in gtk due to mismatches between XEvents and GdkEvents. If that
> happens, I'd urge you to just implement the aura version. There's a chance
we'll
> get linux_aura out the door in M32, and if not, it will very likely be M33.
I'm
> not sure how much effort you should really spend on the gtk version.)

Actually, gtk version has done here: [0], currently my code add a window filter
to the Xlib root window in order to listen for the KeyPress event depending on
gtk API: "gdk_window_add_filter". Maybe we can get rid of this limitation and
share the implementation between desktop aura and gtk in the future. I would
like to investigate solution during my holiday of Chinese National day.

By the way, any chance that chromium.org can provide a doc about: *_views,
*_gtk, *_cocoa, *_win, *_linux, *_mac, *_chromeos, *_aura, *_x11, *_aurax11, and
so on. Or maybe it has been documented but I didn't find it. Though so of them
are quite straightforward...

Thanks.

[0]: https://codereview.chromium.org/25007002/patch/14001/15009

[Elliot Glaysher, 2013-10-03 00:53:25]

On 2013/10/03 00:27:59, zhchbin wrote:
> Actually, gtk version has done here: [0], currently my code add a window
filter
> to the Xlib root window in order to listen for the KeyPress event depending on
> gtk API: "gdk_window_add_filter". Maybe we can get rid of this limitation and
> share the implementation between desktop aura and gtk in the future. I would
> like to investigate solution during my holiday of Chinese National day.

If it's already done, then I don't mind if the gtk solution goes in. After
reading it, I'm not sure whether factoring things so that code can be shared is
worth it. If you want to look into doing an aura implementation, take a look at
MessagePumpX11::{Add,Remove}DispatcherForRootWindow().
 
> By the way, any chance that http://chromium.org can provide a doc about:
*_views,
> *_gtk, *_cocoa, *_win, *_linux, *_mac, *_chromeos, *_aura, *_x11, *_aurax11,
and
> so on. Or maybe it has been documented but I didn't find it. Though so of them
> are quite straightforward...

It's not exactly documentation, but the file build/filename_rules.gypi lists the
mapping of gyp flags to filename exclusion regexes.

[Finnur, 2013-10-03 11:01:06]

> Are these keyboard shortcuts on by default? I assume they're opt-out instead
of
> opt-in?

We're going for a hybrid. The current thinking is to allow developers to
automatically assign shortcuts, but keep the pool of available shortcuts for
such purposes really restricted, such as Ctrl+Shift+[0..9] (and no others). The
user, however, would have a much larger pool to choose from and would be free to
change/delete the shortcut combination as they please (or make it non-global).

That way we have the full power of this feature, while at the same time limiting
accidental damage due to automatic assignments.

[Finnur, 2013-10-03 15:55:11]

Yoyo, can I ask you to review this CL (except for singleton_hwnd)?

Scott, can you review:

ui/gfx/win/singleton_hwnd.h
ui/gfx/win/singleton_hwnd.cc
... and do OWNERS check on:
chrome/browser/ui/views/browser_actions_container.cc
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc

Elliot (review/owners check on the stub):
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc

Nico (review/owners check on the stub):
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm


There are a lot of comments already on this CL because I threw it out to look
for comments from my compatriot working on the GTK version and it evolved into
this, which I think is now ready to be reviewed (patch set 21). Don't feel like
you need to catch up on the comment history or old patches (although you are
welcome to do so).

[Finnur, 2013-10-03 16:23:07]

> There are a lot of comments already on this CL because I threw it out to look

Meant to say: "Threw it out there", not "threw it out". :)

[(unused - use chromium), 2013-10-03 16:36:10]

My file says "error: old chunk mismatch" – can you try reuploading?


On Thu, Oct 3, 2013 at 9:23 AM, <finnur@chromium.org> wrote:

> There are a lot of comments already on this CL because I threw it out to
>> look
>>
>
> Meant to say: "Threw it out there", not "threw it out". :)
>
>
https://codereview.chromium.**org/23812010/<https://codereview.chromium.org/2...
>

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

[Nico, 2013-10-03 16:48:52]

Are there any tests for this? I couldn't see any filenames matching "_test"

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
// no further work is required.
This seems to be in every UI implementation. Is there some way to put this in
model code to share it somehow?

(You could for example have event_targets_ return an iterator that iterates over
the already-filtered sequence, which would allow you to get rid of the if, and
if this method on event_targets_ took command_name as parameter, it could return
an iterator with at most one element)

[Elliot Glaysher, 2013-10-03 17:39:31]

gtk stub lgtm

[sky, 2013-10-03 18:01:21]

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwnd.h
File ui/gfx/win/singleton_hwnd.h (right):

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwn...
ui/gfx/win/singleton_hwnd.h:37: void Init();
Is there a way not to expose this? Could we move this code into the constructor?

[Finnur, 2013-10-03 21:57:57]

Nico: No, there's no test yet, mostly because..
a) This is implemented on Windows only.
b) It requires loading Chrome and minimizing it (or switching to another app) to
test the global shortcut. I wasn't sure if that was supported by the framework.

I fully intend to add a test, but was delaying that until a later CL when the
feature is more complete.

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
// no further work is required.
I'll take a closer look tomorrow morning.

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwnd.h
File ui/gfx/win/singleton_hwnd.h (right):

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwn...
ui/gfx/win/singleton_hwnd.h:37: void Init();
Sure, we can create the window in the constructor. But it looked to me like we
want to create the window as late as possible, which in my case complicates the
design, so I opted for an optional "create now" function.

If you prefer I can create the window in the constructor. Just say the word.

[Yoyo Zhou, 2013-10-03 22:08:50]

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:61: bool
WhitelistedGlobalShortcut(const ui::Accelerator& accelerator) {
IsWhitelisted... seems like a clearer name.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:87:
ExtensionCommandsGlobalRegistry::GetFactoryInstance()->GetForProfile(profile);
A better way to do this is to call DependsOn in
CommandService::DeclareFactoryDependencies.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:288:
WhitelistedGlobalShortcut(iter->second.accelerator())) {
Why is this check here regardless of the scope?

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.h:54: ANY_SCOPE,
What does this mean? (This is only used for querying?)

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.h:106: // named commands
of the right |scope| or no active named commands when |type|
..."no such active named commands"...?

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/extension_commands_global_registry.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:47:
DependsOn(ExtensionSystemFactory::GetInstance());
This is already the default for DeclareFactoryDependencies so you don't need to
override it if it just looks like this.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:67: // Add all
the active keybindings (except page actions and browser actions,
This comment doesn't seem to make sense, since page/browser actions can't be
global keybindings.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:82: LOG(ERROR)
<< "Adding global keybinding for " << extension->name().c_str()
VLOG?

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:109: LOG(ERROR)
<< "Removing keybinding for " << extension->name().c_str()
ditto

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/extension_commands_global_registry.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.h:74:
std::pair<std::string, std::string> > EventTargets;
Why is this called EventTargets? I don't see either 'event' or 'target' in the
description.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener.cc:51: NOTREACHED();
If this is intended, the function comment should be clearer about when it's
expected to be called.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_aurax11.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_aurax11.cc:70: // 2) Call
base class RegisterAccelerator.
Unregister

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_aurax11.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_aurax11.h:16: class
GlobalShortcutListenerAuraX11 : public GlobalShortcutListener {
Should there be a TODO to implement this class?

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_chromeos.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_chromeos.h:19: class
GlobalShortcutListenerChromeOs : public GlobalShortcutListener {
nit: ChromeOS is a more common spelling than ChromeOs and also looks less weird.

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_win.cc:81: // Ensure that the
window has been created before calling RegisterHotKey
Which window is this?

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/performan...
File chrome/browser/performance_monitor/startup_timer.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/performan...
chrome/browser/performance_monitor/startup_timer.cc:113:
performance_monitor::Database* database =
You should be able to remove this now.

[sky, 2013-10-03 23:30:10]

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwnd.h
File ui/gfx/win/singleton_hwnd.h (right):

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwn...
ui/gfx/win/singleton_hwnd.h:37: void Init();
On 2013/10/03 21:57:58, Finnur wrote:
> Sure, we can create the window in the constructor. But it looked to me like we
> want to create the window as late as possible, which in my case complicates
the
> design, so I opted for an optional "create now" function.
> 
> If you prefer I can create the window in the constructor. Just say the word.

The constructor is only run on the first call to GetInstance() right? At that
point creating the window seems good to me. So, yes please:)

[Finnur, 2013-10-04 17:57:01]

Updated to address review comments. PTAL.

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:61: bool
WhitelistedGlobalShortcut(const ui::Accelerator& accelerator) {
Agreed. I also added the scope check here (good catch).

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:87:
ExtensionCommandsGlobalRegistry::GetFactoryInstance()->GetForProfile(profile);
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> A better way to do this is to call DependsOn in
> CommandService::DeclareFactoryDependencies.

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:288:
WhitelistedGlobalShortcut(iter->second.accelerator())) {
Fixed (see above).

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.h:54: ANY_SCOPE,
Added comments.

On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> What does this mean? (This is only used for querying?)

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.h:106: // named commands
of the right |scope| or no active named commands when |type|
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> ..."no such active named commands"...?

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/extension_commands_global_registry.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:47:
DependsOn(ExtensionSystemFactory::GetInstance());
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> This is already the default for DeclareFactoryDependencies so you don't need
to
> override it if it just looks like this.

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:67: // Add all
the active keybindings (except page actions and browser actions,
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> This comment doesn't seem to make sense, since page/browser actions can't be
> global keybindings.

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:82: LOG(ERROR)
<< "Adding global keybinding for " << extension->name().c_str()
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> VLOG?

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.cc:109: LOG(ERROR)
<< "Removing keybinding for " << extension->name().c_str()
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> ditto

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/extension_commands_global_registry.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/extension_commands_global_registry.h:74:
std::pair<std::string, std::string> > EventTargets;
Moved to base class (and added a bit more comments to explain).

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener.cc:51: NOTREACHED();
Clarified.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_aurax11.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_aurax11.cc:70: // 2) Call
base class RegisterAccelerator.
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> Unregister

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_aurax11.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_aurax11.h:16: class
GlobalShortcutListenerAuraX11 : public GlobalShortcutListener {
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> Should there be a TODO to implement this class?

Done.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener_chromeos.h (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_chromeos.h:19: class
GlobalShortcutListenerChromeOs : public GlobalShortcutListener {
On 2013/10/03 22:08:50, Yoyo Zhou wrote:
> nit: ChromeOS is a more common spelling than ChromeOs and also looks less
weird.

Done.

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener_win.cc:81: // Ensure that the
window has been created before calling RegisterHotKey
The singleton-hwnd, but this code is now redundant after addressing Scott's
comments.

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/performan...
File chrome/browser/performance_monitor/startup_timer.cc (right):

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/performan...
chrome/browser/performance_monitor/startup_timer.cc:113:
performance_monitor::Database* database =
Indeed.

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
// no further work is required.
I opted for moving this into the base class, which kills quite a bit of code and
duplication for the map. Would you mind taking a look at that code shift
(ExtensionKeybindingRegistry* classes)?

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwnd.h
File ui/gfx/win/singleton_hwnd.h (right):

https://codereview.chromium.org/23812010/diff/223001/ui/gfx/win/singleton_hwn...
ui/gfx/win/singleton_hwnd.h:37: void Init();
Done.

[Yoyo Zhou, 2013-10-04 18:12:06]

LGTM

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

https://codereview.chromium.org/23812010/diff/223001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:150:
// no further work is required.
On 2013/10/04 17:57:02, Finnur wrote:
> I opted for moving this into the base class, which kills quite a bit of code
and
> duplication for the map. Would you mind taking a look at that code shift
> (ExtensionKeybindingRegistry* classes)?

Looks fine to me.

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener.h (right):

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener.h:49: // Called by platform
specific implementations of this class whenever a key
What I meant is here, it's only called for keys that have observers registered.

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/performan...
File chrome/browser/performance_monitor/startup_timer.cc (right):

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/performan...
chrome/browser/performance_monitor/startup_timer.cc:115:
performance_monitor::Database* database =
This file is still in this CL for some reason.

[sky, 2013-10-04 18:32:23]

LGTM

[Nico, 2013-10-04 20:57:22]

cocoa file lgtm

(If I were the main reviewer, I wouldn't let this in without tests. You can
write a test that only runs on the platforms where this is implemented, and you
can mock out pieces that are difficult to run)

[Finnur, 2013-10-11 10:59:22]

Nico: Understood. Given the amount of total work, I felt both that I needed to
split it into smaller CLs (to lessen the burden on the reviewers) and find a
manageable set to implement before going on a multiple day offsite (which I just
barely managed to), in the hope that progress could be made on derived
changelists while I was away (and that was actually what happened).

Anyway, long story short: I'm checking this in and will be working on writing a
test for this functionality now.

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/extension...
File chrome/browser/extensions/global_shortcut_listener.h (right):

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/extension...
chrome/browser/extensions/global_shortcut_listener.h:49: // Called by platform
specific implementations of this class whenever a key
Good point. Added that.

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/performan...
File chrome/browser/performance_monitor/startup_timer.cc (right):

https://codereview.chromium.org/23812010/diff/263001/chrome/browser/performan...
chrome/browser/performance_monitor/startup_timer.cc:115:
performance_monitor::Database* database =
Removed.

[Finnur, 2013-10-11 13:13:58]

Committed patchset #30 manually as r228176 (presubmit successful).