[Mac] A more robust way to ensure panels avoid key status on window close

[Mac] A more robust way to ensure panels avoid key status on window close

r127517 added a private function override to [NSApplication
_removeWindow:] to ensure that closing a window would not raise a docked
panel (i.e Hangouts). However, [NSApp _removeWindow] is just called from
[NSWindow dealloc] which can happen at unpredictable times, and might
not actually be associated with a window closing. It's uncertain whether
it's safe to call [NSWindow makeKeyWindow] at this point and the logic
seems somehow connected to the rather nasty bug in
http://crbug.com/459306

This CL implements an alternative that just starts observing
NSWindowWillCloseNotification for all windows via a leaky singleton.
It's created only after the first panel is created, to ensure the
behaviour is avoided completely when panels are not used. It also adds a
check to ensure the window forced to become key is on the active space,
which was missing.

AppShimMenuController also observes NSWindowWillCloseNotification and
never accounted for the weirdness that panels do on window close. So,
simplify the logic that sets the mainmenu back to Chrome Browser items
when closing a packaged app window. This is to ensure the observers
don't randomly fight, since they both care about the contents of [NSApp
orderedWindows]. This regresses a fix for flickering menus added in
r224710, but that's minor compared to http://crbug.com/459306

As a bonus, this crazy logic gets a test, in interactive_ui_tests.

BUG=459306
TEST=Open a browser, open the hangouts extension, dock the chat window
at the bottom of the screen, Open a second browser window. Now close
that second browser window, the first browser window should activate,
and the omnibox in that first browser should have a focus ring.

Committed: https://crrev.com/19fbc1adba623e695d07d2a2009d5fa66b1f5677
Cr-Commit-Position: refs/heads/master@{#323157}

[tapted, 2015-03-31 09:23:20]

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

[tapted, 2015-03-31 11:04:29]

Patchset #4 (id:60001) has been deleted

[tapted, 2015-03-31 11:06:08]

Patchset #4 (id:80001) has been deleted

[tapted, 2015-03-31 11:21:05]

tapted@chromium.org changed reviewers:
+ shess@chromium.org
- jackhou@chromium.org, mark@chromium.org

[tapted, 2015-03-31 11:21:55]

tapted@chromium.org changed reviewers:
+ jackhou@chromium.org, mark@chromium.org
- shess@chromium.org

[tapted, 2015-03-31 11:34:10]

Hi Mark... please take a look. This is to fix a nasty bug. Some details on
http://crbug.com/459306 but unfortunately no repro case - this is my best guess
(rationale in the CL description)

jack - PTAL at the app_shim_menu_controller_mac change

https://codereview.chromium.org/1040993005/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (right):

https://codereview.chromium.org/1040993005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:103: [self
isKeyWindow] ||
This case is encountered when there are multiple panels, a panel is active (i.e.
self) and another panel is closed. `app previousKeyWindow` hasn't yet been
updated for the check below, because the panel being *closed* hasn't become key
(which would make `self` previous). It wasn't an issue before because the
_removeWindow hook happened later, or (in tests) not at all. Now that the
trigger is more deterministic, a few cross-platform
{Docked,Stacked}PanelBrowserTests tickled this.

[Mark Mentovai, 2015-03-31 13:13:31]

LGTM, although it would be good to find a way to not have a flickering menu.

https://codereview.chromium.org/1040993005/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (right):

https://codereview.chromium.org/1040993005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:185: if
(!g_window_close_watcher)
Doesn’t need to be global. You can just write

  static WindowCloseWatcher* watcher = [[WindowCloseWatcher alloc] init];

right here.

[jackhou1, 2015-03-31 21:50:04]

lgtm

[tapted, 2015-03-31 23:39:16]

Patchset #5 (id:120001) has been deleted

[tapted, 2015-03-31 23:55:30]

On 2015/03/31 13:13:31, Mark Mentovai wrote:
> LGTM, although it would be good to find a way to not have a flickering menu.

I'll file a bug to track. It's something of a corner-case, since it's uncommon
for packaged apps to have multiple windows, or for multiple packaged apps to be
used. And this only affects when a window is closed rather than just switching
between them.

And there's already a bug for deprecating panels - http://crbug.com/467808 .
They basically never made it to stable except for hangouts via a whitelist.
They're not a feature we want to maintain for lots of reasons, except maybe on
ChromeOS. I'm pining for the day we can finally delete all that code...

https://codereview.chromium.org/1040993005/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (right):

https://codereview.chromium.org/1040993005/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:185: if
(!g_window_close_watcher)
On 2015/03/31 13:13:31, Mark Mentovai wrote:
> Doesn’t need to be global. You can just write
> 
>   static WindowCloseWatcher* watcher = [[WindowCloseWatcher alloc] init];
> 
> right here.

Ooh - I didn't think of that. It results in

`error: unused variable 'watcher'`

But, a `(void)watcher;` on the line following suppresses the warning. That's not
exactly nice, but I think it's nicer than the global. (Done.)

[tapted, 2015-04-01 00:50:07]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-04-01 00:50:07]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
jackhou@chromium.org
Link to the patchset: https://codereview.chromium.org/1040993005/#ps140001
(title: "static")

[commit-bot: I haz the power, 2015-04-01 00:50:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1040993005/140001

[commit-bot: I haz the power, 2015-04-01 00:54:08]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-04-01 00:54:56]

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

[Mark Mentovai, 2015-04-01 02:13:59]

There's ALLOW_UNUSED_LOCAL for this situation too.

[tapted, 2015-04-01 02:21:39]

On 2015/04/01 02:13:59, Mark Mentovai wrote:
> There's ALLOW_UNUSED_LOCAL for this situation too.

ah, that is nicer - I'll remember that. After reading the discussion about it at
https://codereview.chromium.org/650393002/diff/80001/base/compiler_specific.h...
I don't think it's worth updating, but let me know if you think otherwise.
Thanks!

[Mark Mentovai, 2015-04-01 03:51:32]

You don’t have to change what you’ve checked in if you don’t want to. I just
wanted to point out the macro for the future. It’s nice because it has better
documentary value than (void).