Better fix for Lion dictionary popover cmd-W bug.

This reverts http://crrev.com/117681 and http://crrev.com/104931 and instead uses new 10.7 notifications to change the shortcuts on Close Tab and Close Window items, so that cmd-W does a "Close Window" when the dictionary is open.

The above is also how Safari appears to solve this issue, as can be seen by going to the File menu when the dictionary popover is up.

BUG=104931, 110306, 109061
TEST=1. 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 the tab
should stay open. Hit cmd-W again. The tab should close.
2. Try cmd-shift-W with > 1 tab open. The window should close.
3. Open a window that doesn't have tabs (e.g. Chrome -> About
Chrome). File -> Close Tab should be disabled.

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

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

12 files changed, 60 insertions, 199 deletions

diff --git a/chrome/app/nibs/MainMenu.xib b/chrome/app/nibs/MainMenu.xib
index 247cd96..09fd8d3 100644
--- a/chrome/app/nibs/MainMenu.xib
+++ b/chrome/app/nibs/MainMenu.xib
@@ -1450,6 +1450,14 @@
 					<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>
@@ -1854,14 +1862,6 @@
 					</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">698</int>
+			<int key="maxID">697</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
deleted file mode 100644
index ec72d88..0000000
--- a/chrome/app/nibs/main_menu_unittest.mm
+++ /dev/null
@@ -1,60 +0,0 @@
-// 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 e32e478..b780071 100644
--- a/chrome/browser/app_controller_mac.h
+++ b/chrome/browser/app_controller_mac.h
@@ -63,14 +63,14 @@ class Profile;
 
   // Outlet for the tabpose menu item so we can hide it.
   IBOutlet NSMenuItem* tabposeMenuItem_;
+
+  // Indicates wheter an NSPopover is currently being shown.
+  BOOL hasPopover_;
 }
 
 @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 cf82cba..000bc5c 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -89,6 +89,13 @@ using content::UserMetricsAction;
 
 namespace {
 
+// Declare notification names from the 10.7 SDK.
+#if !defined(MAC_OS_X_VERSION_10_7) || \
+    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
+NSString* NSPopoverDidShowNotification = @"NSPopoverDidShowNotification";
+NSString* NSPopoverDidCloseNotification = @"NSPopoverDidCloseNotification";
+#endif
+
 // True while AppController is calling Browser::OpenEmptyWindow(). We need a
 // global flag here, analogue to BrowserInit::InProcessStartup() because
 // otherwise the SessionService will try to restore sessions when we make a new
@@ -192,13 +199,6 @@ 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
@@ -249,6 +249,19 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
              name:NSWindowDidResignMainNotification
            object:nil];
 
+  if (base::mac::IsOSLionOrLater()) {
+    [notificationCenter
+        addObserver:self
+           selector:@selector(popoverDidShow:)
+               name:NSPopoverDidShowNotification
+             object:nil];
+    [notificationCenter
+        addObserver:self
+           selector:@selector(popoverDidClose:)
+               name:NSPopoverDidCloseNotification
+             object:nil];
+  }
+
   // Set up the command updater for when there are no windows open
   [self initMenuState];
 
@@ -271,12 +284,6 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
   [[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 {
@@ -377,15 +384,16 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
 // If the window has a tab controller, make "close window" be cmd-shift-w,
 // otherwise leave it as the normal cmd-w. Capitalization of the key equivalent
 // affects whether the shift modifer is used.
-- (void)adjustCloseWindowMenuItemKeyEquivalent:(BOOL)hasTabs {
-  [closeWindowMenuItem_ setKeyEquivalent:(hasTabs ? @"W" : @"w")];
+- (void)adjustCloseWindowMenuItemKeyEquivalent:(BOOL)enableCloseTabShortcut {
+  [closeWindowMenuItem_ setKeyEquivalent:(enableCloseTabShortcut ? @"W" :
+                                                                   @"w")];
   [closeWindowMenuItem_ setKeyEquivalentModifierMask:NSCommandKeyMask];
 }
 
 // If the window has a tab controller, make "close tab" take over cmd-w,
 // otherwise it shouldn't have any key-equivalent because it should be disabled.
-- (void)adjustCloseTabMenuItemKeyEquivalent:(BOOL)hasTabs {
-  if (hasTabs) {
+- (void)adjustCloseTabMenuItemKeyEquivalent:(BOOL)enableCloseTabShortcut {
+  if (enableCloseTabShortcut) {
     [closeTabMenuItem_ setKeyEquivalent:@"w"];
     [closeTabMenuItem_ setKeyEquivalentModifierMask:NSCommandKeyMask];
   } else {
@@ -423,8 +431,9 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
 
   BOOL hasTabs =
       [[window windowController] isKindOfClass:[TabWindowController class]];
-  [self adjustCloseWindowMenuItemKeyEquivalent:hasTabs];
-  [self adjustCloseTabMenuItemKeyEquivalent:hasTabs];
+  BOOL enableCloseTabShortcut = hasTabs && !hasPopover_;
+  [self adjustCloseWindowMenuItemKeyEquivalent:enableCloseTabShortcut];
+  [self adjustCloseTabMenuItemKeyEquivalent:enableCloseTabShortcut];
 }
 
 // Fix up the "close tab/close window" command-key equivalents. We do this
@@ -487,6 +496,18 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
   }
 }
 
+// Called on Lion and later when a popover (e.g. dictionary) is shown.
+- (void)popoverDidShow:(NSNotification*)notify {
+  hasPopover_ = YES;
+  [self fixCloseMenuItemKeyEquivalents];
+}
+
+// Called on Lion and later when a popover (e.g. dictionary) is closed.
+- (void)popoverDidClose:(NSNotification*)notify {
+  hasPopover_ = NO;
+  [self fixCloseMenuItemKeyEquivalents];
+}
+
 // Called when the user has changed browser windows, meaning the backing profile
 // may have changed. This can cause a rebuild of the user-data menus. This is a
 // no-op if the new profile is the same as the current one. This will always be
@@ -1109,7 +1130,7 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
 // process, gets the required data and launches Print dialog.
 - (void)submitCloudPrintJob:(NSAppleEventDescriptor*)event {
   // Pull parameter list out of Apple Event.
-  NSAppleEventDescriptor *paramList =
+  NSAppleEventDescriptor* paramList =
       [event paramDescriptorForKeyword:cloud_print::kAECloudPrintClass];
 
   if (paramList != nil) {
@@ -1282,7 +1303,7 @@ const AEEventClass kAECloudPrintUninstallClass = 'GCPu';
   return bookmarkMenuBridge_.get();
 }
 
-- (void)applicationDidChangeScreenParameters:(NSNotification *)notification {
+- (void)applicationDidChangeScreenParameters:(NSNotification*)notification {
   // During this callback the working area is not always already updated. Defer.
   [self performSelector:@selector(delayedPanelManagerScreenParametersUpdate)
              withObject:nil
diff --git a/chrome/browser/chrome_browser_main_mac.mm b/chrome/browser/chrome_browser_main_mac.mm
index af65702..b767e22 100644
--- a/chrome/browser/chrome_browser_main_mac.mm
+++ b/chrome/browser/chrome_browser_main_mac.mm
@@ -102,8 +102,6 @@ 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 43d23fb..0c39fd3 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.h
+++ b/chrome/browser/ui/cocoa/browser_window_controller.h
@@ -263,9 +263,6 @@ class WebContents;
 // 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/browser_window_controller.mm b/chrome/browser/ui/cocoa/browser_window_controller.mm
index 64d7774..8335b28 100644
--- a/chrome/browser/ui/cocoa/browser_window_controller.mm
+++ b/chrome/browser/ui/cocoa/browser_window_controller.mm
@@ -972,8 +972,8 @@ enum {
 }
 
 // Called to validate menu and toolbar items when this window is key. All the
-// items we care about have been set with the -performClose:, -commandDispatch:
-// or -commandDispatchUsingKeyModifiers: actions and a target of FirstResponder
+// items we care about have been set with the |-commandDispatch:| or
+// |-commandDispatchUsingKeyModifiers:| actions and a target of FirstResponder
 // in IB. If it's not one of those, let it continue up the responder chain to be
 // handled elsewhere. We pull out the tag as the cross-platform constant to
 // differentiate and dispatch the various commands.
@@ -984,8 +984,7 @@ enum {
 - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
   SEL action = [item action];
   BOOL enable = NO;
-  if (action == @selector(performClose:) ||
-      action == @selector(commandDispatch:) ||
+  if (action == @selector(commandDispatch:) ||
       action == @selector(commandDispatchUsingKeyModifiers:)) {
     NSInteger tag = [item tag];
     if (browser_->command_updater()->SupportsCommand(tag)) {
diff --git a/chrome/browser/ui/cocoa/chrome_browser_window.h b/chrome/browser/ui/cocoa/chrome_browser_window.h
index 7009c82..6ea2f86 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window.h
+++ b/chrome/browser/ui/cocoa/chrome_browser_window.h
@@ -22,11 +22,6 @@
 - (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 9fa696a..c5f1fab 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window.mm
+++ b/chrome/browser/ui/cocoa/chrome_browser_window.mm
@@ -5,8 +5,6 @@
 #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"
 
@@ -51,25 +49,4 @@
   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 f65ba67..ae43983 100644
--- a/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
+++ b/chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm
@@ -12,23 +12,6 @@
 #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() {
@@ -62,43 +45,3 @@ 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/browser/ui/cocoa/fullscreen_window.mm b/chrome/browser/ui/cocoa/fullscreen_window.mm
index b5b7160a..9125b21 100644
--- a/chrome/browser/ui/cocoa/fullscreen_window.mm
+++ b/chrome/browser/ui/cocoa/fullscreen_window.mm
@@ -4,7 +4,6 @@
 
 #import "chrome/browser/ui/cocoa/fullscreen_window.h"
 
-#import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
 
 @implementation FullscreenWindow
@@ -73,17 +72,10 @@
 // We need our own version, since the default one wants to flash the close
 // button (and possibly other things), which results in nothing happening.
 - (void)performClose:(id)sender {
-  id delegate = [self delegate];
-
-  // Route -performClose: to -commandDispatch: on the delegate when coming from
-  // the "close tab" menu item. See comment in chrome_browser_window.mm.
-  if ([self performCloseShouldRouteToCommandDispatch:sender]) {
-    [delegate commandDispatch:sender];
-    return;
-  }
+  BOOL shouldClose = YES;
 
   // If applicable, check if this window should close.
-  BOOL shouldClose = YES;
+  id delegate = [self delegate];
   if ([delegate respondsToSelector:@selector(windowShouldClose:)])
     shouldClose = [delegate windowShouldClose:self];
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 9c75f70..6da020b 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1248,8 +1248,7 @@
       'sources': [
         'app/breakpad_mac_stubs.mm',
         'app/chrome_dll.rc',
-        # All unittests in app, browser, common, renderer and service.
-        'app/nibs/main_menu_unittest.mm',
+        # All unittests in browser, common, renderer and service.
         'browser/about_flags_unittest.cc',
         'browser/accessibility/browser_accessibility_mac_unittest.mm',
         'browser/app_controller_mac_unittest.mm',