Enable commands API for platform apps.

Enable commands API for platform apps.
Also make sure that accelerators associated with commands are actually triggered when a ShellWindow is active.

BUG=144434
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155790

[Marijn Kruisselbrink, 2012-08-30 22:00:35]

Antony: can you review the chrome/common/extensions part?
Evan: can you review the chrome/browser/ui/gtk part?
rsesek: can you review the chrome/browser/ui/cocoa part?
Ben: can you review the chrome/browser/ui/views part?

[asargent_no_longer_on_chrome, 2012-08-30 23:14:27]

chrome/common/extensions LGTM

[Mihai Parparita -not on Chrome, 2012-08-30 23:31:15]

After thinking about this a bit more, I think it might make sense for the
behavior to be slightly different. I think the ultimate desired behaviors are:

1. Commands registered by apps are system-wide (regardless of which window or
process is in the foreground)
2. Commands registered by extensions apply to Chrome only

This CL brings us slightly closer to 1 (though commands only work when Chrome is
in the foreground). However, it goes beyond 2. Assuming I'm reading this right,
extension commands will fire when an app window is in the foreground. Since
extensions are inherently tied to Chrome-the-browser, I don't think it makes
sense for them to be involved with Chrome-the-app-runtime.

This is analogous to the change that we made to the extension windowing API
(http://crrev.com/138340), where extensions couldn't see app windows through
chrome.windows.* API.

Implementing 2 would require some kind of scoping/filtering in
ExtensionKeybindingRegistry, not sure how amenable to that it is.

[Marijn Kruisselbrink, 2012-08-30 23:59:43]

On 2012/08/30 23:31:15, Mihai Parparita wrote:
> After thinking about this a bit more, I think it might make sense for the
> behavior to be slightly different. I think the ultimate desired behaviors are:
> 
> 1. Commands registered by apps are system-wide (regardless of which window or
> process is in the foreground)
> 2. Commands registered by extensions apply to Chrome only
> 
> This CL brings us slightly closer to 1 (though commands only work when Chrome
is
> in the foreground). However, it goes beyond 2. Assuming I'm reading this
right,
> extension commands will fire when an app window is in the foreground. Since
> extensions are inherently tied to Chrome-the-browser, I don't think it makes
> sense for them to be involved with Chrome-the-app-runtime.
> 
> This is analogous to the change that we made to the extension windowing API
> (http://crrev.com/138340), where extensions couldn't see app windows through
> chrome.windows.* API.
> 
> Implementing 2 would require some kind of scoping/filtering in
> ExtensionKeybindingRegistry, not sure how amenable to that it is.
If there is some way to figure out if the "active" window is associated with
some extension from the ExtensionKeybindingRegistry side this should be pretty
straight-forward; otherwise I guess I need to add something to pass the
extension or shell window an event originated in myself...

[Mihai Parparita -not on Chrome, 2012-08-31 00:07:31]

On Thu, Aug 30, 2012 at 4:59 PM, <mek@chromium.org> wrote:
>
> If there is some way to figure out if the "active" window is associated
> with
> some extension from the ExtensionKeybindingRegistry side this should be
> pretty
> straight-forward; otherwise I guess I need to add something to pass the
> extension or shell window an event originated in myself...
>

You can use a variant of WindowControllerList::CurrentWindowForFunction
(you don't have an ExtensionFunction in this case, but you can get at the
active window's WindowController, and use its IsVisibleToExtension method).

Mihai

[Mihai Parparita -not on Chrome, 2012-08-31 00:07:31]

On Thu, Aug 30, 2012 at 4:59 PM, <mek@chromium.org> wrote:
>
> If there is some way to figure out if the "active" window is associated
> with
> some extension from the ExtensionKeybindingRegistry side this should be
> pretty
> straight-forward; otherwise I guess I need to add something to pass the
> extension or shell window an event originated in myself...
>

You can use a variant of WindowControllerList::CurrentWindowForFunction
(you don't have an ExtensionFunction in this case, but you can get at the
active window's WindowController, and use its IsVisibleToExtension method).

Mihai

[Marijn Kruisselbrink, 2012-08-31 16:34:02]

On 2012/08/31 00:07:31, Mihai Parparita wrote:
> On Thu, Aug 30, 2012 at 4:59 PM, <mailto:mek@chromium.org> wrote:
> >
> > If there is some way to figure out if the "active" window is associated
> > with
> > some extension from the ExtensionKeybindingRegistry side this should be
> > pretty
> > straight-forward; otherwise I guess I need to add something to pass the
> > extension or shell window an event originated in myself...
> >
> 
> You can use a variant of WindowControllerList::CurrentWindowForFunction
> (you don't have an ExtensionFunction in this case, but you can get at the
> active window's WindowController, and use its IsVisibleToExtension method).
Unfortunately it doesn't seem to be that easy. In http://crrev.com/140947 all
the WindowController related bits got removed from ShellWindow, so anything
related to WindowController is not going to be helpfull to figure out if a
platform app is active...

[Marijn Kruisselbrink, 2012-08-31 22:19:45]

I now added an extra const Extension* parameter to the
ExtensionKeybindingRegistry, which when non-null is used to limit registration
of keybindings to just those of that extension, and use that in ShellWindow to
make sure only keybindings of the extension in that window are acted upon. Not
sure if this is the best approach, but it seems to do exactly what is desired.


On 2012/08/31 16:34:02, Marijn Kruisselbrink wrote:
> On 2012/08/31 00:07:31, Mihai Parparita wrote:
> > On Thu, Aug 30, 2012 at 4:59 PM, <mailto:mek@chromium.org> wrote:
> > >
> > > If there is some way to figure out if the "active" window is associated
> > > with
> > > some extension from the ExtensionKeybindingRegistry side this should be
> > > pretty
> > > straight-forward; otherwise I guess I need to add something to pass the
> > > extension or shell window an event originated in myself...
> > >
> > 
> > You can use a variant of WindowControllerList::CurrentWindowForFunction
> > (you don't have an ExtensionFunction in this case, but you can get at the
> > active window's WindowController, and use its IsVisibleToExtension method).
> Unfortunately it doesn't seem to be that easy. In http://crrev.com/140947 all
> the WindowController related bits got removed from ShellWindow, so anything
> related to WindowController is not going to be helpfull to figure out if a
> platform app is active...

[Mihai Parparita -not on Chrome, 2012-08-31 22:37:54]

As discussed, it would be even better if commands registered by platform
apps worked regardless of the window type that was in the foreground or who
owned it.

Mihai

On Thu, Aug 30, 2012 at 4:31 PM, <mihaip@chromium.org> wrote:

> After thinking about this a bit more, I think it might make sense for the
> behavior to be slightly different. I think the ultimate desired behaviors
> are:
>
> 1. Commands registered by apps are system-wide (regardless of which window
> or
> process is in the foreground)
> 2. Commands registered by extensions apply to Chrome only
>
> This CL brings us slightly closer to 1 (though commands only work when
> Chrome is
> in the foreground). However, it goes beyond 2. Assuming I'm reading this
> right,
> extension commands will fire when an app window is in the foreground. Since
> extensions are inherently tied to Chrome-the-browser, I don't think it
> makes
> sense for them to be involved with Chrome-the-app-runtime.
>
> This is analogous to the change that we made to the extension windowing API
> (http://crrev.com/138340), where extensions couldn't see app windows
> through
> chrome.windows.* API.
>
> Implementing 2 would require some kind of scoping/filtering in
> ExtensionKeybindingRegistry, not sure how amenable to that it is.
>
>
https://chromiumcodereview.**appspot.com/10911020/<https://chromiumcodereview...
>

[Marijn Kruisselbrink, 2012-08-31 23:35:23]

On 2012/08/31 22:37:54, Mihai Parparita wrote:
> As discussed, it would be even better if commands registered by platform
> apps worked regardless of the window type that was in the foreground or who
> owned it.
Okay, what about this then. Not entirely sure if having an enum like this is the
best/nicest API, but apart from commands of course still only working when
chrome is focused, I think this is the behavior you want.

Marijn

[Mihai Parparita -not on Chrome, 2012-09-04 22:09:45]

LGTM, thanks, this is pretty much what I had in mind.

https://chromiumcodereview.appspot.com/10911020/diff/4003/chrome/browser/exte...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://chromiumcodereview.appspot.com/10911020/diff/4003/chrome/browser/exte...
chrome/browser/extensions/extension_keybinding_registry.h:29: PLATFORM_APP_ONLY
Nit: call this PLATFORM_APPS_ONLY.

[Aaron Boodman, 2012-09-04 23:09:38]

On 2012/08/30 23:31:15, Mihai Parparita wrote:
> 2. Commands registered by extensions apply to Chrome only

In the long term, I disagree with this. We are going to want extensions to work
when app windows have focus. (We are going to want extensions to be able to
implement window managers that affect all windows).

That said, currently extensions don't know anything about platform apps windows,
so this approach is consistent.

[commit-bot: I haz the power, 2012-09-05 00:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10911020/13001

[commit-bot: I haz the power, 2012-09-05 00:03:35]

Presubmit check for 10911020-13001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/gtk
    chrome/browser/ui/views
    chrome/browser/ui/cocoa

Presubmit checks took 2.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Robert Sesek, 2012-09-05 15:14:52]

cocoa/ LGTM

[Evan Stade, 2012-09-05 21:38:27]

just nits, otherwise lgtm

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
(right):

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc:24:
Profile* profile, gfx::NativeWindow window,
every param on a new line.

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:96: // Add the keybinding
registry, now that the window has been realized.
are you sure the window has been realized? Note that realization is something
that occurs to GdkWindows, not GtkWindows.

If ExtensionKeybindingRegistryGtk requires the GdkWindow (i.e. window_->window)
to be realized before construction, you should probably document that somewhere.

[Marijn Kruisselbrink, 2012-09-06 18:09:56]

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:96: // Add the keybinding
registry, now that the window has been realized.
On 2012/09/05 21:38:27, Evan Stade wrote:
> are you sure the window has been realized? Note that realization is something
> that occurs to GdkWindows, not GtkWindows.
> 
> If ExtensionKeybindingRegistryGtk requires the GdkWindow (i.e.
window_->window)
> to be realized before construction, you should probably document that
somewhere.
Ah yes, I'm not sure actually if ExtensionKeybindingRegistryGtk requires the
window to be realized. I just copied this bit from browser_window_gtk, and there
the code just before it calls gtk_widget_realize. It seems to work fine, but to
be honest I don't really know what gtk_widget_realize does that could influence
this. I don't think ExtensionKeybindingRegistryGtk does anything that requires
the widget to be realized, so maybe I can just remove the part of the comment
that talks about the window being realized?

[Evan Stade, 2012-09-06 18:20:22]

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10911020/diff/13001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1823: // Add the keybinding
registry, now that the window has been realized.
well this makes more sense given the above. I don't know why
ExtensionKeybindingRegistryGtk would require the widget's GDK resources to be
realized, perhaps you should ask the original author.

[Ben Goodger (Google), 2012-09-10 15:42:59]

lgtm

[commit-bot: I haz the power, 2012-09-10 16:18:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10911020/11

[commit-bot: I haz the power, 2012-09-10 16:18:39]

Failed to apply patch for chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/gtk/extensions/shell_window_gtk.cc
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 112 (offset 20 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc.rej

[commit-bot: I haz the power, 2012-09-10 16:46:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10911020/10004

[commit-bot: I haz the power, 2012-09-10 19:11:22]

Change committed as 155790

[Finnur, 2012-09-11 10:29:42]

A bit late to the game -- I didn't see this CL until now.

Firstly, I'm not familiar with these shell window classes. Why do they need
keybindings and are there other types of windows we are missing?

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ext...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ext...
chrome/browser/extensions/extension_keybinding_registry.cc:41:
AddExtensionKeybinding(*iter, std::string());
nit: This is now multiline and needs braces.

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ext...
chrome/browser/extensions/extension_keybinding_registry.cc:89: }
I'm a little worried about the fact that this class was originally intended to
be completely in sync with other such classes (on the same platform). You'd
always get the same answer no matter what window was checking the registry, but
now the registry contains different shortcut assignments based on which type of
window it belongs to.

Now it seems that platform-app windows only know about platform-app shortcuts,
but other windows know about all shortcuts. Can that lead to problems?

What happens (or should happen) when you hit a platform-app-specific keyboard
shortcut in a non-platform-app window?

And similar for the reverse, should a keyboard shortcut for a regular extension
(non-platform app) be ignored when struck while a platform-app window has focus?

Also (thinking out loud here), lets assume you have an extension (not a
platform-app) register Ctrl+Y and then you install a platform-app that also
wants Ctrl+Y. I think AssignInitialKeybinding will do the right thing and not
override the Pref keybinding for the non-platform-app extension. Correct? Then,
the platform-app shortcut becomes inactive at install time, because another one
has taken Ctrl+Y? That's expected, right?

And finally, how should this work in the config UI? I assume platform apps can
only have named commands specified (not browser/page actions) and that those
just get listed and you can assign a shortcut from a platform app to a
non-platform app and vice versa, without problems. Correct?

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ext...
chrome/browser/extensions/extension_keybinding_registry.cc:100: {
nit: Should be on the line above.

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ext...
chrome/browser/extensions/extension_keybinding_registry.cc:107: NOTREACHED();
For enum types it is better to leave out the default clause because then you'll
get a compile error on non-Windows platforms when a new enum value is added and
they forget to update this switch statement to account for it.

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ui/...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

https://chromiumcodereview.appspot.com/10911020/diff/10004/chrome/browser/ui/...
chrome/browser/ui/views/extensions/shell_window_views.h:128:
UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
nit: Maybe document why we need this?