[MacViews] Use ChromeCommandDispatcherDelegate to handle keyboard shortcuts.

[MacViews] Use ChromeCommandDispatcherDelegate to handle keyboard shortcuts.

This ensures that global keyboard shortcuts (e.g. Cmd+1) work when
no WebContents has focus.

Also add shortcuts defined in global_keyboard_shortcuts_mac.h into
c/b/ui/views/accelerator_table.cc so that global shortcuts work while
the WebContents does have focus.

At this point, all keyboard shortcuts should work with the exception
that many are not yet enabled in the main menu because MacViews
lacks NSUserInterfaceValidations.

BUG=508438
Committed: https://crrev.com/347ce6ba6128466a34f742f35d37df8a12aea2eb
Cr-Commit-Position: refs/heads/master@{#346825}

[jackhou1, 2015-08-27 01:42:17]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-08-27 01:42:17]

tapted, PTAL

[tapted, 2015-08-27 02:58:19]

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:39:
[base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(ns_window)
Should we change the return type of CreateNSWindow to be
NativeWidgetMacNSWindow? That should probably be part of the contract, since
there are lots of assumptions that this is the case.

Also/alternatively, did you consider putting this in OnWidgetCreated()? I don't
know if that's better... and it would make the cast necessary again, but this
seems like something it would make sense to do there (if it works out to be
neater)

https://codereview.chromium.org/1259513002/diff/60001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:59: // Public methods.
nit: should come before private methods (after initializer (or after dealloc, if
there was one))

https://codereview.chromium.org/1259513002/diff/60001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:65: // NSWindow overrides.
I think technically _isTitleHidden is an NSWindow override as well

[jackhou1, 2015-08-27 22:37:48]

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:39:
[base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(ns_window)
On 2015/08/27 02:58:18, tapted wrote:
> Should we change the return type of CreateNSWindow to be
> NativeWidgetMacNSWindow? That should probably be part of the contract, since
> there are lots of assumptions that this is the case.

Yeah I considered that, but didn't because native_widget_mac.h is included in
.cc files, like widget_test.h. Would you prefer the #if defined(__OBJC__) vs
casting here?

> Also/alternatively, did you consider putting this in OnWidgetCreated()? I
don't
> know if that's better... and it would make the cast necessary again, but this
> seems like something it would make sense to do there (if it works out to be
> neater)

You mean WidgetObserver::OnWidgetCreated()? It wouldn't work for
AppWindowNativeWidgetMac which isn't a BrowserView.

https://codereview.chromium.org/1259513002/diff/60001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:59: // Public methods.
On 2015/08/27 02:58:18, tapted wrote:
> nit: should come before private methods (after initializer (or after dealloc,
if
> there was one))

Done.

https://codereview.chromium.org/1259513002/diff/60001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:65: // NSWindow overrides.
On 2015/08/27 02:58:18, tapted wrote:
> I think technically _isTitleHidden is an NSWindow override as well

Done.

[tapted, 2015-08-28 01:36:07]

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:39:
[base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(ns_window)
On 2015/08/27 22:37:47, jackhou1 wrote:
> On 2015/08/27 02:58:18, tapted wrote:
> > Should we change the return type of CreateNSWindow to be
> > NativeWidgetMacNSWindow? That should probably be part of the contract, since
> > there are lots of assumptions that this is the case.
> 
> Yeah I considered that, but didn't because native_widget_mac.h is included in
> .cc files, like widget_test.h. Would you prefer the #if defined(__OBJC__) vs
> casting here?

Yeah.. I think so. It's ok just for the forward declare. I think it's more
important that implementors of this override to know that they really do need to
return something that inherits from NativeWidgetMacNSWindow


> > Also/alternatively, did you consider putting this in OnWidgetCreated()? I
> don't
> > know if that's better... and it would make the cast necessary again, but
this
> > seems like something it would make sense to do there (if it works out to be
> > neater)
> 
> You mean WidgetObserver::OnWidgetCreated()? It wouldn't work for
> AppWindowNativeWidgetMac which isn't a BrowserView.

For App Windows, I think it would need to go on ChromeNativeAppWindowViewsMac --
there's already an OnWidgetCreated override there.

[jackhou1, 2015-08-28 05:15:10]

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:39:
[base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(ns_window)
On 2015/08/28 01:36:06, tapted wrote:
> On 2015/08/27 22:37:47, jackhou1 wrote:
> > On 2015/08/27 02:58:18, tapted wrote:
> > > Should we change the return type of CreateNSWindow to be
> > > NativeWidgetMacNSWindow? That should probably be part of the contract,
since
> > > there are lots of assumptions that this is the case.
> > 
> > Yeah I considered that, but didn't because native_widget_mac.h is included
in
> > .cc files, like widget_test.h. Would you prefer the #if defined(__OBJC__) vs
> > casting here?
> 
> Yeah.. I think so. It's ok just for the forward declare. I think it's more
> important that implementors of this override to know that they really do need
to
> return something that inherits from NativeWidgetMacNSWindow

Done.
 
> > > Also/alternatively, did you consider putting this in OnWidgetCreated()? I
> > don't
> > > know if that's better... and it would make the cast necessary again, but
> this
> > > seems like something it would make sense to do there (if it works out to
be
> > > neater)
> > 
> > You mean WidgetObserver::OnWidgetCreated()? It wouldn't work for
> > AppWindowNativeWidgetMac which isn't a BrowserView.
> 
> For App Windows, I think it would need to go on ChromeNativeAppWindowViewsMac
--
> there's already an OnWidgetCreated override there.

Actually, I'm not sure what I was talking about. BrowserFrameMac is not a
BrowserView (or other WidgetObserver).

Also, looking at the list of global shortcuts, it doesn't seem like any of them
are relevant to native app windows. Plus native app windows have no UI other
than the WebContents, so they wouldn't trigger anyway. Reverted the changes to
AppWindowNativeWidgetMac.

[tapted, 2015-08-28 05:48:14]

lgtm

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1259513002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:39:
[base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>(ns_window)
On 2015/08/28 05:15:09, jackhou1 wrote:
> Also, looking at the list of global shortcuts, it doesn't seem like any of
them
> are relevant to native app windows. Plus native app windows have no UI other
> than the WebContents, so they wouldn't trigger anyway. Reverted the changes to
> AppWindowNativeWidgetMac.

Ah, good point!

https://codereview.chromium.org/1259513002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm (right):

https://codereview.chromium.org/1259513002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:83: // are handled
elsewhere.
elsewhere -> something more specific?

[jackhou1, 2015-09-01 06:51:05]

https://codereview.chromium.org/1259513002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm (right):

https://codereview.chromium.org/1259513002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:83: // are handled
elsewhere.
On 2015/08/28 05:48:14, tapted wrote:
> elsewhere -> something more specific?

Done.

[jackhou1, 2015-09-01 06:51:29]

jackhou@chromium.org changed reviewers:
+ sky@chromium.org

[jackhou1, 2015-09-01 06:51:29]

sky, please review for OWNERS in c/b/ui/views/

[sky, 2015-09-01 13:31:40]

LGTM

https://codereview.chromium.org/1259513002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1259513002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/accelerator_table.cc:24: const ui::EventFlags
kPlatformCommand = ui::EF_COMMAND_DOWN;
kPlatformModifier?

[jackhou1, 2015-09-02 01:48:11]

https://codereview.chromium.org/1259513002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/accelerator_table.cc (right):

https://codereview.chromium.org/1259513002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/accelerator_table.cc:24: const ui::EventFlags
kPlatformCommand = ui::EF_COMMAND_DOWN;
On 2015/09/01 13:31:40, sky wrote:
> kPlatformModifier?

Done.

[jackhou1, 2015-09-02 01:48:16]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-09-02 01:48:16]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1259513002/#ps160001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-09-02 01:48:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259513002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259513002/160001

[commit-bot: I haz the power, 2015-09-02 02:32:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-02 02:33:33]

Patchset 9 (id:??) landed as
https://crrev.com/347ce6ba6128466a34f742f35d37df8a12aea2eb
Cr-Commit-Position: refs/heads/master@{#346825}