[Mac] Remove BrowserCommandExecutor protocol.

[Mac] Remove BrowserCommandExecutor protocol.

Only BrowserWindowController implements this. Other implementors just
turn it into a no-op. ChromeEventProcessingWindow (CEPW) now simply
checks whether it's a Browser window and executes the command.

This is the first step to factor out keyboard shortcut handling in CEPW into
a component that can be reused in Views as it removes one (of two)
dependencies on -[NSWindow windowController], which cannot be used
to implement browser-side behavior under Views.

Added new browser_test for shortcuts in global_keyboard_shortcuts_mac.

Some history:
BrowserCommandExecutor was added in r30619 (6 years ago) to allow a
non-browser window to execute browser commands, but became obsolete
a month later in r32445 when HtmlDialogWindowController (now
WebDialogWindowController) decided to ignore those commands instead.

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

[jackhou1, 2015-07-27 03:47:10]

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

[jackhou1, 2015-07-27 03:47:11]

tapted, PTAL

This is a pretty simple change that removes the use of -[NSWindow
windowController] in ChromeEventProcessingWindow. 

But, it might be better to keep BrowserCommandExecutor and later move
-validateUserInterfaceItem and -commandDispatch to it. This seems like a more
Cocoa-y use of NSWindowController, but it means that Views needs to accommodate
window controllers being set from the Browser side. WDYT?

Overview of all changes is here:
https://codereview.chromium.org/1259903002/

[tapted, 2015-07-27 04:58:57]

On 2015/07/27 03:47:11, jackhou1 wrote:
> This is a pretty simple change that removes the use of -[NSWindow
> windowController] in ChromeEventProcessingWindow. 

this seems reasonable - the CL description might need something stronger to
justify it though. Like, "removing this allows the <something> to do <blah>.
Otherwise it would have to do <foo>". It seems like a neat simplification
anyway, just seems to crave a bit more context.

> But, it might be better to keep BrowserCommandExecutor and later move
> -validateUserInterfaceItem and -commandDispatch to it. This seems like a more
> Cocoa-y use of NSWindowController, but it means that Views needs to
accommodate
> window controllers being set from the Browser side. WDYT?

I don't think we can support Views ever having to cater for NSWindowControllers
-- that slot is already taken up, and we shouldn't be involving client-supplied
ObjectiveC classes in a C++ API if we can avoid it. (i.e. currently client code
can supply an NSWindow subclass - doing more raises ownership/lifetime issues).

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/global_k...
File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right):

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:22: class
ScopedTabStripModelObserver : public TabStripModelObserver {
nit: class comment

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:62: LOG(INFO) <<
[NSStringFromClass([NSApp class]) UTF8String];
remove?

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

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:19: +
(NSEvent*)keyEventForWindow:(NSWindow*)window fromKeyEvent:(NSEvent*)event;
Should this be a function just declared in an anonymous namespace?

Or - probably better.. should it just be something like

- (NSEvent*)adoptOtherWindowKeyEvent:(NSEvent*)event;

since I think the only call is

[self keyEventForWindow:self ..

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:24: -
(BOOL)handleExtraKeyboardShortcut:(NSEvent*)event
Can this just be a function in an anonymous namespace now? (but also see below)

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:33: const unichar
keyCode = [event keyCode];
hrm. this should probably be `int`. [NSEvent keyCode] returns an unsigned short.
Probably fine to leave it.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:43: Browser* browser =
chrome::FindBrowserWithWindow(event.window);
Pretty sure that it's not guaranteed that event.window == self here. (e.g.
[PermissionBubbleWindow performKeyEquivalent:] passes [self parentWindow])

Probably keeping this an instance method, and passing `self` instead of
event.window would be closer to the prior code.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:122: if ([event
window] != self)
nit: needs curlies (if it stays this way)

[jackhou1, 2015-07-28 05:19:45]

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/global_k...
File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right):

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:22: class
ScopedTabStripModelObserver : public TabStripModelObserver {
On 2015/07/27 04:58:57, tapted wrote:
> nit: class comment

Done.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:62: LOG(INFO) <<
[NSStringFromClass([NSApp class]) UTF8String];
On 2015/07/27 04:58:57, tapted wrote:
> remove?

Done.

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

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:19: +
(NSEvent*)keyEventForWindow:(NSWindow*)window fromKeyEvent:(NSEvent*)event;
On 2015/07/27 04:58:57, tapted wrote:
> Should this be a function just declared in an anonymous namespace?
> 
> Or - probably better.. should it just be something like
> 
> - (NSEvent*)adoptOtherWindowKeyEvent:(NSEvent*)event;
> 
> since I think the only call is
> 
> [self keyEventForWindow:self ..

Made it anonymous.

This will be factored out into ChromeCommandDispatchDelegate, so the window will
be passed in.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:24: -
(BOOL)handleExtraKeyboardShortcut:(NSEvent*)event
On 2015/07/27 04:58:57, tapted wrote:
> Can this just be a function in an anonymous namespace now? (but also see
below)

Done.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:33: const unichar
keyCode = [event keyCode];
On 2015/07/27 04:58:57, tapted wrote:
> hrm. this should probably be `int`. [NSEvent keyCode] returns an unsigned
short.
> Probably fine to leave it.

Done.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:43: Browser* browser =
chrome::FindBrowserWithWindow(event.window);
On 2015/07/27 04:58:57, tapted wrote:
> Pretty sure that it's not guaranteed that event.window == self here. (e.g.
> [PermissionBubbleWindow performKeyEquivalent:] passes [self parentWindow])
> 
> Probably keeping this an instance method, and passing `self` instead of
> event.window would be closer to the prior code.

This is also code that we want to factor out, so changed to passing in the
desired window.

https://codereview.chromium.org/1250533003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:122: if ([event
window] != self)
On 2015/07/27 04:58:57, tapted wrote:
> nit: needs curlies (if it stays this way)

Done.

[tapted, 2015-07-28 06:57:10]

https://codereview.chromium.org/1250533003/diff/60001/chrome/browser/global_k...
File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right):

https://codereview.chromium.org/1250533003/diff/60001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:22: // Observes a
TabStripModel for a single ActiveTabChanged.
I think this needs to say why it's needed. It's not obvious to me why
chrome::NewTab and the performKeyEquivalent calls result in asynchronous
behavior

E.g. is it waiting for the renderer? or the window server? are there delayed
tasks?

Is it possible this isn't needed, and the test will be non-flaky merely with
base::RunLoop().RunUntilIdle()?

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

https://codereview.chromium.org/1250533003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:42: Browser* browser =
chrome::FindBrowserWithWindow(event.window);
event.window -> window

[jackhou1, 2015-07-29 00:33:53]

https://codereview.chromium.org/1250533003/diff/60001/chrome/browser/global_k...
File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right):

https://codereview.chromium.org/1250533003/diff/60001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:22: // Observes a
TabStripModel for a single ActiveTabChanged.
On 2015/07/28 06:57:10, tapted wrote:
> I think this needs to say why it's needed. It's not obvious to me why
> chrome::NewTab and the performKeyEquivalent calls result in asynchronous
> behavior
> 
> E.g. is it waiting for the renderer? or the window server? are there delayed
> tasks?
> 
> Is it possible this isn't needed, and the test will be non-flaky merely with
> base::RunLoop().RunUntilIdle()?

Ah, you're right. It seems to be fine without it.

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

https://codereview.chromium.org/1250533003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chrome_event_processing_window.mm:42: Browser* browser =
chrome::FindBrowserWithWindow(event.window);
On 2015/07/28 06:57:10, tapted wrote:
> event.window -> window

Oops. Done.

[tapted, 2015-07-29 01:18:06]

lgtm

For an extra-compelling CL description, I think you can say

"BrowserCommandExecutor was added 6 years ago in r30619 to allow a non-browser
window to execute browser commands, but became obsolete a month later in r32445
when HtmlDialogWindowController (now WebDialogWindowController) decided to
ignore those commands instead."

[jackhou1, 2015-07-29 01:23:37]

On 2015/07/29 01:18:06, tapted wrote:
> lgtm
> 
> For an extra-compelling CL description, I think you can say
> 
> "BrowserCommandExecutor was added 6 years ago in r30619 to allow a non-browser
> window to execute browser commands, but became obsolete a month later in
r32445
> when HtmlDialogWindowController (now WebDialogWindowController) decided to
> ignore those commands instead."

Done.

[jackhou1, 2015-07-29 01:24:56]

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

[jackhou1, 2015-07-29 01:24:56]

rsesek, please review for OWNERS in:

chrome/browser/

[tapted, 2015-07-30 00:20:14]

https://codereview.chromium.org/1250533003/diff/80001/chrome/browser/global_k...
File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right):

https://codereview.chromium.org/1250533003/diff/80001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:20:
IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, GlobalKeyboardShortcuts) {
oops - I missed this. There should probably be a `using
GlobalKeyboardShortcutsTest = ExtensionBrowserTest`, and here do

IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, SwitchTabsMac)

[Robert Sesek, 2015-07-30 01:34:29]

LGTM. Nice refactor!

[jackhou1, 2015-07-30 01:45:57]

https://codereview.chromium.org/1250533003/diff/80001/chrome/browser/global_k...
File chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm (right):

https://codereview.chromium.org/1250533003/diff/80001/chrome/browser/global_k...
chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm:20:
IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, GlobalKeyboardShortcuts) {
On 2015/07/30 00:20:14, tapted wrote:
> oops - I missed this. There should probably be a `using
> GlobalKeyboardShortcutsTest = ExtensionBrowserTest`, and here do
> 
> IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, SwitchTabsMac)

Done.

[jackhou1, 2015-07-30 01:46:12]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-07-30 01:46:13]

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

[commit-bot: I haz the power, 2015-07-30 01:46:38]

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

[commit-bot: I haz the power, 2015-07-30 03:11:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-30 03:12:00]

Patchset 6 (id:??) landed as
https://crrev.com/06c25a9881b76f7e0b0ab27050d8c470fe9bdc2d
Cr-Commit-Position: refs/heads/master@{#341062}