[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.

Review URL: https://codereview.chromium.org/1040993005

Cr-Commit-Position: refs/heads/master@{#323157}

4 files changed, 159 insertions, 26 deletions

diff --git a/chrome/browser/chrome_browser_application_mac.mm b/chrome/browser/chrome_browser_application_mac.mm
index 1203c38..e648774 100644
--- a/chrome/browser/chrome_browser_application_mac.mm
+++ b/chrome/browser/chrome_browser_application_mac.mm
@@ -651,27 +651,13 @@ void SwizzleInit() {
 }
 
 - (id)_removeWindow:(NSWindow*)window {
+  // Note _removeWindow is called from -[NSWindow dealloc], which can happen at
+  // unpredictable times due to reference counting. Just update state.
   {
     base::AutoLock lock(previousKeyWindowsLock_);
     [self removePreviousKeyWindow:window];
   }
-  id result = [super _removeWindow:window];
-
-  // Ensure app has a key window after a window is removed.
-  // OS wants to make a panel browser window key after closing an app window
-  // because panels use a higher priority window level, but panel windows may
-  // refuse to become key, leaving the app with no key window. The OS does
-  // not seem to consider other windows after the first window chosen refuses
-  // to become key. Force consideration of other windows here.
-  if ([self isActive] && [self keyWindow] == nil) {
-    NSWindow* key =
-        [self makeWindowsPerform:@selector(canBecomeKeyWindow) inOrder:YES];
-    [key makeKeyWindow];
-  }
-
-  // Return result from the super class. It appears to be the app that
-  // owns the removed window (determined via experimentation).
-  return result;
+  return [super _removeWindow:window];
 }
 
 - (id)_setKeyWindow:(NSWindow*)window {
diff --git a/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm b/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm
index b437f6b..d0cf828 100644
--- a/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm
+++ b/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm
@@ -373,9 +373,9 @@ void SetItemWithTagVisible(NSMenuItem* top_level_item,
   // http://crbug.com/406944.
   base::mac::ScopedNSAutoreleasePool pool;
 
-  id window = [notification object];
   NSString* name = [notification name];
   if ([name isEqualToString:NSWindowDidBecomeMainNotification]) {
+    id window = [notification object];
     extensions::AppWindow* appWindow =
         AppWindowRegistryUtil::GetAppWindowForNativeWindowAnyProfile(
             window);
@@ -394,14 +394,16 @@ void SetItemWithTagVisible(NSMenuItem* top_level_item,
     else
       [self removeMenuItems];
   } else if ([name isEqualToString:NSWindowWillCloseNotification]) {
-    // If there are any other windows that can become main, leave the menu. It
-    // will be changed when another window becomes main. Otherwise, restore the
-    // Chrome menu.
-    for (NSWindow* w : [NSApp windows]) {
-      if ([w canBecomeMainWindow] && ![w isEqual:window] && [w isOnActiveSpace])
-        return;
-    }
-
+    // Always reset back to the Chrome menu. This once scanned [NSApp windows]
+    // to predict whether we could expect another Chrome window to become main,
+    // and skip the reset. However, panels need to do strange things during
+    // window close to ensure panels never get chosen for key status over a
+    // browser window (which is likely because they are given an elevated
+    // [NSWindow level]). Trying to handle this case is not robust.
+    // Unfortunately, resetting the menu to Chrome unconditionally means that
+    // if another packaged app window becomes key, the menu will flicker.
+    // TODO(tapted): Investigate restoring the logic when the panel code is
+    // removed.
     [self removeMenuItems];
   } else {
     NOTREACHED();
diff --git a/chrome/browser/ui/cocoa/panels/panel_cocoa_browsertest.mm b/chrome/browser/ui/cocoa/panels/panel_cocoa_browsertest.mm
index 6f8cc1c..d0c82bd 100644
--- a/chrome/browser/ui/cocoa/panels/panel_cocoa_browsertest.mm
+++ b/chrome/browser/ui/cocoa/panels/panel_cocoa_browsertest.mm
@@ -4,14 +4,71 @@
 
 #import <Cocoa/Cocoa.h>
 
+#import "base/mac/scoped_nsobject.h"
+#include "base/run_loop.h"
+#include "base/test/test_timeouts.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/panels/base_panel_browser_test.h"
 #include "chrome/browser/ui/panels/panel.h"
 #include "content/public/test/test_utils.h"
 
+// Class that spins a run loop until an NSWindow gains key status.
+@interface CocoaActivationWaiter : NSObject {
+ @private
+  base::RunLoop* runLoop_;
+  BOOL observed_;
+}
+
+- (id)initWithWindow:(NSWindow*)window;
+- (void)windowDidBecomeKey:(NSNotification*)notification;
+- (BOOL)waitUntilActive;
+
+@end
+
+@implementation CocoaActivationWaiter
+
+- (id)initWithWindow:(NSWindow*)window {
+  EXPECT_FALSE([window isKeyWindow]);
+  if ((self = [super init])) {
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(windowDidBecomeKey:)
+               name:NSWindowDidBecomeKeyNotification
+             object:window];
+  }
+  return self;
+}
+
+- (void)dealloc {
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
+- (void)windowDidBecomeKey:(NSNotification*)notification {
+  observed_ = YES;
+  if (runLoop_)
+    runLoop_->Quit();
+}
+
+- (BOOL)waitUntilActive {
+  if (observed_)
+    return YES;
+
+  base::RunLoop runLoop;
+  base::MessageLoop::current()->task_runner()->PostDelayedTask(
+      FROM_HERE, runLoop.QuitClosure(), TestTimeouts::action_timeout());
+  runLoop_ = &runLoop;
+  runLoop.Run();
+  runLoop_ = nullptr;
+  return observed_;
+}
+
+@end
+
 typedef BasePanelBrowserTest PanelCocoaBrowserTest;
 
 IN_PROC_BROWSER_TEST_F(PanelCocoaBrowserTest, MenuItems) {
@@ -83,3 +140,39 @@ IN_PROC_BROWSER_TEST_F(PanelCocoaBrowserTest, MenuItems) {
 
   panel->Close();
 }
+
+// Test that panels do not become active when closing a window, even when a
+// panel is otherwise the topmost window.
+IN_PROC_BROWSER_TEST_F(PanelCocoaBrowserTest, InactivePanelsNotActivated) {
+  // Note CreateDockedPanel() sets wait_for_fully_created and SHOW_AS_ACTIVE,
+  // so the following spins a run loop until the respective panel is activated.
+  Panel* docked_panel_1 = CreateDockedPanel("Panel1", gfx::Rect());
+  EXPECT_TRUE([docked_panel_1->GetNativeWindow() isKeyWindow]);
+
+  // Activate the browser. Otherwise closing the second panel will correctly
+  // raise the first panel since panels are allowed to become key if they were
+  // actually the most recently focused window (as opposed to being merely the
+  // topmost window on the z-order stack).
+  NSWindow* browser_window = browser()->window()->GetNativeWindow();
+  base::scoped_nsobject<CocoaActivationWaiter> waiter(
+      [[CocoaActivationWaiter alloc] initWithWindow:browser_window]);
+  browser()->window()->Activate();
+  EXPECT_TRUE([waiter waitUntilActive]);
+
+  // Creating a second panel will activate it (and make it topmost).
+  Panel* docked_panel_2 = CreateDockedPanel("Panel2", gfx::Rect());
+  EXPECT_TRUE([docked_panel_2->GetNativeWindow() isKeyWindow]);
+
+  // Verify the assumptions that the panels are actually topmost.
+  EXPECT_EQ(docked_panel_2->GetNativeWindow(),
+            [[NSApp orderedWindows] objectAtIndex:0]);
+  EXPECT_EQ(docked_panel_1->GetNativeWindow(),
+            [[NSApp orderedWindows] objectAtIndex:1]);
+
+  // Close the second panel and wait for the browser to become active.
+  waiter.reset([[CocoaActivationWaiter alloc] initWithWindow:browser_window]);
+  docked_panel_2->Close();
+
+  EXPECT_TRUE([waiter waitUntilActive]);
+  EXPECT_TRUE([browser_window isKeyWindow]);
+}
diff --git a/chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm b/chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm
index 9d96a83..06ce789 100644
--- a/chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm
+++ b/chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm
@@ -46,6 +46,11 @@ const double kBoundsAnimationMaxDurationSeconds = 0.18;
 // Edge thickness to trigger user resizing via system, in screen pixels.
 const double kWidthOfMouseResizeArea = 15.0;
 
+// Notification observer to prevent panels becoming key when windows are closed.
+@interface WindowCloseWatcher : NSObject
+- (void)windowWillClose:(NSNotification*)notification;
+@end
+
 @interface PanelWindowControllerCocoa (PanelsCanBecomeKey)
 // Internal helper method for extracting the total number of panel windows
 // from the panel manager. Used to decide if panel can become the key window.
@@ -88,6 +93,7 @@ const double kWidthOfMouseResizeArea = 15.0;
   return ([app isHandlingSendEvent]  && [[app currentEvent] window] == self) ||
       [controller activationRequestedByPanel] ||
       [app isCyclingWindows] ||
+      [self isKeyWindow] ||
       [app previousKeyWindow] == self ||
       [[app windows] count] == static_cast<NSUInteger>([controller numPanels]);
 }
@@ -168,6 +174,10 @@ const double kWidthOfMouseResizeArea = 15.0;
     animateOnBoundsChange_ = YES;
     canBecomeKeyWindow_ = YES;
     activationRequestedByPanel_ = NO;
+
+    // Leaky singleton. Initialized when the first panel is created.
+    static WindowCloseWatcher* watcher = [[WindowCloseWatcher alloc] init];
+    (void)watcher;  // Suppress the unused variable warning.
   }
   return self;
 }
@@ -927,3 +937,45 @@ const double kWidthOfMouseResizeArea = 15.0;
 }
 
 @end
+
+@implementation WindowCloseWatcher
+
+- (id)init {
+  if ((self = [super init])) {
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(windowWillClose:)
+               name:NSWindowWillCloseNotification
+             object:nil];
+  }
+  return self;
+}
+
+- (void)windowWillClose:(NSNotification*)notification {
+  // If it looks like a panel may (refuse to) become key after this window is
+  // closed, then explicitly set the topmost browser window on the active space
+  // to be key (if there is one). Otherwise AppKit will stop looking for windows
+  // to make key once it encounters the panel.
+  id closingWindow = [notification object];
+  BOOL orderNext = NO;
+  for (NSWindow* window : [NSApp orderedWindows]) {
+    if ([window isEqual:closingWindow] || ![window isOnActiveSpace])
+      continue;
+
+    if ([window isKindOfClass:[PanelWindowCocoaImpl class]] &&
+        ![window canBecomeKeyWindow]) {
+      orderNext = YES;
+      continue;
+    }
+
+    if (orderNext) {
+      if (![window canBecomeKeyWindow])
+        continue;
+
+      [window makeKeyWindow];
+    }
+    return;
+  }
+}
+
+@end