Fix for parent window closing behind the bookmark bubble.

BUG=27752
TEST=none

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

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

5 files changed, 146 insertions, 142 deletions

diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.h b/chrome/browser/cocoa/bookmark_bubble_controller.h
index c8bfd0b..fe89d08 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.h
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.h
@@ -10,24 +10,12 @@ class BookmarkModel;
 class BookmarkNode;
 @class BookmarkBubbleController;
 
-// Protocol for a BookmarkBubbleController's (BBC's) delegate.
-@protocol BookmarkBubbleControllerDelegate
-
-// The bubble asks the delegate to perform an edit when needed.
-- (void)editBookmarkNode:(const BookmarkNode*)node;
-
-// The bubble tells the delegate when it will go away.
-- (void)bubbleWindowWillClose:(NSWindow*)window;
-
-@end
-
 // Controller for the bookmark bubble.  The bookmark bubble is a
 // bubble that pops up when clicking on the STAR next to the URL to
 // add or remove it as a bookmark.  This bubble allows for editing of
 // the bookmark in various ways (name, folder, etc.)
 @interface BookmarkBubbleController : NSWindowController<NSWindowDelegate> {
  @private
-  id<BookmarkBubbleControllerDelegate> delegate_;  // weak like other delegates
   NSWindow* parentWindow_;  // weak
   NSPoint topLeftForBubble_;
 
@@ -42,24 +30,27 @@ class BookmarkNode;
   IBOutlet NSPopUpButton* folderPopUpButton_;
 }
 
+@property (readonly, nonatomic) const BookmarkNode* node;
+
 // |node| is the bookmark node we edit in this bubble.
 // |alreadyBookmarked| tells us if the node was bookmarked before the
 //   user clicked on the star.  (if NO, this is a brand new bookmark).
 // The owner of this object is responsible for showing the bubble if
 // it desires it to be visible on the screen.  It is not shown by the
 // init routine.  Closing of the window happens implicitly on dealloc.
-- (id)initWithDelegate:(id<BookmarkBubbleControllerDelegate>)delegate
-          parentWindow:(NSWindow*)parentWindow
-      topLeftForBubble:(NSPoint)topLeftForBubble
-                 model:(BookmarkModel*)model
-                  node:(const BookmarkNode*)node
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+          topLeftForBubble:(NSPoint)topLeftForBubble
+                     model:(BookmarkModel*)model
+                      node:(const BookmarkNode*)node
      alreadyBookmarked:(BOOL)alreadyBookmarked;
 
 // Actions for buttons in the dialog.
-- (IBAction)edit:(id)sender;
 - (IBAction)ok:(id)sender;
 - (IBAction)remove:(id)sender;
 - (IBAction)cancel:(id)sender;
+
+// These actions send a -editBookmarkNode: action up the responder chain.
+- (IBAction)edit:(id)sender;
 - (IBAction)folderChanged:(id)sender;
 
 @end
diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.mm b/chrome/browser/cocoa/bookmark_bubble_controller.mm
index aba44b9..e3bee67 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.mm
@@ -24,6 +24,8 @@
 
 @implementation BookmarkBubbleController
 
+@synthesize node = node_;
+
 + (id)chooseAnotherFolderObject {
   // Singleton object to act as a representedObject for the "choose another
   // folder" item in the pop up.
@@ -34,31 +36,40 @@
   return object;
 }
 
-- (id)initWithDelegate:(id<BookmarkBubbleControllerDelegate>)delegate
-          parentWindow:(NSWindow*)parentWindow
-      topLeftForBubble:(NSPoint)topLeftForBubble
-                 model:(BookmarkModel*)model
-                  node:(const BookmarkNode*)node
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+          topLeftForBubble:(NSPoint)topLeftForBubble
+                     model:(BookmarkModel*)model
+                      node:(const BookmarkNode*)node
      alreadyBookmarked:(BOOL)alreadyBookmarked {
   NSString* nibPath =
       [mac_util::MainAppBundle() pathForResource:@"BookmarkBubble"
                                           ofType:@"nib"];
   if ((self = [super initWithWindowNibPath:nibPath owner:self])) {
-    delegate_ = delegate;
     parentWindow_ = parentWindow;
     topLeftForBubble_ = topLeftForBubble;
     model_ = model;
     node_ = node;
     alreadyBookmarked_ = alreadyBookmarked;
+
+    // Watch to see if the parent window closes, and if so, close this one.
+    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+    [center addObserver:self
+               selector:@selector(windowWillClose:)
+                   name:NSWindowWillCloseNotification
+                 object:parentWindow_];
   }
   return self;
 }
 
+- (void)dealloc {
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
 - (void)windowWillClose:(NSNotification *)notification {
   [self autorelease];
 }
 
-
 // We want this to be a child of a browser window.  addChildWindow:
 // (called from this function) will bring the window on-screen;
 // unfortunately, [NSWindowController showWindow:] will also bring it
@@ -81,19 +92,19 @@
 
   [self fillInFolderList];
 
-  [[self window] makeKeyAndOrderFront:self];
+  [window makeKeyAndOrderFront:self];
 }
 
 - (void)close {
   [parentWindow_ removeChildWindow:[self window]];
-  [delegate_ bubbleWindowWillClose:[self window]];
   [super close];
 }
 
 // Shows the bookmark editor sheet for more advanced editing.
 - (void)showEditor {
-  [self ok:nil];
-  [delegate_ editBookmarkNode:node_];
+  [self ok:self];
+  // Send the action up through the responder chain.
+  [NSApp sendAction:@selector(editBookmarkNode:) to:nil from:self];
 }
 
 - (IBAction)edit:(id)sender {
@@ -142,17 +153,13 @@
 // notifications. When key is resigned mirror Windows behavior and close the
 // window.
 - (void)windowDidResignKey:(NSNotification*)notification {
-  DCHECK_EQ([notification object], [self window]);
-
-  // We must tell our delegate our window is gone RIGHT NOW.  The
-  // performSelector: call below triggers a close but it might get
-  // processed after a request to show a new bubble (e.g. in another
-  // window).
-  [delegate_ bubbleWindowWillClose:[self window]];
-
-  // Can't call close from within a window delegate method. Call close for the
-  // next time through the event loop.
-  [self performSelector:@selector(ok:) withObject:self afterDelay:0];
+  NSWindow* window = [self window];
+  DCHECK_EQ([notification object], window);
+  if ([window isVisible]) {
+    // If the window isn't visible, it is already closed, and this notification
+    // has been sent as part of the closing operation, so no need to close.
+    [self ok:self];
+  }
 }
 
 // Look at the dialog; if the user has changed anything, update the
diff --git a/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
index d63aaa3..76b8d38 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
@@ -13,58 +13,16 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
-@interface BBDelegate : NSObject<BookmarkBubbleControllerDelegate> {
- @private
-  InfoBubbleWindow* window_;
-  int edits_;
-}
-
-@property (readonly) int edits;
-@property (readonly, getter=isWindowClosing) BOOL windowClosing;
-
-@end
-
-@implementation BBDelegate
-
-@synthesize edits = edits_;
-
-- (NSPoint)topLeftForBubble {
-  return NSMakePoint(10, 300);
-}
-
-- (void)editBookmarkNode:(const BookmarkNode*)node {
-  edits_++;
-}
-
-// Tell us (a delegate) which controller it will "own".  This sets up
-// the test classes (e.g. simulates what Chromium would do after
-// creating a BookmarkBubbleController).
-- (void)setWindowController:(NSWindowController *)controller {
-  window_ = static_cast<InfoBubbleWindow*>([controller window]);
-  [controller showWindow:self];
-}
-
-// The bubble tells the delegate when it will go away.
-- (void)bubbleWindowWillClose:(NSWindow*)window {
-  // empty
- }
-
-- (BOOL)isWindowClosing {
-  return [window_ isClosing];
-}
-
-@end
-
 namespace {
 
 class BookmarkBubbleControllerTest : public CocoaTest {
  public:
+  static int edits_;
   BrowserTestHelper helper_;
-  scoped_nsobject<BBDelegate> delegate_;
   BookmarkBubbleController* controller_;
 
   BookmarkBubbleControllerTest() : controller_(nil) {
-    delegate_.reset([[BBDelegate alloc] init]);
+    edits_ = 0;
   }
 
   virtual void TearDown() {
@@ -78,20 +36,27 @@ class BookmarkBubbleControllerTest : public CocoaTest {
     if (controller_)
       [controller_ close];
     controller_ = [[BookmarkBubbleController alloc]
-                       initWithDelegate:delegate_.get()
-                       parentWindow:test_window()
-                       topLeftForBubble:[delegate_ topLeftForBubble]
-                       model:helper_.profile()->GetBookmarkModel()
-                       node:node
-                       alreadyBookmarked:YES];
+                      initWithParentWindow:test_window()
+                          topLeftForBubble:TopLeftForBubble()
+                                     model:helper_.profile()->GetBookmarkModel()
+                                      node:node
+                         alreadyBookmarked:YES];
     EXPECT_TRUE([controller_ window]);
-    [delegate_ setWindowController:controller_];
+    [controller_ showWindow:nil];
     return controller_;
   }
 
   BookmarkModel* GetBookmarkModel() {
     return helper_.profile()->GetBookmarkModel();
   }
+
+  bool IsWindowClosing() {
+    return [static_cast<InfoBubbleWindow*>([controller_ window]) isClosing];
+  }
+
+  NSPoint TopLeftForBubble() {
+    return NSMakePoint(10, 300);
+  }
 };
 
 // Confirm basics about the bubble window (e.g. that it is inside the
@@ -110,6 +75,22 @@ TEST_F(BookmarkBubbleControllerTest, TestBubbleWindow) {
                              [window frame]));
 }
 
+// Test that we can handle closing the parent window
+TEST_F(BookmarkBubbleControllerTest, TestClosingParentWindow) {
+  BookmarkModel* model = GetBookmarkModel();
+  const BookmarkNode* node = model->AddURL(model->GetBookmarkBarNode(),
+                                           0,
+                                           L"Bookie markie title",
+                                           GURL("http://www.google.com"));
+  BookmarkBubbleController* controller = ControllerForNode(node);
+  EXPECT_TRUE(controller);
+  NSWindow* window = [controller window];
+  EXPECT_TRUE(window);
+  base::ScopedNSAutoreleasePool pool;
+  [test_window() performClose:NSApp];
+}
+
+
 // Confirm population of folder list
 TEST_F(BookmarkBubbleControllerTest, TestFillInFolder) {
   // Create some folders, including a nested folder
@@ -158,11 +139,11 @@ TEST_F(BookmarkBubbleControllerTest, TestEdit) {
   BookmarkBubbleController* controller = ControllerForNode(node);
   EXPECT_TRUE(controller);
 
-  EXPECT_EQ([delegate_ edits], 0);
-  EXPECT_FALSE([delegate_ isWindowClosing]);
+  EXPECT_EQ(edits_, 0);
+  EXPECT_FALSE(IsWindowClosing());
   [controller edit:controller];
-  EXPECT_EQ([delegate_ edits], 1);
-  EXPECT_TRUE([delegate_ isWindowClosing]);
+  EXPECT_EQ(edits_, 1);
+  EXPECT_TRUE(IsWindowClosing());
 }
 
 // CallClose; bubble gets closed.
@@ -172,14 +153,14 @@ TEST_F(BookmarkBubbleControllerTest, TestClose) {
                                              0,
                                              L"Bookie markie title",
                                              GURL("http://www.google.com"));
-  EXPECT_EQ([delegate_ edits], 0);
+  EXPECT_EQ(edits_, 0);
 
   BookmarkBubbleController* controller = ControllerForNode(node);
   EXPECT_TRUE(controller);
-  EXPECT_FALSE([delegate_ isWindowClosing]);
+  EXPECT_FALSE(IsWindowClosing());
   [controller ok:controller];
-  EXPECT_EQ([delegate_ edits], 0);
-  EXPECT_TRUE([delegate_ isWindowClosing]);
+  EXPECT_EQ(edits_, 0);
+  EXPECT_TRUE(IsWindowClosing());
 }
 
 // User changes title and parent folder in the UI
@@ -277,7 +258,7 @@ TEST_F(BookmarkBubbleControllerTest, TestRemove) {
 
   [controller remove:controller];
   EXPECT_FALSE(model->IsBookmarked(gurl));
-  EXPECT_TRUE([delegate_ isWindowClosing]);
+  EXPECT_TRUE(IsWindowClosing());
 }
 
 // Confirm picking "choose another folder" caused edit: to be called.
@@ -292,9 +273,9 @@ TEST_F(BookmarkBubbleControllerTest, PopUpSelectionChanged) {
 
   NSPopUpButton* button = [controller folderPopUpButton];
   [button selectItemWithTitle:[[controller class] chooseAnotherFolderString]];
-  EXPECT_EQ([delegate_ edits], 0);
+  EXPECT_EQ(edits_, 0);
   [button sendAction:[button action] to:[button target]];
-  EXPECT_EQ([delegate_ edits], 1);
+  EXPECT_EQ(edits_, 1);
 }
 
 // Create a controller that simulates the bookmark just now being created by
@@ -309,12 +290,11 @@ TEST_F(BookmarkBubbleControllerTest, EscapeRemovesNewBookmark) {
                                            gurl);
   BookmarkBubbleController* controller =
       [[BookmarkBubbleController alloc]
-          initWithDelegate:delegate_.get()
-              parentWindow:test_window()
-          topLeftForBubble:[delegate_ topLeftForBubble]
-                     model:helper_.profile()->GetBookmarkModel()
-                      node:node
-         alreadyBookmarked:NO];  // The last param is the key difference.
+          initWithParentWindow:test_window()
+              topLeftForBubble:TopLeftForBubble()
+                         model:helper_.profile()->GetBookmarkModel()
+                          node:node
+             alreadyBookmarked:NO];  // The last param is the key difference.
   EXPECT_TRUE([controller window]);
   // Calls release on controller.
   [controller cancel:nil];
@@ -339,3 +319,16 @@ TEST_F(BookmarkBubbleControllerTest, EscapeDoesntTouchExistingBookmark) {
 }
 
 }  // namespace
+
+@implementation NSApplication (BookmarkBubbleUnitTest)
+// Add handler for the editBookmarkNode: action to NSApp for testing purposes.
+// Normally this would be sent up the responder tree correctly, but since
+// tests run in the background, key window and main window are never set on
+// NSApplication. Adding it to NSApplication directly removes the need for
+// worrying about what the current window with focus is.
+- (void)editBookmarkNode:(id)sender {
+  EXPECT_TRUE([sender respondsToSelector:@selector(node)]);
+  BookmarkBubbleControllerTest::edits_++;
+}
+
+@end
diff --git a/chrome/browser/cocoa/browser_window_controller.h b/chrome/browser/cocoa/browser_window_controller.h
index d7fedde..fed8fc1 100644
--- a/chrome/browser/cocoa/browser_window_controller.h
+++ b/chrome/browser/cocoa/browser_window_controller.h
@@ -45,7 +45,6 @@ class TabStripModelObserverBridge;
 @interface BrowserWindowController :
   TabWindowController<NSUserInterfaceValidations,
                       BookmarkBarControllerDelegate,
-                      BookmarkBubbleControllerDelegate,
                       BrowserCommandExecutor,
                       ViewResizer,
                       GTMThemeDelegate> {
@@ -73,9 +72,7 @@ class TabStripModelObserverBridge;
   // be shut down before our destructors are called.
   StatusBubbleMac* statusBubble_;
 
-  // Strong. We don't wrap it in scoped_nsobject because we must close
-  // it appropriately in [windowWillClose:].
-  BookmarkBubbleController* bookmarkBubbleController_;
+  BookmarkBubbleController* bookmarkBubbleController_;  // Weak.
   scoped_nsobject<GTMTheme> theme_;
   BOOL ownsBrowser_;  // Only ever NO when testing
   CGFloat verticalOffsetForStatusBubble_;
diff --git a/chrome/browser/cocoa/browser_window_controller.mm b/chrome/browser/cocoa/browser_window_controller.mm
index 4e326aa..71c09f2 100644
--- a/chrome/browser/cocoa/browser_window_controller.mm
+++ b/chrome/browser/cocoa/browser_window_controller.mm
@@ -231,6 +231,8 @@ willPositionSheet:(NSWindow*)sheet
   if (ownsBrowser_ == NO)
     browser_.release();
 
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+
   [super dealloc];
 }
 
@@ -245,8 +247,8 @@ willPositionSheet:(NSWindow*)sheet
   // We need the window to go away now.
   // We can't actually use |-autorelease| here because there's an embedded
   // run loop in the |-performClose:| which contains its own autorelease pool.
-  // Instead we use call it after a zero-length delay, which gets us back
-  // to the main event loop.
+  // Instead call it after a zero-length delay, which gets us back to the main
+  // event loop.
   [self performSelector:@selector(autorelease)
              withObject:nil
              afterDelay:0];
@@ -260,15 +262,14 @@ willPositionSheet:(NSWindow*)sheet
   DCHECK_EQ([notification object], [self window]);
   DCHECK(!browser_->tabstrip_model()->count());
   [savedRegularWindow_ close];
-  [bookmarkBubbleController_ close];
   // We delete statusBubble here because we need to kill off the dependency
   // that its window has on our window before our window goes away.
   delete statusBubble_;
   statusBubble_ = NULL;
   // We can't actually use |-autorelease| here because there's an embedded
   // run loop in the |-performClose:| which contains its own autorelease pool.
-  // Instead we call it after a zero-length delay, which gets us back
-  // to the main event loop.
+  // Instead call it after a zero-length delay, which gets us back to the main
+  // event loop.
   [self performSelector:@selector(autorelease)
              withObject:nil
              afterDelay:0];
@@ -1160,40 +1161,55 @@ willPositionSheet:(NSWindow*)sheet
 
 // Show the bookmark bubble (e.g. user just clicked on the STAR).
 - (void)showBookmarkBubbleForURL:(const GURL&)url
-               alreadyBookmarked:(BOOL)alreadyBookmarked {
+               alreadyBookmarked:(BOOL)alreadyMarked {
   if (!bookmarkBubbleController_) {
     BookmarkModel* model = browser_->profile()->GetBookmarkModel();
     const BookmarkNode* node = model->GetMostRecentlyAddedNodeForURL(url);
     NSPoint topLeft = [self topLeftForBubble];
     bookmarkBubbleController_ =
-        [[BookmarkBubbleController alloc] initWithDelegate:self
-                                              parentWindow:[self window]
-                                          topLeftForBubble:topLeft
-                                                     model:model
-                                                      node:node
-                                         alreadyBookmarked:alreadyBookmarked];
+        [[BookmarkBubbleController alloc] initWithParentWindow:[self window]
+                                              topLeftForBubble:topLeft
+                                                         model:model
+                                                          node:node
+                                             alreadyBookmarked:alreadyMarked];
     [bookmarkBubbleController_ showWindow:self];
+    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+    [center addObserver:self
+               selector:@selector(bubbleWindowWillClose:)
+                   name:NSWindowWillCloseNotification
+                 object:[bookmarkBubbleController_ window]];
   }
 }
 
-// Implement BookmarkBubbleControllerDelegate.
-- (void)bubbleWindowWillClose:(NSWindow*)window {
-  if (window == [bookmarkBubbleController_ window])
-    bookmarkBubbleController_ = nil;
-}
-
-// Implement BookmarkBubbleControllerDelegate.
-- (void)editBookmarkNode:(const BookmarkNode*)node {
-  // A BookmarkEditorController is a sheet that owns itself, and
-  // deallocates itself when closed.
-  [[[BookmarkEditorController alloc]
-     initWithParentWindow:[self window]
-                  profile:browser_->profile()
-                   parent:node->GetParent()
-                     node:node
-            configuration:BookmarkEditor::SHOW_TREE
-                  handler:NULL]
-    runAsModalSheet];
+// Nil out the weak bookmark bubble controller reference.
+- (void)bubbleWindowWillClose:(NSNotification*)notification {
+  DCHECK([notification object] == [bookmarkBubbleController_ window]);
+  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+  [center removeObserver:self
+                 name:NSWindowWillCloseNotification
+               object:[bookmarkBubbleController_ window]];
+  bookmarkBubbleController_ = nil;
+}
+
+// Handle the editBookmarkNode: action sent from bookmark bubble controllers.
+- (void)editBookmarkNode:(id)sender {
+  BOOL responds = [sender respondsToSelector:@selector(node)];
+  DCHECK(responds);
+  if (responds) {
+    const BookmarkNode* node = [sender node];
+    if (node) {
+      // A BookmarkEditorController is a sheet that owns itself, and
+      // deallocates itself when closed.
+      [[[BookmarkEditorController alloc]
+         initWithParentWindow:[self window]
+                      profile:browser_->profile()
+                       parent:node->GetParent()
+                         node:node
+                configuration:BookmarkEditor::SHOW_TREE
+                      handler:NULL]
+        runAsModalSheet];
+    }
+  }
 }