[Mac] Move UI item validation to UserInterfaceItemCommandHandler.

[Mac] Move UI item validation to UserInterfaceItemCommandHandler.

NSUserInterfaceValidations moves from BrowserWindowCocoa to
the NSWindow. NSWindow then delegates them to UserInterfaceItemCommandHandler.

The implementation moves to BrowserWindowCommandHandler,
and is only set on the CommandDispatchingWindow for browser
windows.

This will allow MacViews to share the same UI item validation as
Cocoa.

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

[jackhou1, 2015-08-27 05:15:09]

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

[jackhou1, 2015-08-27 05:15:09]

tapted, PTAL

I've also updated the combined CL: https://codereview.chromium.org/1259903002/

[tapted, 2015-08-27 07:24:43]

some initial comments - still more to absorb

https://codereview.chromium.org/1250403002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h (right):

https://codereview.chromium.org/1250403002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h:13: // shortcuts
and executing them with chrome::ExecuteCommand.
update comment?

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
ui/base/cocoa/command_dispatcher.h:82: @protocol
CommandDispatchingWindow<NSUserInterfaceValidations>
does the window need to implement this (i.e. is the dispatcher relying on this?
it doesn't seem to, and it uses NSWidnow for storage anyway

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
ui/base/cocoa/command_dispatcher.h:84: @property(assign, nonatomic)
id<UserInterfaceItemCommandHandler> commandHandler;
Could we just force the owner to implement this?

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

https://codereview.chromium.org/1250403002/diff/40001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:5: #import
"ui/views/cocoa/native_widget_mac_nswindow.h"
the header should change too right...? (to implement the protocol)

[tapted, 2015-08-28 05:04:02]

ok I think I've grokked it

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
ui/base/cocoa/command_dispatcher.h:101: - (void)commandDispatch:(id)sender;
What's calling these?

What happens if we refactor those call sites to call methods on the
CommandDispatcher directly?

CommandDispatchingWindow then just needs a method to return the
CommandDisptcher*.


(I think I may have given the opposite advice for redispatchKeyEvent, but now
there's multiple functions it looks a bit clumsy -- i.e. with that change,
callers of [[foo window] redispatchKeyEvent:..] could be updated to instead do 
[[[foo window] commandDispatcher] redispatchKeyEvent:..])

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

https://codereview.chromium.org/1250403002/diff/40001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:171: return [[self commandHandler]
validateUserInterfaceItem:item window:self];
(if my other comment doesn't make sense) could these calls instead be something
like

[commandDispatcher_ commandDispatch:sender];

(i.e. CommandDispatcher just assumes its delegate will handle it, and uses
window=owner). Then get rid of setCommandHandler

[jackhou1, 2015-09-03 01:12:49]

https://codereview.chromium.org/1250403002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h (right):

https://codereview.chromium.org/1250403002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h:13: // shortcuts
and executing them with chrome::ExecuteCommand.
On 2015/08/27 07:24:42, tapted wrote:
> update comment?

Done.

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
File ui/base/cocoa/command_dispatcher.h (right):

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
ui/base/cocoa/command_dispatcher.h:82: @protocol
CommandDispatchingWindow<NSUserInterfaceValidations>
On 2015/08/27 07:24:42, tapted wrote:
> does the window need to implement this (i.e. is the dispatcher relying on
this?
> it doesn't seem to, and it uses NSWidnow for storage anyway

Yeah it's not necessary to require this.

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
ui/base/cocoa/command_dispatcher.h:84: @property(assign, nonatomic)
id<UserInterfaceItemCommandHandler> commandHandler;
On 2015/08/27 07:24:42, tapted wrote:
> Could we just force the owner to implement this?

Yeah, this is only used to set it on NativeWidgetMacNSWindow, moved to there.

https://codereview.chromium.org/1250403002/diff/40001/ui/base/cocoa/command_d...
ui/base/cocoa/command_dispatcher.h:101: - (void)commandDispatch:(id)sender;
On 2015/08/28 05:04:02, tapted wrote:
> What's calling these?
> 
> What happens if we refactor those call sites to call methods on the
> CommandDispatcher directly?
> 
> CommandDispatchingWindow then just needs a method to return the
> CommandDisptcher*.
> 
> 
> (I think I may have given the opposite advice for redispatchKeyEvent, but now
> there's multiple functions it looks a bit clumsy -- i.e. with that change,
> callers of [[foo window] redispatchKeyEvent:..] could be updated to instead do

> [[[foo window] commandDispatcher] redispatchKeyEvent:..])

These are hard coded in the nibs. I.e. a button might have
{target=FirstResponder, selector=commandDispatch:}, so it'll go up the chain
looking for something that responds to the selector.

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

https://codereview.chromium.org/1250403002/diff/40001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:5: #import
"ui/views/cocoa/native_widget_mac_nswindow.h"
On 2015/08/27 07:24:42, tapted wrote:
> the header should change too right...? (to implement the protocol)

Done.

In patch set 3 it wasn't necessary for NativeWidgetMacNSWindow to implement
NSUserInterfaceValidations because it was implied by implementing
CommandDispatchingWindow.

https://codereview.chromium.org/1250403002/diff/40001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:171: return [[self commandHandler]
validateUserInterfaceItem:item window:self];
On 2015/08/28 05:04:02, tapted wrote:
> (if my other comment doesn't make sense) could these calls instead be
something
> like
> 
> [commandDispatcher_ commandDispatch:sender];
> 
> (i.e. CommandDispatcher just assumes its delegate will handle it, and uses
> window=owner). Then get rid of setCommandHandler

It feels unnecessary for these methods to go through CommandDispatcher. And the
fact that ChromeCommandDispatcherDelegate is also
UserInterfaceItemCommandHandler is just a convenient implementation detail. They
don't really need to be coupled. In fact, we could even move
UserInterfaceItemCommandHandler to another file.

BTW, in patch set 4, setCommandHandler is only on NativeWidgetMacNSWindow, and
CEPW calls
[commandDispatcherDelegate_ validate...]

[tapted, 2015-09-03 04:15:38]

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:195: [menuItem
setTitle:[controller titleForFullscreenMenuItem]];
titleForFullscreenMenuItem is only used by this function. There are implications
here for mac_views_browser if we need the controller (right?).

I think we can move titleForFullscreenMenuItem to an anonymous namespace here
that accepts a Browser* and (for now) tries to find a BrowserWindowController.
Then TODO(..): Call BaseWindow::IsFullscreen() instead (except that returns the
wrong value on SnowLeopard currently...).

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:248:
chrome::IsCommandEnabled(browser, IDC_ENCODING_MENU) ? YES : NO;
nit: I don't think the YES : NO is necessary. It's inconsistent with
IDC_CLOSE_TAB anyway.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:262: // Identify
the actual BWC to which the command should be dispatched. It might
nit: BWC -> Browser?

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:263: // belong to
a background window, yet this controller gets it because it is
controller -> dispatcher? (or something?)

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:292: // See
comment above for why we do this.
Perhaps a FindBrowserForSender() function in an anonymous namespace - this
borders on too much copy paste

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:21:
NSUserInterfaceValidations>
NSWindow already conforms to NSUserInterfaceValidations so I think this is
redundant

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_event_processing_window.mm (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:48: -
(BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
move down below NSWindow overrides. Maybe

// NSWindow overrides (NSUserInterfaceValidations implementation).

But then... should it also call [super validateUserInterfaceItem] if the
dispatcher returns YES?

I think it should... I suspect this might be responsible for things like
disabling menu items when a Sheet is open (e.g. Cmd+S)

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:73: [ns_window
setUserInterfaceItemCommandHandler:command_dispatcher_delegate_];
can NativeWidgetMacNSWindow assume these are always the same for now? (i.e. do
we lose anything?)

Nothing stood out in chrome_command_dispatcher_delegate.mm that would make sense
to handle differently, e.g., for app windows.

https://codereview.chromium.org/1250403002/diff/80001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/1250403002/diff/80001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.h:17: :
NSWindow<CommandDispatchingWindow, NSUserInterfaceValidations>
same here - NSUserInterfaceValidations is redundant

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

https://codereview.chromium.org/1250403002/diff/80001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:177: -
(BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
same comments as on CEPW

[jackhou1, 2015-09-03 06:15:39]

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:195: [menuItem
setTitle:[controller titleForFullscreenMenuItem]];
On 2015/09/03 04:15:38, tapted wrote:
> titleForFullscreenMenuItem is only used by this function. There are
implications
> here for mac_views_browser if we need the controller (right?).
> 
> I think we can move titleForFullscreenMenuItem to an anonymous namespace here
> that accepts a Browser* and (for now) tries to find a BrowserWindowController.
> Then TODO(..): Call BaseWindow::IsFullscreen() instead (except that returns
the
> wrong value on SnowLeopard currently...).

Done.

The logic for what text to show is currently in BWC. I think that needs to move
out of BWC into chrome::mac:: somewhere.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:248:
chrome::IsCommandEnabled(browser, IDC_ENCODING_MENU) ? YES : NO;
On 2015/09/03 04:15:38, tapted wrote:
> nit: I don't think the YES : NO is necessary. It's inconsistent with
> IDC_CLOSE_TAB anyway.

Done.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:262: // Identify
the actual BWC to which the command should be dispatched. It might
On 2015/09/03 04:15:38, tapted wrote:
> nit: BWC -> Browser?

Done.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:263: // belong to
a background window, yet this controller gets it because it is
On 2015/09/03 04:15:38, tapted wrote:
> controller -> dispatcher? (or something?)

Done.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:292: // See
comment above for why we do this.
On 2015/09/03 04:15:38, tapted wrote:
> Perhaps a FindBrowserForSender() function in an anonymous namespace - this
> borders on too much copy paste

Done.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:21:
NSUserInterfaceValidations>
On 2015/09/03 04:15:38, tapted wrote:
> NSWindow already conforms to NSUserInterfaceValidations so I think this is
> redundant

Done.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chrome_event_processing_window.mm (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:48: -
(BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
On 2015/09/03 04:15:38, tapted wrote:
> move down below NSWindow overrides. Maybe
> 
> // NSWindow overrides (NSUserInterfaceValidations implementation).
> 
> But then... should it also call [super validateUserInterfaceItem] if the
> dispatcher returns YES?
> 
> I think it should... I suspect this might be responsible for things like
> disabling menu items when a Sheet is open (e.g. Cmd+S)

Done.

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame_mac.mm (right):

https://codereview.chromium.org/1250403002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame_mac.mm:73: [ns_window
setUserInterfaceItemCommandHandler:command_dispatcher_delegate_];
On 2015/09/03 04:15:38, tapted wrote:
> can NativeWidgetMacNSWindow assume these are always the same for now? (i.e. do
> we lose anything?)
> 
> Nothing stood out in chrome_command_dispatcher_delegate.mm that would make
sense
> to handle differently, e.g., for app windows.

Done.

https://codereview.chromium.org/1250403002/diff/80001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/1250403002/diff/80001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.h:17: :
NSWindow<CommandDispatchingWindow, NSUserInterfaceValidations>
On 2015/09/03 04:15:38, tapted wrote:
> same here - NSUserInterfaceValidations is redundant

Done.

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

https://codereview.chromium.org/1250403002/diff/80001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.mm:177: -
(BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
On 2015/09/03 04:15:38, tapted wrote:
> same comments as on CEPW

Done.

[tapted, 2015-09-03 06:54:49]

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

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:121: return
[controller titleForFullscreenMenuItem];
Can you remove titleForFullscreenMenuItem and put the logic here instead?

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:125: // might
belong to a background window, yet this dispatcher gets it because it
nit: this -> another ?

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:226: if
(chrome::mac::SupportsSystemFullscreen())
... pretty sure this is dead code now. either it doesn't support
systemFullscreen, or IDC_PRESENTATION_MODE is hidden - can't have both. Maybe a
good followup to poke erik or sarah with

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:279: Browser*
browser = FindBrowserForSender(sender, window);
nit: These temporaries are probably no longer needed (or if you think they are,
they should be declared closer to the first use)

[tapted, 2015-09-03 06:55:29]

looks like there are some test failures too..

[jackhou1, 2015-09-03 08:12:29]

All the tests are due to assuming that the window is a browser window. By moving
logic from BrowserWindowController to (effectively) CEPW, all windows are using
the same validation. This isn't correct, e.g. panels expect
-validateUserInterfaceItems to be unimplemented, so it's deferred to
AppController. I think we'll have to separate ChromeCommandDispatcherDelegate
and UserInterfaceItemHandler.

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

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:121: return
[controller titleForFullscreenMenuItem];
On 2015/09/03 06:54:49, tapted wrote:
> Can you remove titleForFullscreenMenuItem and put the logic here instead?

Ah oops, I see what you mean.

Done.

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:125: // might
belong to a background window, yet this dispatcher gets it because it
On 2015/09/03 06:54:49, tapted wrote:
> nit: this -> another ?

Done.

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:226: if
(chrome::mac::SupportsSystemFullscreen())
On 2015/09/03 06:54:49, tapted wrote:
> ... pretty sure this is dead code now. either it doesn't support
> systemFullscreen, or IDC_PRESENTATION_MODE is hidden - can't have both. Maybe
a
> good followup to poke erik or sarah with

When it does support systemFullscreen, the IDC_PRESENTATION_MODE menu item is
given the same title ("Enter Fullscreen") and set as the alternate so that
Shift+Cmd+f does the same as Ctrl+Cmd+f.

https://codereview.chromium.org/1250403002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm:279: Browser*
browser = FindBrowserForSender(sender, window);
On 2015/09/03 06:54:49, tapted wrote:
> nit: These temporaries are probably no longer needed (or if you think they
are,
> they should be declared closer to the first use)

Done.

[jackhou1, 2015-09-04 01:50:33]

tapted, PTAL at patch set 8.

[tapted, 2015-09-04 02:56:24]

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

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.h:8: #import
<Cocoa/Cocoa.h>
nit: Foundation/Foundation.h for NSObject

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.h:15: :
NSObject<UserInterfaceItemCommandHandler>
I wonder... does this even need to be NSObject? Can we just use C++ interfaces?
I don't know if that's better...

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

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:71: // Get the text
for the "Enter/Exit Fullscreen/Presentation Mode" menu item.
I think this still needs a TODO, to remove the dependency on BWC(Private). Easy
to do if 10.6 goes away....

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:115: // should be
handled by AppController.
From the comment, It's not too clear without context to understand why it's
NOTREACHED

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:246: @end  //
BrowserWindowCommandHandler
nit: doesn't need comment

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

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:232:
setCommandHandler:commandHandler_];
So, I'm currently thinking something like

[[window commandDispatcher] setCommandHandler:
    make_scoped_ptr(new BrowserWindowCommandHandler)];

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

https://codereview.chromium.org/1250403002/diff/140001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:108: @protocol
UserInterfaceItemCommandHandler
since this is more optional, perhaps it should be a separate file?

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

https://codereview.chromium.org/1250403002/diff/140001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.mm:28: @synthesize commandHandler;
With this approach (i.e. assign not retain), I think the handler can easily be
destroyed before the NSWindow.

E.g. BrowserFrameMac can remove the last reference to its handler and destroy
it, but the NSWindow can still be receiving events, leading to UAF in this
class.

I think either this needs to be a scoped_nsobject + retain semantics, or the
commandDispatcher should own it instead.

https://codereview.chromium.org/1250403002/diff/140001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.mm:184: // Since this class implements
these selectors, |super| will always say they
Then perhaps this logic can move into commandDispatcher and be shared

[jackhou1, 2015-09-04 06:13:59]

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

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.h:8: #import
<Cocoa/Cocoa.h>
On 2015/09/04 02:56:24, tapted wrote:
> nit: Foundation/Foundation.h for NSObject

Done.

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.h:15: :
NSObject<UserInterfaceItemCommandHandler>
On 2015/09/04 02:56:24, tapted wrote:
> I wonder... does this even need to be NSObject? Can we just use C++
interfaces?
> I don't know if that's better...

It can be C++, but I'm not sure if there's much benefit. Most of the code
surrounding it is ObjC.

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

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:71: // Get the text
for the "Enter/Exit Fullscreen/Presentation Mode" menu item.
On 2015/09/04 02:56:24, tapted wrote:
> I think this still needs a TODO, to remove the dependency on BWC(Private).
Easy
> to do if 10.6 goes away....

Done.

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:115: // should be
handled by AppController.
On 2015/09/04 02:56:24, tapted wrote:
> From the comment, It's not too clear without context to understand why it's
> NOTREACHED

Updated comment.

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:246: @end  //
BrowserWindowCommandHandler
On 2015/09/04 02:56:24, tapted wrote:
> nit: doesn't need comment

Done.

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

https://codereview.chromium.org/1250403002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.mm:232:
setCommandHandler:commandHandler_];
On 2015/09/04 02:56:24, tapted wrote:
> So, I'm currently thinking something like
> 
> [[window commandDispatcher] setCommandHandler:
>     make_scoped_ptr(new BrowserWindowCommandHandler)];

Made CommandDispatchingWindow retain this, so BrowserWindowController doesn't
need to own it.

I don't think it really needs to go through CommandDispatcher, plus it would add
more boilerplate.

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

https://codereview.chromium.org/1250403002/diff/140001/ui/base/cocoa/command_...
ui/base/cocoa/command_dispatcher.h:108: @protocol
UserInterfaceItemCommandHandler
On 2015/09/04 02:56:24, tapted wrote:
> since this is more optional, perhaps it should be a separate file?

Done.

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

https://codereview.chromium.org/1250403002/diff/140001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.mm:28: @synthesize commandHandler;
On 2015/09/04 02:56:24, tapted wrote:
> With this approach (i.e. assign not retain), I think the handler can easily be
> destroyed before the NSWindow.
> 
> E.g. BrowserFrameMac can remove the last reference to its handler and destroy
> it, but the NSWindow can still be receiving events, leading to UAF in this
> class.
> 
> I think either this needs to be a scoped_nsobject + retain semantics, or the
> commandDispatcher should own it instead.

Changed to retain.

https://codereview.chromium.org/1250403002/diff/140001/ui/views/cocoa/native_...
ui/views/cocoa/native_widget_mac_nswindow.mm:184: // Since this class implements
these selectors, |super| will always say they
On 2015/09/04 02:56:24, tapted wrote:
> Then perhaps this logic can move into commandDispatcher and be shared

Ah, I agree sharing this logic would be good. But we'd need to add
defaultValidateUserInterfaceItem to CommandDispatchingWindow... and I still
don't think CommandDispatcher really needs to be involved.

[tapted, 2015-09-04 12:19:05]

lgtm

[jackhou1, 2015-09-04 12:22:32]

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

[jackhou1, 2015-09-04 12:22:33]

sky, please review for OWNERS of:

chrome/browser/ui/views/frame/browser_frame_mac.mm
ui/base/BUILD.gn
ui/base/ui_base.gyp

[sky, 2015-09-04 14:59:58]

LGTM

[jackhou1, 2015-09-04 16:25:01]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-09-04 16:25:10]

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

[commit-bot: I haz the power, 2015-09-04 16:30:05]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-04 16:30:49]

Patchset 10 (id:??) landed as
https://crrev.com/ebb37f1c45a019e44352d3440898ed309130b75c
Cr-Commit-Position: refs/heads/master@{#347420}