[Mac] Factor out keyboard shortcut handling from ChromeEventProcessingWindow.

[Mac] Factor out keyboard shortcut handling from ChromeEventProcessingWindow.

This pulls the redispatching logic into CommandDispatcher which allows
it to be reused independent of Chrome, and is a major step towards enabling
keyboard shortcuts in MacViews. By using CommandDispatcher in
NativeWidgetMacNSWindow, main menu key equivalents (e.g. Cmd+c) now
work when the WebContents has focus.

Chrome-specific parts are moved to ChromeCommandDispatcherDelegate,
which will, in future, also be used by the Views browser in mac_views_browser=1
builds.

This includes minor changes to webview_example to demo command redispatch
in non-Chrome Views. E.g. Cmd+q now works in views_examples_with_content_exe
when the webview is focused.

Some test coverage is provided by GlobalKeyboardShortcutsTest.

BUG=508438, 504677
TEST=All keyboard shortcuts continue to work as before.

Committed: https://crrev.com/4c458c5afb65ce9bb7c4f3b1825825db0a4540a0
Cr-Commit-Position: refs/heads/master@{#346530}

[jackhou1, 2015-07-31 01:03:30]

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

[jackhou1, 2015-07-31 01:03:30]

tapted, PTAL

[tapted, 2015-07-31 03:06:50]

We need to split the "real" chrome dependencies out of the CommandDispatcher --
these need to stay in CEPW -- either before it calls into the CommandDispatcher,
or called by the CommandDispatcher via a CommandDispatcherDelegate method.

Basically, CEPW should be the CommandDispatcherDelegate and there should be a
"real" class `@interface CommandDispatcher` in ui/base/cocoa/

Currently ChromeCommandDispatchDelegate *is* the CommandDispatcher and there is
no delegate.

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

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_utils.mm:52: if ([command_delegate
handleExtraKeyboardShortcut:event window:window])
If it's cleaner, I think it's OK to leave handleExtraKeyboardShortcut: as a
method on CEPW. Since, this seems to be the only thing that wants the
commandDelegate accessor. This avoids exposing
chrome_command_dispatch_delegate.h (i.e. you can remove the #import as well)

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

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:111:
window:(NSWindow*)window
Could the delegate simply have a (weak) NSWindow* owner_; member?

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:116: if
(event.window) {
event.window -> window? (or, owner_?) (or, comment why not?)

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:117:
BrowserWindowController* controller = [event.window windowController];
event.window -> window?

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:117:
BrowserWindowController* controller = [event.window windowController];
ObjCCastStrict?

And.. that might actually fail. It looks like NativeAppWindowController
implements handledByExtensionCommand: but doesn't inherit from
BrowserWindowController.

Probably |controller| should just be declared `id`... [or there should be a
separate protocol..]

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:143: if
(nativePerformKeyEquivalent(event))
e.g. this could become [owner_ defaultPerformKeyEquivalent:event]

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:13: @protocol
CommandDispatchDelegate;
with the commandDelegate accessor removed, this can go too

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:35: // NSWindow
overrides:
These don't need to be redeclared in the header

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

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:28:
screen:(NSScreen*)screen {
Does something call this override in Chrome? (can it be NOTREACHED()?)
UnderlayOpenGLHostingWindow doesn't override it, but I guess that's just a
DCHECK.

And this is not a `designated initializer` I think the way ObjC works is that
non-designated initializers are obliged to call a designated initializer. (i.e.
the implementation of this in the base class will call the designated
initializer first, without passing |screen|.) But a stack from that NOTREACHED()
would verify this.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:51: return [super
performKeyEquivalent:event];
Ah. That's why.

I think it would be nicer to have something like

@protocol CommandDispatchOwner<NSWindow>
- (BOOL)defaultPerformKeyEquivalent;
@end

Then have CEPW implement it.

https://codereview.chromium.org/1255783002/diff/100001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatch_delegate.h (right):

https://codereview.chromium.org/1255783002/diff/100001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatch_delegate.h:11: @protocol CommandDispatchDelegate
hm - I imagined this as a regular @interface with some logic in it -- we want to
share as much of the complex logic in CEPW as possible.

I think this needs to be

@interface CommandDispatcher

which is *owned* by the CommandDisptcherDelegate, implemented by CEPW and
NativeWidgetMacNSWindow

https://codereview.chromium.org/1255783002/diff/100001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatch_delegate.h:23: // Called before a native
-sendEvent. Returns NO if handling of this event
Flip condition - it's currentlt the reverse of performKeyEquivalent, which
returns `YES` when the event is handled.

[jackhou1, 2015-08-03 06:05:24]

On 2015/07/31 03:06:50, tapted wrote:
> We need to split the "real" chrome dependencies out of the CommandDispatcher
--
> these need to stay in CEPW -- either before it calls into the
CommandDispatcher,
> or called by the CommandDispatcher via a CommandDispatcherDelegate method.
> 
> Basically, CEPW should be the CommandDispatcherDelegate and there should be a
> "real" class `@interface CommandDispatcher` in ui/base/cocoa/
> 
> Currently ChromeCommandDispatchDelegate *is* the CommandDispatcher and there
is
> no delegate.

Hm... I think the only non-Chrome part is the event redispatching code. What
about just merging the CommandDispatcher implementation with another NSWindow?
I.e. pull the Chrome stuff out of ChromeEventProcessingWindow into the
following:

@interface EventRedispatchingWindow : NSWindow
-setDelegate
-redispatchKeyEvent
-sendEvent
-performKeyEquivalent
@end

@interface ChromeEventProcessingWindow : EventRedispatchWindow
@interface NativeWidgetMacNSWindow : EventRedispatchWindow

@interface EventRedispatchingWindowDelegate
-handleExtensionCommand
-prePerformKeyEquivalent
-postPerformKeyEquivalent
@end

[tapted, 2015-08-03 06:13:37]

On 2015/08/03 06:05:24, jackhou1 wrote:
> On 2015/07/31 03:06:50, tapted wrote:
> > We need to split the "real" chrome dependencies out of the CommandDispatcher
> --
> > these need to stay in CEPW -- either before it calls into the
> CommandDispatcher,
> > or called by the CommandDispatcher via a CommandDispatcherDelegate method.
> > 
> > Basically, CEPW should be the CommandDispatcherDelegate and there should be
a
> > "real" class `@interface CommandDispatcher` in ui/base/cocoa/
> > 
> > Currently ChromeCommandDispatchDelegate *is* the CommandDispatcher and there
> is
> > no delegate.
> 
> Hm... I think the only non-Chrome part is the event redispatching code. What
> about just merging the CommandDispatcher implementation with another NSWindow?
> I.e. pull the Chrome stuff out of ChromeEventProcessingWindow into the
> following:
> 
> @interface EventRedispatchingWindow : NSWindow
> -setDelegate
> -redispatchKeyEvent
> -sendEvent
> -performKeyEquivalent
> @end
> 
> @interface ChromeEventProcessingWindow : EventRedispatchWindow
> @interface NativeWidgetMacNSWindow : EventRedispatchWindow
> 
> @interface EventRedispatchingWindowDelegate
> -handleExtensionCommand
> -prePerformKeyEquivalent
> -postPerformKeyEquivalent
> @end

That might work, although it would be nice to factor out the inheritance chain
approach since it limits flexibility a bit and feels a bit clumsy (e.g. there's
also this `UnderlayOpenGLWindow` in the mix already).

Let me dive deeper into where it's headed, and maybe have a play. Can you rebase
http://crrev.com/1259903002/ to master now the BrowserCommandExecutor has
landed?

[jackhou1, 2015-08-03 06:39:33]

On 2015/08/03 06:13:37, tapted wrote:
> On 2015/08/03 06:05:24, jackhou1 wrote:
> > On 2015/07/31 03:06:50, tapted wrote:
> > > We need to split the "real" chrome dependencies out of the
CommandDispatcher
> > --
> > > these need to stay in CEPW -- either before it calls into the
> > CommandDispatcher,
> > > or called by the CommandDispatcher via a CommandDispatcherDelegate method.
> > > 
> > > Basically, CEPW should be the CommandDispatcherDelegate and there should
be
> a
> > > "real" class `@interface CommandDispatcher` in ui/base/cocoa/
> > > 
> > > Currently ChromeCommandDispatchDelegate *is* the CommandDispatcher and
there
> > is
> > > no delegate.
> > 
> > Hm... I think the only non-Chrome part is the event redispatching code. What
> > about just merging the CommandDispatcher implementation with another
NSWindow?
> > I.e. pull the Chrome stuff out of ChromeEventProcessingWindow into the
> > following:
> > 
> > @interface EventRedispatchingWindow : NSWindow
> > -setDelegate
> > -redispatchKeyEvent
> > -sendEvent
> > -performKeyEquivalent
> > @end
> > 
> > @interface ChromeEventProcessingWindow : EventRedispatchWindow
> > @interface NativeWidgetMacNSWindow : EventRedispatchWindow
> > 
> > @interface EventRedispatchingWindowDelegate
> > -handleExtensionCommand
> > -prePerformKeyEquivalent
> > -postPerformKeyEquivalent
> > @end
> 
> That might work, although it would be nice to factor out the inheritance chain
> approach since it limits flexibility a bit and feels a bit clumsy (e.g.
there's
> also this `UnderlayOpenGLWindow` in the mix already).

Agreed.
 
> Let me dive deeper into where it's headed, and maybe have a play. Can you
rebase
> http://crrev.com/1259903002/ to master now the BrowserCommandExecutor has
> landed?

Done.

[jackhou1, 2015-08-04 01:03:59]

Just addressing the minor comments.

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

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_utils.mm:52: if ([command_delegate
handleExtraKeyboardShortcut:event window:window])
On 2015/07/31 03:06:49, tapted wrote:
> If it's cleaner, I think it's OK to leave handleExtraKeyboardShortcut: as a
> method on CEPW. Since, this seems to be the only thing that wants the
> commandDelegate accessor. This avoids exposing
> chrome_command_dispatch_delegate.h (i.e. you can remove the #import as well)

Done.

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

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:111:
window:(NSWindow*)window
On 2015/07/31 03:06:49, tapted wrote:
> Could the delegate simply have a (weak) NSWindow* owner_; member?

Done.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:116: if
(event.window) {
On 2015/07/31 03:06:49, tapted wrote:
> event.window -> window? (or, owner_?) (or, comment why not?) 

Done.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:117:
BrowserWindowController* controller = [event.window windowController];
On 2015/07/31 03:06:49, tapted wrote:
> ObjCCastStrict?
> 
> And.. that might actually fail. It looks like NativeAppWindowController
> implements handledByExtensionCommand: but doesn't inherit from
> BrowserWindowController.
> 
> Probably |controller| should just be declared `id`... [or there should be a
> separate protocol..]

Done.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatch_delegate.mm:143: if
(nativePerformKeyEquivalent(event))
On 2015/07/31 03:06:50, tapted wrote:
> e.g. this could become [owner_ defaultPerformKeyEquivalent:event]

Done.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:13: @protocol
CommandDispatchDelegate;
On 2015/07/31 03:06:50, tapted wrote:
> with the commandDelegate accessor removed, this can go too

Done.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:35: // NSWindow
overrides:
On 2015/07/31 03:06:50, tapted wrote:
> These don't need to be redeclared in the header

Done.

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

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:28:
screen:(NSScreen*)screen {
On 2015/07/31 03:06:50, tapted wrote:
> Does something call this override in Chrome? (can it be NOTREACHED()?)
> UnderlayOpenGLHostingWindow doesn't override it, but I guess that's just a
> DCHECK.
> 
> And this is not a `designated initializer` I think the way ObjC works is that
> non-designated initializers are obliged to call a designated initializer.
(i.e.
> the implementation of this in the base class will call the designated
> initializer first, without passing |screen|.) But a stack from that
NOTREACHED()
> would verify this.

Done.

https://codereview.chromium.org/1255783002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:51: return [super
performKeyEquivalent:event];
On 2015/07/31 03:06:50, tapted wrote:
> Ah. That's why.
> 
> I think it would be nicer to have something like
> 
> @protocol CommandDispatchOwner<NSWindow>
> - (BOOL)defaultPerformKeyEquivalent;
> @end
> 
> Then have CEPW implement it.

Done.

https://codereview.chromium.org/1255783002/diff/100001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatch_delegate.h (right):

https://codereview.chromium.org/1255783002/diff/100001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatch_delegate.h:11: @protocol CommandDispatchDelegate
On 2015/07/31 03:06:50, tapted wrote:
> hm - I imagined this as a regular @interface with some logic in it -- we want
to
> share as much of the complex logic in CEPW as possible.
> 
> I think this needs to be
> 
> @interface CommandDispatcher
> 
> which is *owned* by the CommandDisptcherDelegate, implemented by CEPW and
> NativeWidgetMacNSWindow

Yeah, CEPW and NativeWidgetMacNSWindow only need to implement
-defaultPerformKeyEquivalent, everything else can be shared. But, most of the
shared stuff is Chrome-specific and shouldn't live in Views.

https://codereview.chromium.org/1255783002/diff/100001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatch_delegate.h:23: // Called before a native
-sendEvent. Returns NO if handling of this event
On 2015/07/31 03:06:50, tapted wrote:
> Flip condition - it's currentlt the reverse of performKeyEquivalent, which
> returns `YES` when the event is handled.

Done.

[jackhou1, 2015-08-18 06:10:29]

tapted, PTAL

Patch set 8 has the following separation:
- @protocol CommandDispatcher is the NSWindow that actually receives
-performKeyEquivalent and -sendEvent. It also owns CommandDispatcherImpl.
- @interface CommandDispatcherImpl implements the dispatching logic.
- @protocol CommandDispatcherDelegate contains the non-Views code and is
implemented by ChromeCommandDispatcherDelegate.
- @protocol RenderWidgetHostViewMacBase has been renamed
CommandDispatcherTarget.

This CL should be a no-op as far as the browser is concerned, but does add
CommandDispatcher to views_examples_with_content_exe. The only key equivalent
defined in the main menu is Cmd+q, which you can test with the webview example
(it previously did not work).

[tapted, 2015-08-18 07:22:13]

I have more to absorb, but some initial high-level stuff that might make that
easier :)

https://codereview.chromium.org/1255783002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h (right):

https://codereview.chromium.org/1255783002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h:19: -
(BOOL)handleExtraKeyboardShortcut:(NSEvent*)event window:(NSWindow*)window;
Can this stay in ChromeEventProcessingWindow?

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

https://codereview.chromium.org/1255783002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:79: //
redispatched event, |event.window| gives the correct window.
What about if it isn't redispatched? (is event.window always correct?) This
might be irrelevant if we can move this back to CEPW

https://codereview.chromium.org/1255783002/diff/140001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/140001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:30: @protocol CommandDispatcher
I *think* this can be merged with CommandDispatcherDelegate -- then then there's
no need for a separate delegate object (it's always the owner)

Then CommandDispatcherImpl can just be called CommandDispatcher

https://codereview.chromium.org/1255783002/diff/140001/ui/views/controls/webv...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/1255783002/diff/140001/ui/views/controls/webv...
ui/views/controls/webview/webview.cc:253: void WebView::HandleKeyboardEvent(
is this override only to support views_examples? (if so, we might want to
separate that out to a different CL - I think in practice  it's more common for
webviews to be used like 

WebContents* web_contents = foo();
webview->SetWebContents(web_contents);

and in this case the WebView isn't the delegate for the WebContents, so this
won't get called.

An alternative fix for ViewsExamples might be to make WebViewExample the
WebContentsDelegate, and make use of UnhandledKeyboardEventHandler (i.e. same as
NativeAppWindowViews and friends). Of course we'd need to implement
UnhandledKeyboardEventHandler::HandleNativeKeyboardEvent like you were before
(and we may still need RepostNativeEvent to be implmeneted as well, but it looks
like WebDialog is the only caller and AFAIK it's not used much outside of
ChromeOS.... if at all even: InlineLoginDialog looks like dead code outside
ChromeOS).

[jackhou1, 2015-08-25 06:31:12]

https://codereview.chromium.org/1255783002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h (right):

https://codereview.chromium.org/1255783002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h:19: -
(BOOL)handleExtraKeyboardShortcut:(NSEvent*)event window:(NSWindow*)window;
On 2015/08/18 07:22:12, tapted wrote:
> Can this stay in ChromeEventProcessingWindow?

It could, but then we'd need to expose Handle*KeyboardShortcut.

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

https://codereview.chromium.org/1255783002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:79: //
redispatched event, |event.window| gives the correct window.
On 2015/08/18 07:22:12, tapted wrote:
> What about if it isn't redispatched? (is event.window always correct?) This
> might be irrelevant if we can move this back to CEPW

I think it should be fine, pretty sure the logic is the same as it was in CEPW
(I just added some commentary).

https://codereview.chromium.org/1255783002/diff/140001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/140001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:30: @protocol CommandDispatcher
On 2015/08/18 07:22:12, tapted wrote:
> I *think* this can be merged with CommandDispatcherDelegate -- then then
there's
> no need for a separate delegate object (it's always the owner)
> 
> Then CommandDispatcherImpl can just be called CommandDispatcher

In MacViews, NativeWidgetMacNSWindow is the CommandDispatcher, but there needs
to be a separate component for Chrome-specific stuff, hence the separation.
Also, this way, we can share ChromeCommandDispatcherDelegate between Cocoa and
Views.

https://codereview.chromium.org/1255783002/diff/140001/ui/views/controls/webv...
File ui/views/controls/webview/webview.cc (right):

https://codereview.chromium.org/1255783002/diff/140001/ui/views/controls/webv...
ui/views/controls/webview/webview.cc:253: void WebView::HandleKeyboardEvent(
On 2015/08/18 07:22:13, tapted wrote:
> is this override only to support views_examples? (if so, we might want to
> separate that out to a different CL - I think in practice  it's more common
for
> webviews to be used like 
> 
> WebContents* web_contents = foo();
> webview->SetWebContents(web_contents);
> 
> and in this case the WebView isn't the delegate for the WebContents, so this
> won't get called.
> 
> An alternative fix for ViewsExamples might be to make WebViewExample the
> WebContentsDelegate, and make use of UnhandledKeyboardEventHandler (i.e. same
as
> NativeAppWindowViews and friends). Of course we'd need to implement
> UnhandledKeyboardEventHandler::HandleNativeKeyboardEvent like you were before
> (and we may still need RepostNativeEvent to be implmeneted as well, but it
looks
> like WebDialog is the only caller and AFAIK it's not used much outside of
> ChromeOS.... if at all even: InlineLoginDialog looks like dead code outside
> ChromeOS).

Done.

Left it in this CL because it's a fairly small change, let me know if you prefer
to have it split out.

[tapted, 2015-08-26 03:04:49]

https://codereview.chromium.org/1255783002/diff/140001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/140001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:30: @protocol CommandDispatcher
On 2015/08/25 06:31:12, jackhou1 wrote:
> On 2015/08/18 07:22:12, tapted wrote:
> > I *think* this can be merged with CommandDispatcherDelegate -- then then
> there's
> > no need for a separate delegate object (it's always the owner)
> > 
> > Then CommandDispatcherImpl can just be called CommandDispatcher
> 
> In MacViews, NativeWidgetMacNSWindow is the CommandDispatcher, but there needs
> to be a separate component for Chrome-specific stuff, hence the separation.
> Also, this way, we can share ChromeCommandDispatcherDelegate between Cocoa and
> Views.

Ahhhhh. Yep. my plan would only work if we could inject the CEPW at
BrowserFrameMac::CreateNSWindow(), but that's not gonna work :(.

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_utils.mm:35:
base::mac::ObjCCastStrict<ChromeEventProcessingWindow>(window);
(this is a good change, but distracts a tiny bit - omit for now?)

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

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:1: // Copyright
2015 The Chromium Authors. All rights reserved.
Is there a way to mess with --similarity so that we can diff this against
chrome_event_processing_window.mm?

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:80: if
(event.window) {
nit: event.window -> [event window] (also above and below)

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.mm (right):

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:38: NOTREACHED();
could mark it @optional in the protocol maybe?

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:12: // CommandDispatcherImpl will dispatch
-performKeyEquivalent to this first.
Should say, "If the NSWindow's firstResponder implements
CommandDispatcherTarget, it is given the first opportunity to process a command,
possibly asynchronously. If the event is not handled, the target must return the
event to the CommandDispatchingWindow via -[[event window]
redispatchKeyEvent:event].

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:17: @end
declare performKeyEquivalent:(NSvent*)

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:19: // Implements a set of hooks into the
-performKeyEquivalent flow.
perhaps more like "Provides CommandDispatcher with the means to redirect key
equivalents at different stages of event handling."

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:21: -
(BOOL)handledByExtensionCommand:(NSEvent*)event
nit: objc style guide says stuff in .h must have a comment

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:28: // RenderWidgetHostView and handle
redispatching them if they are not handled.
Is `RenderWidgetHostView` relevant to this part? I think we can just say
something like ~The set of methods an NSWindow subclass needs to implement to
use CommandDispatcher.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:30: @protocol CommandDispatcher
I think call this `CommandDispatchingWindow` (or CommandDispatcherOwner?)

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:31: -
(void)setCommandDispatcherDelegate:(id<CommandDispatcherDelegate>)delegate;
Not sure if this belongs here (coming back to this)
Yeah.. does anything call this?

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:32: // Sends a key event to |NSApp
sendEvent:|, but also makes sure that it's not
nit: blank line before

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:33: // short-circuited to the RWHV. This is
used to send keyboard events to the menu
RHHV -> CommandDispatcherTarget

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:39: // Short-circuit to the default
-[NSResponder performKeyEquivalent:] which
nit: blank line before

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:44: // Handles dispatch of key events
including redispatching an unhandled event
I think this class should be declared first in this file (forward declare
CommandDispatchingWindow and the delgate). The comment should start with a
why-this-exists kind of rationale. E.g.

// An NSWindow can create a CommandDispatcher to assist with the interception
and redirection of key equivalents in order to execute commands. Delegate
functions indicate at what stage of the event processing a command should be
triggered (e.g. <words>). If no command is triggered, the key equivalent is
allowed to propagate to the default AppKit handling for the owning window.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:46: @interface CommandDispatcherImpl :
NSObject {
Then this can just be called `CommandDispatcher`

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:56: -
(void)setDelegate:(id<CommandDispatcherDelegate>)delegate;
@property? (then it doesn't need a comment :p)

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:58: // The CommandDispatcher should override
-[NSResponder performKeyEquivalent:]
CommandDispatcher -> CommandDispatchingWindow

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:59: // with this. Handles various keyboard
shortcut hooks before and after native
nit: with this -> and call this instead

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

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
Maybe we can even trick `git cl upload` to diff this against CEPW.mm as well

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:69: // Give the web site a chance to handle
the event. If it doesn't want to
the web site -> a CommandDispatcherTarget (e.g. a web site)

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:93: // Ordinarily, the event's window should
be this window. However, when
this window -> |owner_|

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:98: // (number) is our own.
our own -> our |owner_|'s

[jackhou1, 2015-08-26 06:24:43]

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_utils.mm:35:
base::mac::ObjCCastStrict<ChromeEventProcessingWindow>(window);
On 2015/08/26 03:04:48, tapted wrote:
> (this is a good change, but distracts a tiny bit - omit for now?)

Done.

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

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/08/26 03:04:48, tapted wrote:
> Is there a way to mess with --similarity so that we can diff this against
> chrome_event_processing_window.mm?

Done.

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:80: if
(event.window) {
On 2015/08/26 03:04:48, tapted wrote:
> nit: event.window -> [event window] (also above and below)

Done.

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.mm (right):

https://codereview.chromium.org/1255783002/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:38: NOTREACHED();
On 2015/08/26 03:04:48, tapted wrote:
> could mark it @optional in the protocol maybe?

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:12: // CommandDispatcherImpl will dispatch
-performKeyEquivalent to this first.
On 2015/08/26 03:04:49, tapted wrote:
> Should say, "If the NSWindow's firstResponder implements
> CommandDispatcherTarget, it is given the first opportunity to process a
command,
> possibly asynchronously. If the event is not handled, the target must return
the
> event to the CommandDispatchingWindow via -[[event window]
> redispatchKeyEvent:event].

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:17: @end
On 2015/08/26 03:04:49, tapted wrote:
> declare performKeyEquivalent:(NSvent*)

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:19: // Implements a set of hooks into the
-performKeyEquivalent flow.
On 2015/08/26 03:04:49, tapted wrote:
> perhaps more like "Provides CommandDispatcher with the means to redirect key
> equivalents at different stages of event handling."

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:21: -
(BOOL)handledByExtensionCommand:(NSEvent*)event
On 2015/08/26 03:04:49, tapted wrote:
> nit: objc style guide says stuff in .h must have a comment

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:28: // RenderWidgetHostView and handle
redispatching them if they are not handled.
On 2015/08/26 03:04:49, tapted wrote:
> Is `RenderWidgetHostView` relevant to this part? I think we can just say
> something like ~The set of methods an NSWindow subclass needs to implement to
> use CommandDispatcher.

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:30: @protocol CommandDispatcher
On 2015/08/26 03:04:49, tapted wrote:
> I think call this `CommandDispatchingWindow` (or CommandDispatcherOwner?)

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:31: -
(void)setCommandDispatcherDelegate:(id<CommandDispatcherDelegate>)delegate;
On 2015/08/26 03:04:49, tapted wrote:
> Not sure if this belongs here (coming back to this)
> Yeah.. does anything call this?

Moved to NativeWidgetMacNSWindow.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:32: // Sends a key event to |NSApp
sendEvent:|, but also makes sure that it's not
On 2015/08/26 03:04:49, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:33: // short-circuited to the RWHV. This is
used to send keyboard events to the menu
On 2015/08/26 03:04:48, tapted wrote:
> RHHV -> CommandDispatcherTarget

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:39: // Short-circuit to the default
-[NSResponder performKeyEquivalent:] which
On 2015/08/26 03:04:49, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:44: // Handles dispatch of key events
including redispatching an unhandled event
On 2015/08/26 03:04:49, tapted wrote:
> I think this class should be declared first in this file (forward declare
> CommandDispatchingWindow and the delgate). The comment should start with a
> why-this-exists kind of rationale. E.g.
> 
> // An NSWindow can create a CommandDispatcher to assist with the interception
> and redirection of key equivalents in order to execute commands. Delegate
> functions indicate at what stage of the event processing a command should be
> triggered (e.g. <words>). If no command is triggered, the key equivalent is
> allowed to propagate to the default AppKit handling for the owning window.

Done.

Re-wrote most of the commentary in this file, PTAL.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:46: @interface CommandDispatcherImpl :
NSObject {
On 2015/08/26 03:04:48, tapted wrote:
> Then this can just be called `CommandDispatcher`

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:56: -
(void)setDelegate:(id<CommandDispatcherDelegate>)delegate;
On 2015/08/26 03:04:48, tapted wrote:
> @property? (then it doesn't need a comment :p)

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:58: // The CommandDispatcher should override
-[NSResponder performKeyEquivalent:]
On 2015/08/26 03:04:48, tapted wrote:
> CommandDispatcher -> CommandDispatchingWindow

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:59: // with this. Handles various keyboard
shortcut hooks before and after native
On 2015/08/26 03:04:49, tapted wrote:
> nit: with this -> and call this instead

Done.

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

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2015/08/26 03:04:49, tapted wrote:
> Maybe we can even trick `git cl upload` to diff this against CEPW.mm as well

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:69: // Give the web site a chance to handle
the event. If it doesn't want to
On 2015/08/26 03:04:49, tapted wrote:
> the web site -> a CommandDispatcherTarget (e.g. a web site)

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:93: // Ordinarily, the event's window should
be this window. However, when
On 2015/08/26 03:04:49, tapted wrote:
> this window -> |owner_|

Done.

https://codereview.chromium.org/1255783002/diff/160001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:98: // (number) is our own.
On 2015/08/26 03:04:49, tapted wrote:
> our own -> our |owner_|'s

Done.

[tapted, 2015-08-26 06:46:54]

lgtm

(also, I had written this comment before but something ated it):

I think there's some coverage provided in the chrome side with a test you added
recently? (good to mention in the CL description). But, is there something
(maybe platform-independent with event generator) that we could add to
webview_unittest?

CL description should have a brief mention of the changes to webview_example.h
as well

https://codereview.chromium.org/1255783002/diff/200001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/200001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:54: // given the first opportunity to process
a command. To handle an event
nit: Move from "To handle an event..." onto a comment on performKeyEquivalent,
and add some vertical whitespace

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.h:22: -
(void)setCommandDispatcherDelegate:(id<CommandDispatcherDelegate>)delegate;
as discussed, this can come in the follow-up

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
File ui/views/cocoa/native_widget_mac_nswindow.mm (right):

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.mm:158: // Public methods.
nit: new methods before overrides (in the follow-up)

[jackhou1, 2015-08-26 07:38:18]

> I think there's some coverage provided in the chrome side with a test you
added
> recently? (good to mention in the CL description). But, is there something
> (maybe platform-independent with event generator) that we could add to
> webview_unittest?

I don't think we can add something platform-independent. The only thing to test
in the non-Chrome part is whether redispatching an event means the native OS
gets to handle it. I think on Aura, that might not even be the case,
redispatched events go straight to the focus manager.

It would be good to have an interactive test of the above for just Mac, e.g.
Cmd+t on a RenderWidgetHostView should work. I'm not sure if this is already
covered somewhere.

https://codereview.chromium.org/1255783002/diff/200001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/200001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:54: // given the first opportunity to process
a command. To handle an event
On 2015/08/26 06:46:54, tapted wrote:
> nit: Move from "To handle an event..." onto a comment on performKeyEquivalent,
> and add some vertical whitespace

Done.

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.h:22: -
(void)setCommandDispatcherDelegate:(id<CommandDispatcherDelegate>)delegate;
On 2015/08/26 06:46:54, tapted wrote:
> as discussed, this can come in the follow-up

Done.

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
File ui/views/cocoa/native_widget_mac_nswindow.mm (right):

https://codereview.chromium.org/1255783002/diff/200001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.mm:158: // Public methods.
On 2015/08/26 06:46:54, tapted wrote:
> nit: new methods before overrides (in the follow-up)

Done.

[jackhou1, 2015-08-26 07:55:19]

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

[jackhou1, 2015-08-26 07:55:20]

rsesek, please take a look in case you have any comments/suggestions, it's a
fairly big refactor for Mac.

[Robert Sesek, 2015-08-26 17:46:50]

https://codereview.chromium.org/1255783002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h (right):

https://codereview.chromium.org/1255783002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h:16: // Checks if
|event| is a window, delayed window, or browser keyboard shortcut.
This reads like the window is delayed rather than the event.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:20: // and overriding -[NSWindow
-performKeyEquivalent:] and -[NSWindow -sendEvent:]
nit: -[NSWindow performkeyEquivalent:] (the - is at the front of the [), same
with -sendEvent:

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:23: @private
These ivars can be hidden in the .mm:

@implementation CommandDispatcher {
}

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:29: @property(retain, nonatomic)
id<CommandDispatcherDelegate> delegate;
Delegates aren't usually retained. Maybe make a note of this?

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:31: -
(id)initWithOwner:(NSWindow<CommandDispatchingWindow>*)owner;
instancetype

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:39: // circuited to the
CommandDispatcherTarget. This is used to allow default
" but also ensures it is not short-circuited to the CommandDispatcherTarget."
isn't clear to me

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:49: - (BOOL)preSendEvent:(NSEvent*)event;
Should this be ordered before -redispatchKeyEvent: if this should be called
first?

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:70: -
(BOOL)handledByExtensionCommand:(NSEvent*)event
naming: eventHandledByExtensionCommand: ?

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:75: -
(BOOL)prePerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window;
What |window| is this?

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:86: // This can be implemented with
-[CommandDispatcher -redispatchKeyEvent:]. It's
nit: no - in the []

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

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:61: isRedispatch:redispatchingEvent_])
nit: requires braces since multi-line if

[jackhou1, 2015-08-27 00:01:25]

https://codereview.chromium.org/1255783002/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h (right):

https://codereview.chromium.org/1255783002/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h:16: // Checks if
|event| is a window, delayed window, or browser keyboard shortcut.
On 2015/08/26 17:46:49, Robert Sesek wrote:
> This reads like the window is delayed rather than the event.

Done.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:20: // and overriding -[NSWindow
-performKeyEquivalent:] and -[NSWindow -sendEvent:]
On 2015/08/26 17:46:49, Robert Sesek wrote:
> nit: -[NSWindow performkeyEquivalent:] (the - is at the front of the [), same
> with -sendEvent:

Done.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:23: @private
On 2015/08/26 17:46:49, Robert Sesek wrote:
> These ivars can be hidden in the .mm:
> 
> @implementation CommandDispatcher {
> }

Done.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:29: @property(retain, nonatomic)
id<CommandDispatcherDelegate> delegate;
On 2015/08/26 17:46:49, Robert Sesek wrote:
> Delegates aren't usually retained. Maybe make a note of this?

Looking at this again, the delegate should be owned by either the NSWindow or
something that owns the NSWindow anyway, so no need to retain. Changed to
assign.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:31: -
(id)initWithOwner:(NSWindow<CommandDispatchingWindow>*)owner;
On 2015/08/26 17:46:49, Robert Sesek wrote:
> instancetype

Done.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:39: // circuited to the
CommandDispatcherTarget. This is used to allow default
On 2015/08/26 17:46:49, Robert Sesek wrote:
> " but also ensures it is not short-circuited to the CommandDispatcherTarget."
> isn't clear to me

Deleted that bit and added a new sentence.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:49: - (BOOL)preSendEvent:(NSEvent*)event;
On 2015/08/26 17:46:49, Robert Sesek wrote:
> Should this be ordered before -redispatchKeyEvent: if this should be called
> first?

This is the order that the event is handled, i.e. it comes in at
-performKeyEquivalent, goes to CommandDispatcherTarget (a RenderWidgetHostView),
if unhandled it goes to -redispatchKeyEvent, then when the event comes back to
-preSendEvent, handling for that event stops.

Also this is closer to the order that was in ChromeEventProcessingWindow, which
makes the diff for command_dispatcher.mm a bit nicer.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:70: -
(BOOL)handledByExtensionCommand:(NSEvent*)event
On 2015/08/26 17:46:49, Robert Sesek wrote:
> naming: eventHandledByExtensionCommand: ?

Done.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:75: -
(BOOL)prePerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window;
On 2015/08/26 17:46:49, Robert Sesek wrote:
> What |window| is this?

Added comment.

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:86: // This can be implemented with
-[CommandDispatcher -redispatchKeyEvent:]. It's
On 2015/08/26 17:46:49, Robert Sesek wrote:
> nit: no - in the []

Done.

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

https://codereview.chromium.org/1255783002/diff/220001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:61: isRedispatch:redispatchingEvent_])
On 2015/08/26 17:46:49, Robert Sesek wrote:
> nit: requires braces since multi-line if

Done.

[jackhou1, 2015-08-27 22:32:50]

rsesek, did you have any other comments?

[Robert Sesek, 2015-08-27 23:25:23]

Sorry, was sitting on some drafts.

(Normally because of timezones I'd LG w/ nits this, but this is such a
fundamental component I want to give it a whole nother once-over tomorrow).

https://codereview.chromium.org/1255783002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

https://codereview.chromium.org/1255783002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:22:
base::scoped_nsobject<CommandDispatcher> commandDispatcher_;
Hide these ivars too.

https://codereview.chromium.org/1255783002/diff/280001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/280001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:36: // the event returns to the window at
-preSendEvent, handling will stop. The
nit: -preSendEvent: (colon part of name)

https://codereview.chromium.org/1255783002/diff/280001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:42: // this before a native -sendEvent.
Ensures that a redispatched event is not
nit: -sendEvent:

https://codereview.chromium.org/1255783002/diff/280001/ui/views/cocoa/native_...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/1255783002/diff/280001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.h:18:
base::scoped_nsobject<CommandDispatcher> commandDispatcher_;
Hide these ivars too.

[jackhou1, 2015-08-28 01:00:47]

> Sorry, was sitting on some drafts.
> 
> (Normally because of timezones I'd LG w/ nits this, but this is such a
> fundamental component I want to give it a whole nother once-over tomorrow).

No worries, thanks for the thorough review.

https://codereview.chromium.org/1255783002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

https://codereview.chromium.org/1255783002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:22:
base::scoped_nsobject<CommandDispatcher> commandDispatcher_;
On 2015/08/27 23:25:23, Robert Sesek wrote:
> Hide these ivars too.

Done.

https://codereview.chromium.org/1255783002/diff/280001/ui/base/cocoa/command_...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1255783002/diff/280001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:36: // the event returns to the window at
-preSendEvent, handling will stop. The
On 2015/08/27 23:25:23, Robert Sesek wrote:
> nit: -preSendEvent: (colon part of name)

Done.

https://codereview.chromium.org/1255783002/diff/280001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:42: // this before a native -sendEvent.
Ensures that a redispatched event is not
On 2015/08/27 23:25:23, Robert Sesek wrote:
> nit: -sendEvent:

Done.

https://codereview.chromium.org/1255783002/diff/280001/ui/views/cocoa/native_...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/1255783002/diff/280001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.h:18:
base::scoped_nsobject<CommandDispatcher> commandDispatcher_;
On 2015/08/27 23:25:23, Robert Sesek wrote:
> Hide these ivars too.

Done.

[Robert Sesek, 2015-08-28 19:25:32]

LGTM w/ a nit

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

https://codereview.chromium.org/1255783002/diff/320001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:75: // the |handle*| methods above.
No more handle methods.

[jackhou, 2015-08-31 07:26:17]

jackhou@google.com changed reviewers:
+ jackhou@google.com

[jackhou, 2015-08-31 07:26:18]

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

https://codereview.chromium.org/1255783002/diff/320001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.mm:75: // the |handle*| methods above.
On 2015/08/28 19:25:32, Robert Sesek (OOO til 3 Sept) wrote:
> No more handle methods.

Done.

[jackhou1, 2015-08-31 07:27:41]

jackhou@chromium.org changed reviewers:
+ sievers@chromium.org, sky@chromium.org
- jackhou@google.com

[jackhou1, 2015-08-31 07:27:41]

sievers, please review for OWNERS in content/

sky, please review for OWNERS in ui/

[sky, 2015-08-31 15:45:03]

Rubber stamp LGTM as I believe others have reviewed this thoroughly.

[no sievers, 2015-08-31 19:27:15]

content lgtm

[jackhou1, 2015-09-01 00:01:58]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-09-01 00:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1255783002/#ps360001
(title: "UI_BASE_EXPORT CommandDispatcher")

[commit-bot: I haz the power, 2015-09-01 00:02:19]

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

[commit-bot: I haz the power, 2015-09-01 00:08:52]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-09-01 00:09:39]

Patchset 19 (id:??) landed as
https://crrev.com/4c458c5afb65ce9bb7c4f3b1825825db0a4540a0
Cr-Commit-Position: refs/heads/master@{#346530}