Fix Lion dictionary popup staying after tab-close via cmd-W.

This was caused by the close menu item not sending a -[performClose:],
which was what the popup was looking for.

Adds some unit tests and refactors some code to make it more testable.

BUG=104931
TEST=Open a tab and double 3-finger tap on a word to bring up the
dictionary popup. Hit cmd-W. The popup should close but they tab
should stay open. Hit cmd-W again. The tab should close.
Try cmd-shift-W with > 1 tab open. The window should close.

Review URL: http://codereview.chromium.org/9026016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@116144 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 132 insertions, 15 deletions

diff --git a/chrome/browser/app_controller_mac.h b/chrome/browser/app_controller_mac.h
index 5f24f86..e32e478 100644
--- a/chrome/browser/app_controller_mac.h
+++ b/chrome/browser/app_controller_mac.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -68,6 +68,9 @@ class Profile;
 @property(readonly, nonatomic) BOOL startupComplete;
 @property(readonly, nonatomic) Profile* lastProfile;
 
+// Registers for various event handlers and performs initialization.
+- (void)registerEventHandlersAndInitialize;
+
 - (void)didEndMainMessageLoop;
 
 // Try to close all browser windows, and if that succeeds then quit.
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index e94ee26..9f7e755 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -193,6 +193,13 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
 // the profile is loaded or any preferences have been registered). Defer any
 // user-data initialization until -applicationDidFinishLaunching:.
 - (void)awakeFromNib {
+}
+
+// This method is called very early in application startup (ie, before
+// the profile is loaded or any preferences have been registered), just
+// after -awakeFromNib. This is separate from -awakeFromNib: so that
+// test code can load nibs without these side effects.
+- (void)registerEventHandlersAndInitialize {
   // We need to register the handlers early to catch events fired on launch.
   NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
   [em setEventHandler:self
@@ -250,6 +257,27 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
   [self initProfileMenu];
 }
 
+- (void)unregisterEventHandlers {
+  NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
+  [em removeEventHandlerForEventClass:kInternetEventClass
+                           andEventID:kAEGetURL];
+  [em removeEventHandlerForEventClass:cloud_print::kAECloudPrintClass
+                           andEventID:cloud_print::kAECloudPrintClass];
+  [em removeEventHandlerForEventClass:kAECloudPrintInstallClass
+                           andEventID:kAECloudPrintInstallClass];
+  [em removeEventHandlerForEventClass:kAECloudPrintUninstallClass
+                           andEventID:kAECloudPrintUninstallClass];
+  [em removeEventHandlerForEventClass:'WWW!'
+                           andEventID:'OURL'];
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+- (void)dealloc {
+  if ([NSApp delegate] == self)
+    [NSApp setDelegate:nil];
+  [super dealloc];
+}
+
 // (NSApplicationDelegate protocol) This is the Apple-approved place to override
 // the default handlers.
 - (void)applicationWillFinishLaunching:(NSNotification*)notification {
@@ -322,14 +350,8 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
 
 // Called when the app is shutting down. Clean-up as appropriate.
 - (void)applicationWillTerminate:(NSNotification*)aNotification {
-  NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
-  [em removeEventHandlerForEventClass:kInternetEventClass
-                           andEventID:kAEGetURL];
-  [em removeEventHandlerForEventClass:'WWW!'
-                           andEventID:'OURL'];
-
   // There better be no browser windows left at this point.
-  CHECK_EQ(BrowserList::size(), 0u);
+  CHECK_EQ(0u, BrowserList::size());
 
   // Tell BrowserList not to keep the browser process alive. Once all the
   // browsers get dealloc'd, it will stop the RunLoop and fall back into main().
@@ -338,7 +360,7 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
   // Close these off if they have open windows.
   [aboutController_ close];
 
-  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [self unregisterEventHandlers];
 }
 
 - (void)didEndMainMessageLoop {
diff --git a/chrome/browser/chrome_browser_main_mac.mm b/chrome/browser/chrome_browser_main_mac.mm
index b8aaf29..af65702 100644
--- a/chrome/browser/chrome_browser_main_mac.mm
+++ b/chrome/browser/chrome_browser_main_mac.mm
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -102,6 +102,8 @@ void ChromeBrowserMainPartsMac::PreMainMessageLoopStart() {
   [nib instantiateNibWithOwner:NSApp topLevelObjects:nil];
   // Make sure the app controller has been created.
   DCHECK([NSApp delegate]);
+  DCHECK([[NSApp delegate] isKindOfClass:[AppController class]]);
+  [[NSApp delegate] registerEventHandlersAndInitialize];
 
   // This is a no-op if the KeystoneRegistration framework is not present.
   // The framework is only distributed with branded Google Chrome builds.
diff --git a/chrome/browser/ui/cocoa/browser_window_controller.h b/chrome/browser/ui/cocoa/browser_window_controller.h
index 324351f..7137578 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.h
+++ b/chrome/browser/ui/cocoa/browser_window_controller.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -261,6 +261,9 @@ class TabContents;
 // The user changed the theme.
 - (void)userChangedTheme;
 
+// Called when the user picks a menu or toolbar item when this window is key.
+- (void)commandDispatch:(id)sender;
+
 // Executes the command in the context of the current browser.
 // |command| is an integer value containing one of the constants defined in the
 // "chrome/app/chrome_command_ids.h" file.
diff --git a/chrome/browser/ui/cocoa/chrome_browser_window.h b/chrome/browser/ui/cocoa/chrome_browser_window.h
index aa19560..7009c82 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window.h
+++ b/chrome/browser/ui/cocoa/chrome_browser_window.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -22,6 +22,11 @@
 - (void)underlaySurfaceAdded;
 - (void)underlaySurfaceRemoved;
 
+
+// Indicates whether -performClose: with the given |sender| should be routed
+// to -commandDispatch: on the delegate.
+- (BOOL)performCloseShouldRouteToCommandDispatch:(id)sender;
+
 @end
 
 #endif  // CHROME_BROWSER_UI_COCOA_CHROME_BROWSER_WINDOW_H_
diff --git a/chrome/browser/ui/cocoa/chrome_browser_window.mm b/chrome/browser/ui/cocoa/chrome_browser_window.mm
index e21507b..9fa696a 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window.mm
+++ b/chrome/browser/ui/cocoa/chrome_browser_window.mm
@@ -1,10 +1,12 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/chrome_browser_window.h"
 
 #include "base/logging.h"
+#include "chrome/app/chrome_command_ids.h"
+#import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
 #include "ui/base/theme_provider.h"
 
@@ -49,4 +51,25 @@
   return [delegate themePatternPhase];
 }
 
+- (BOOL)performCloseShouldRouteToCommandDispatch:(id)sender {
+  return [[self delegate] respondsToSelector:@selector(commandDispatch:)] &&
+         [sender isKindOfClass:[NSMenuItem class]] &&
+         [sender tag] == IDC_CLOSE_TAB;
+}
+
+- (void)performClose:(id)sender {
+  // Route -performClose: to -commandDispatch: on the delegate when coming from
+  // the "close tab" menu item. This is done here, rather than simply connecting
+  // the menu item to -commandDispatch: in IB because certain parts of AppKit,
+  // such as the Lion dictionary pop up, expect Cmd-W to send -performClose:.
+  // See http://crbug.com/104931 for details.
+  if ([self performCloseShouldRouteToCommandDispatch:sender]) {
+    id delegate = [self delegate];
+    [delegate commandDispatch:sender];
+    return;
+  }
+
+  [super performClose:sender];
+}
+
 @end
diff --git a/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm b/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
index 196dc74..f65ba67 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
+++ b/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
@@ -1,16 +1,34 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import <Cocoa/Cocoa.h>
 
 #include "base/debug/debugger.h"
+#include "chrome/app/chrome_command_ids.h"
 #import "chrome/browser/ui/cocoa/chrome_browser_window.h"
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #import "testing/gtest_mac.h"
 #include "testing/platform_test.h"
 
+// An NSWindowDelegate that implements -commandDispatch:.
+@interface TestDelegateWithCommandDispatch : NSObject {
+}
+- (void)commandDispatch:(id)sender;
+@end
+@implementation TestDelegateWithCommandDispatch
+- (void)commandDispatch:(id)sender {
+}
+@end
+
+// An NSWindowDelegate that doesn't implement -commandDispatch:.
+@interface TestDelegateWithoutCommandDispatch : NSObject {
+}
+@end
+@implementation TestDelegateWithoutCommandDispatch
+@end
+
 class ChromeBrowserWindowTest : public CocoaTest {
  public:
   virtual void SetUp() {
@@ -43,3 +61,44 @@ class ChromeBrowserWindowTest : public CocoaTest {
 TEST_F(ChromeBrowserWindowTest, ShowAndClose) {
   [window_ display];
 }
+
+// Tests routing of -performClose:.
+TEST_F(ChromeBrowserWindowTest, PerformCloseTest) {
+  scoped_nsobject<NSMenuItem> zoom_item(
+      [[NSMenuItem alloc] initWithTitle:@"Zoom"
+                                 action:@selector(performZoom:)
+                          keyEquivalent:@""]);
+  scoped_nsobject<NSMenuItem> close_item(
+      [[NSMenuItem alloc] initWithTitle:@"Close"
+                                 action:@selector(performClose:)
+                          keyEquivalent:@""]);
+  scoped_nsobject<NSMenuItem> close_tab_item(
+      [[NSMenuItem alloc] initWithTitle:@"Close Tab"
+                                 action:@selector(performClose:)
+                          keyEquivalent:@""]);
+  [close_tab_item setTag:IDC_CLOSE_TAB];
+
+  scoped_nsobject<id> delegate1(
+      [[TestDelegateWithCommandDispatch alloc] init]);
+  scoped_nsobject<id> delegate2(
+      [[TestDelegateWithoutCommandDispatch alloc] init]);
+
+  struct {
+    id delegate;
+    id sender;
+    BOOL should_route;
+  } cases[] = {
+    { delegate1, delegate1, NO },
+    { delegate1, window_, NO },
+    { delegate1, zoom_item, NO },
+    { delegate1, close_item, NO },
+    { delegate1, close_tab_item, YES },
+    { delegate2, close_tab_item, NO },
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); ++i) {
+    [window_ setDelegate:cases[i].delegate];
+    EXPECT_EQ(cases[i].should_route,
+        [window_ performCloseShouldRouteToCommandDispatch:cases[i].sender]);
+  }
+}