Allow permission bubbles to participate in key event dispatch as if they were a Browser.

Allow permission bubbles to participate in key event dispatch as if they were a Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881, 679339
Review-Url: https://codereview.chromium.org/2666523002
Cr-Commit-Position: refs/heads/master@{#447864}
Committed: https://chromium.googlesource.com/chromium/src/+/e91c25b113011f055428c46609e19ce6550b41ea

[tapted, 2017-01-30 07:14:24]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 07:14:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-30 07:18:15]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 07:18:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 11:23:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-30 11:23:16]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-31 07:23:09]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 07:23:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 08:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 08:17:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tapted, 2017-02-01 02:45:48]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 02:46:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 03:06:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 03:06:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-02-01 06:11:24]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 06:11:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-02-01 06:25:34]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 06:25:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 07:19:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 07:19:54]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-02-01 10:08:16]

Patchset #4 (id:50001) has been deleted

[tapted, 2017-02-01 10:08:26]

Patchset #3 (id:30001) has been deleted

[tapted, 2017-02-01 10:08:40]

Patchset #2 (id:20001) has been deleted

[tapted, 2017-02-01 10:09:33]

Patchset #4 (id:110001) has been deleted

[tapted, 2017-02-01 10:24:00]

Description was changed from

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser

BUG=603881
==========

to

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window.

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881
==========

[tapted, 2017-02-01 10:24:00]

tapted@chromium.org changed reviewers:
+ raymes@chromium.org, rsesek@chromium.org

[tapted, 2017-02-01 10:25:58]

Description was changed from

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window.

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881
==========

to

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881
==========

[tapted, 2017-02-01 10:35:01]

Hi Robert and Raymes, please take a look.
- Robert for the Cocoa stuff
- Raymes for chrome/browser/permissions (the TestApi, and the test too it it's
not too scary :))

https://codereview.chromium.org/2666523002/diff/130001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.mm (left):

https://codereview.chromium.org/2666523002/diff/130001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:81: if ([item action]
== @selector(commandDispatch:) ||
(this logic now moves into CommandDispatcher -- it was duplicated in
NativeWidgetMacNSWindow, so now they can share it \o/)

https://codereview.chromium.org/2666523002/diff/130001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2666523002/diff/130001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:305:
"../browser/permissions/permission_request_manager_test_api.h",
This seems like the right place, so that browser_tests and interactive_ui_tests
can share it... There isn't precedent here for things outside of base because
other files just get compiled twice (but I think that's gross?). E.g.
"../browser/{apps,extensions}/app_browsertest_util.cc" and
render_view_context_menu_browsertest_util.cc are compiled twice - once for
browser_tests and once for interactive_ui_tests

https://codereview.chromium.org/2666523002/diff/130001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.mm (right):

https://codereview.chromium.org/2666523002/diff/130001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:176:
forHandler:(id<UserInterfaceItemCommandHandler>)handler {
I might be able to store this in the CommandDispatcher instead.. Although I feel
like there was a reason we didn't go that route in the first place :/. Something
to explore I guess.

[Robert Sesek, 2017-02-01 14:27:20]

https://codereview.chromium.org/2666523002/diff/130001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/2666523002/diff/130001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm:37:
public ::testing::WithParamInterface<UiMode> {
Great test!

https://codereview.chromium.org/2666523002/diff/130001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.mm (right):

https://codereview.chromium.org/2666523002/diff/130001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:118: if (handler)
Why the discrepancy in handling the result of validateUserInterfaceItem:?

- handler gets to return the result
- appController only gets to if its YES
- owner only gets to if YES
- bubbleParent gets to return the result

[raymes, 2017-02-01 18:33:27]

chrome/browser/permissions lgtm 

I also took a rough look at 
chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm
and didn't see any problems.

Thanks so much for adding good tests that that cover existing codepaths as well
as new ones.

[tapted, 2017-02-01 23:14:18]

Description was changed from

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881
==========

to

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881, 679339
==========

[tapted, 2017-02-01 23:24:16]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 23:24:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-02-01 23:29:54]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[tapted, 2017-02-01 23:30:08]

Patchset #5 (id:150001) has been deleted

[commit-bot: I haz the power, 2017-02-01 23:30:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-02-01 23:36:01]

https://codereview.chromium.org/2666523002/diff/130001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.mm (right):

https://codereview.chromium.org/2666523002/diff/130001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:118: if (handler)
On 2017/02/01 14:27:20, Robert Sesek wrote:
> Why the discrepancy in handling the result of validateUserInterfaceItem:?
> 
> - handler gets to return the result
> - appController only gets to if its YES
> - owner only gets to if YES
> - bubbleParent gets to return the result

The problem is that (unlike performKeyEquivalent) -commandDispatch: doesn't give
a way to say "yes I handled that" (I toyed with adding a BOOL return to do this,
but found that is probably unnecessary).

This means that once NSMenu decides that -commandDispatch: is the chosen action,
it will only go to a single target (i.e. the first in the chain that
respondsToSelector:). All CommandProcessingWindows respond to commandDispatch,
so the "bubble up" logic picks the "right" one by the presence of |handler| --
That is, dispatch: (below) can't later decide to skip [handler
dispatchCommand:..] based on whether [handler validate..] returned YES or NO
here.

[NSApp ..] is the same for both so there shouldn't be a targeting issue there by
falling through (and it's necessary to allow bubbling of commandDispatch:).

The weird case comes from bubbling of things validated by the *superclass* of
bubbleParent via line 128, below. This works for some things, since NSMenu does
its own bubbling until it finds a target that respondsToSelector:.

Investigating further, currently this means that items targeting NSWindow like
`Zoom` which are disabled on bubbles in Views (since the bubble is not
Zoomable), but not in Cocoa (because it uses the `sharedKeyState` private APIs)
will now be enabled on Views (they weren't previously), but will NSBeep if you
try to select it (since the bubble respondsToSelector: but doesn't wanna). I've
added a TODO. It's unclear from the Harmony modality spec what the right action
is (i.e. it currently says all shortcuts that "make sense" should work while a
bubble is active, but I'm not sure what's sensible when the target/action is
specifically to do with window manipulation).

[commit-bot: I haz the power, 2017-02-02 00:36:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 00:36:37]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-02-02 17:37:53]

LGTM. I think I follow that…

[tapted, 2017-02-02 22:35:39]

On 2017/02/02 17:37:53, Robert Sesek wrote:
> LGTM. I think I follow that…

Yah, it's a bit nuts. Even the Apple docs say "processing keyboard input is by
far the most complex part of event dispatch" -

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Ev...

then we need to insert renderers and IPC in to the mix..

[tapted, 2017-02-02 22:36:02]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-02-02 22:36:02]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2666523002/#ps170001
(title: " comments, fix permission->type missed in a refactor")

[commit-bot: I haz the power, 2017-02-02 22:36:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 22:43:55]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1486074962513920,
"parent_rev": "9f855ec8d57996af189f479bbb5f37a378921250", "commit_rev":
"e91c25b113011f055428c46609e19ce6550b41ea"}

[commit-bot: I haz the power, 2017-02-02 22:45:05]

Description was changed from

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881, 679339
==========

to

==========
Allow permission bubbles to participate in key event dispatch as if they were a
Browser.

Accomplish this by allowing the `CommandDispatcher` we use for
ChomeEventProcessingWindows to bubble up key handling, and command
validation/dispatch. Dispatch goes up to a parent window's
CommandDispatcher when certain conditions are met.

Currently all ChomeEventProcessingWindows and Views'
NativeWidgetMacNSWindows have a CommandDispatcher, but only browser
windows provide a CommandHandler. By asking the CommandHandler in the
parent window, we can validate commands in the mainMenu, then forward
the -commandDispatch: action from the menu item when NSMenu calls it on
the key window (which might not be a browser).

Adds a rather fun interactive UI test for this that runs with Views and
Cocoa permissions bubbles, and tests commands dispatched via the
mainMenu (Cmd+w, Cmd+Alt+Left) and performKeyEquivalent (Cmd+Shift+'{')

BUG=603881, 679339

Review-Url: https://codereview.chromium.org/2666523002
Cr-Commit-Position: refs/heads/master@{#447864}
Committed:
https://chromium.googlesource.com/chromium/src/+/e91c25b113011f055428c46609e1...
==========

[commit-bot: I haz the power, 2017-02-02 22:45:06]

Committed patchset #5 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/e91c25b113011f055428c46609e1...