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

10 files changed, 203 insertions, 25 deletions

diff --git a/chrome/app/nibs/MainMenu.xib b/chrome/app/nibs/MainMenu.xib
index 09fd8d3..247cd96 100644
--- a/chrome/app/nibs/MainMenu.xib
+++ b/chrome/app/nibs/MainMenu.xib
@@ -1450,14 +1450,6 @@
 					<object class="IBActionConnection" key="connection">
 						<string key="label">commandDispatch:</string>
 						<reference key="source" ref="1014"/>
-						<reference key="destination" ref="1059729334"/>
-					</object>
-					<int key="connectionID">528</int>
-				</object>
-				<object class="IBConnectionRecord">
-					<object class="IBActionConnection" key="connection">
-						<string key="label">commandDispatch:</string>
-						<reference key="source" ref="1014"/>
 						<reference key="destination" ref="1051826322"/>
 					</object>
 					<int key="connectionID">529</int>
@@ -1862,6 +1854,14 @@
 					</object>
 					<int key="connectionID">697</int>
 				</object>
+				<object class="IBConnectionRecord">
+					<object class="IBActionConnection" key="connection">
+						<string key="label">performClose:</string>
+						<reference key="source" ref="1014"/>
+						<reference key="destination" ref="1059729334"/>
+					</object>
+					<int key="connectionID">698</int>
+				</object>
 			</object>
 			<object class="IBMutableOrderedSet" key="objectRecords">
 				<object class="NSArray" key="orderedObjects">
@@ -3213,7 +3213,7 @@
 				</object>
 			</object>
 			<nil key="sourceID"/>
-			<int key="maxID">697</int>
+			<int key="maxID">698</int>
 		</object>
 		<object class="IBClassDescriber" key="IBDocument.Classes">
 			<object class="NSMutableArray" key="referencedPartialClassDescriptions">
diff --git a/chrome/app/nibs/main_menu_unittest.mm b/chrome/app/nibs/main_menu_unittest.mm
new file mode 100644
index 0000000..ec72d88
--- /dev/null
+++ b/chrome/app/nibs/main_menu_unittest.mm
@@ -0,0 +1,60 @@
+// 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/mac/foundation_util.h"
+#include "base/memory/scoped_nsobject.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "testing/platform_test.h"
+
+class MainMenuTest : public PlatformTest {
+ public:
+  // Recursively find the menu item with the given |tag| in |menu|.
+  NSMenuItem* FindMenuItemWithTag(NSMenu* menu, NSInteger tag) {
+    NSMenuItem* found = [menu itemWithTag:tag];
+    if (found)
+      return found;
+    NSMenuItem* item;
+    for (item in [menu itemArray]) {
+      if ([item hasSubmenu]) {
+        found = FindMenuItemWithTag([item submenu], tag);
+        if (found)
+          return found;
+      }
+    }
+    return nil;
+  }
+};
+
+
+TEST_F(MainMenuTest, CloseTabPerformClose) {
+  scoped_nsobject<NSNib> nib(
+      [[NSNib alloc] initWithNibNamed:@"MainMenu"
+                               bundle:base::mac::MainAppBundle()]);
+  EXPECT_TRUE(nib);
+
+  NSArray* objects = nil;
+  EXPECT_TRUE([nib instantiateNibWithOwner:nil
+                           topLevelObjects:&objects]);
+
+  // Check that "Close Tab" is mapped to -performClose:. This is needed to
+  // ensure the Lion dictionary pop up gets closed on Cmd-W, if it's open.
+  // See http://crbug.com/104931 for details.
+  BOOL found = NO;
+  for (NSUInteger i = 0; i < [objects count]; ++i) {
+    if ([[objects objectAtIndex:i] isKindOfClass:[NSMenu class]]) {
+      NSMenu* menu = [objects objectAtIndex:i];
+      NSMenuItem* closeTabItem = FindMenuItemWithTag(menu, IDC_CLOSE_TAB);
+      if (closeTabItem) {
+        EXPECT_EQ(@selector(performClose:), [closeTabItem action]);
+        found = YES;
+        break;
+      }
+    }
+  }
+  EXPECT_TRUE(found);
+  [objects makeObjectsPerformSelector:@selector(release)];
+  [NSApp setMainMenu:nil];
+}
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]);
+  }
+}
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 3e6ba06..04b5feb 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1229,7 +1229,8 @@
       'sources': [
         'app/breakpad_mac_stubs.mm',
         'app/chrome_dll.rc',
-        # All unittests in browser, common, renderer and service.
+        # All unittests in app, browser, common, renderer and service.
+        'app/nibs/main_menu_unittest.mm',
         'browser/about_flags_unittest.cc',
         'browser/accessibility/browser_accessibility_mac_unittest.mm',
         'browser/app_controller_mac_unittest.mm',