Fixes up bookmark bubbles and the browser window so that they shut down correctly.

BookmarkBubbleController has been made an NSWindowController instead of the
view controller that it used to be, and now loads its window from the nib instead
of creating it on the fly. Also cleans up fullscreen mode so that the window
referenced from browser_window_controller stays constant instead of having
[self window] and window_ potentially pointing at two different windows.

BookmarkBubble.xib has been modified so that it instantiates a window containing
a bubble view instead of just instantiating a view.

BUG=25054
TEST=Try going in and out of full screen mode. Try bringing up a bookmark bubble by clicking on the star. Try creating a pile of windows and then quitting,
Review URL: http://codereview.chromium.org/333017

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

chrome/browser/cocoa/bookmark_bubble_controller.h

@@ -16,36 +16,21 @@ class BookmarkNode;
 // The bubble asks the delegate to perform an edit when needed.
 - (void)editBookmarkNode:(const BookmarkNode*)node;
 
-// The bubble tells its delegate when it's done and can be deallocated.
-- (void)doneWithBubbleController:(BookmarkBubbleController*)controller;
-
 @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.)
-//
-// The bubble is stored in a nib as a view, not as a window, so we can
-// make it an actual bubble.  There is no nib-rific way to encode a
-// NSBorderlessWindowMask NSWindow, and the style of an NSWindow can't
-// be set other than init time.  To deal, we create the NSWindow
-// programatically, but encode the view in a nib.  Thus,
-// BookmarkBubbleController is an NSViewController, not an
-// NSWindowController.
-@interface BookmarkBubbleController : NSViewController<NSWindowDelegate> {
+@interface BookmarkBubbleController : NSWindowController<NSWindowDelegate> {
  @private
-  // Unexpected for this controller, perhaps, but our window does NOT
-  // come from a nib.
-  scoped_nsobject<NSWindow> window_;
-
   id<BookmarkBubbleControllerDelegate> delegate_;  // weak like other delegates
   NSWindow* parentWindow_;  // weak
   NSPoint topLeftForBubble_;
 
   // Both weak; owned by the current browser's profile
-  BookmarkModel* model_;
-  const BookmarkNode* node_;
+  BookmarkModel* model_;  // weak
+  const BookmarkNode* node_;  // weak
 
   // A mapping from titles to nodes so we only have to walk this once.
   scoped_nsobject<NSMutableArray> titleMapping_;
@@ -71,29 +56,26 @@ class BookmarkNode;
                   node:(const BookmarkNode*)node
      alreadyBookmarked:(BOOL)alreadyBookmarked;
 
-- (void)showWindow;
-
 // Actions for buttons in the dialog.
 - (IBAction)edit:(id)sender;
-- (IBAction)close:(id)sender;
+- (IBAction)ok:(id)sender;
 - (IBAction)remove:(id)sender;
+- (IBAction)cancel:(id)sender;
 
 @end
 
 
 // Exposed only for unit testing.
 @interface BookmarkBubbleController(ExposedForUnitTesting)
-- (NSWindow*)createBubbleWindow;
 - (void)fillInFolderList;
-- (BOOL)windowHasBeenClosed;
 - (void)addFolderNodes:(const BookmarkNode*)parent toComboBox:(NSComboBox*)box;
 - (void)updateBookmarkNode;
-- (void)setTitle:(NSString *)title parentFolder:(NSString*)folder;
+- (void)setTitle:(NSString*)title parentFolder:(NSString*)folder;
 - (NSString*)chooseAnotherFolderString;
+- (NSComboBox*)folderComboBox;
 @end
 
-// Also private but I need to declare them specially for @synthesize to work.
-@interface BookmarkBubbleController ()
-@property (readonly) id delegate;
-@property (readonly) NSComboBox* folderComboBox;
-@end
+
+
+
+

chrome/browser/cocoa/bookmark_bubble_controller.mm

@@ -11,25 +11,18 @@
 #include "chrome/browser/metrics/user_metrics.h"
 #include "grit/generated_resources.h"
 
-
-@interface BookmarkBubbleController(PrivateAPI)
-- (void)closeWindow;
-@end
-
 @implementation BookmarkBubbleController
 
-@synthesize delegate = delegate_;
-@synthesize folderComboBox = folderComboBox_;
-
 - (id)initWithDelegate:(id<BookmarkBubbleControllerDelegate>)delegate
           parentWindow:(NSWindow*)parentWindow
       topLeftForBubble:(NSPoint)topLeftForBubble
                  model:(BookmarkModel*)model
                   node:(const BookmarkNode*)node
      alreadyBookmarked:(BOOL)alreadyBookmarked {
-  if ((self = [super initWithNibName:@"BookmarkBubble"
-                              bundle:mac_util::MainAppBundle()])) {
-    // All these are weak...
+  NSString* nibPath =
+      [mac_util::MainAppBundle() pathForResource:@"BookmarkBubble"
+                                          ofType:@"nib"];
+  if ((self = [super initWithWindowNibPath:nibPath owner:self])) {
     delegate_ = delegate;
     parentWindow_ = parentWindow;
     topLeftForBubble_ = topLeftForBubble;
@@ -42,45 +35,37 @@
   return self;
 }
 
-- (void)dealloc {
-  [self closeWindow];
-  [super dealloc];
-}
-
-- (void)showWindow {
-  [self view];  // force nib load and window_ allocation
-  [window_ makeKeyAndOrderFront:self];
+- (void)windowWillClose:(NSNotification *)notification {
+  [self autorelease];
 }
 
-// Actually close the window.  Do nothing else.
-- (void)closeWindow {
-  [parentWindow_ removeChildWindow:window_];
-  [window_ close];
-}
-
-- (void)awakeFromNib {
-  window_.reset([self createBubbleWindow]);
-  [parentWindow_ addChildWindow:window_ ordered:NSWindowAbove];
-
-  // Fill in inital values for text, controls, ...
-
+- (void)windowDidLoad {
+  NSWindow* window = [self window];
+  NSPoint origin = [parentWindow_ convertBaseToScreen:topLeftForBubble_];
+  origin.y -= NSHeight([window frame]);
+  [window setFrameOrigin:origin];
+  [parentWindow_ addChildWindow:window ordered:NSWindowAbove];
   // Default is IDS_BOOMARK_BUBBLE_PAGE_BOOKMARK; "Bookmark".
   // If adding for the 1st time the string becomes "Bookmark Added!"
   if (!alreadyBookmarked_) {
     NSString* title =
-      l10n_util::GetNSString(IDS_BOOMARK_BUBBLE_PAGE_BOOKMARKED);
+        l10n_util::GetNSString(IDS_BOOMARK_BUBBLE_PAGE_BOOKMARKED);
     [bigTitle_ setStringValue:title];
   }
 
   [self fillInFolderList];
 }
 
+- (void)close {
+  [parentWindow_ removeChildWindow:[self window]];
+  [super close];
+}
+
 // Shows the bookmark editor sheet for more advanced editing.
 - (void)showEditor {
   [self updateBookmarkNode];
-  [self closeWindow];
   [delegate_ editBookmarkNode:node_];
-  [delegate_ doneWithBubbleController:self];
+  [self close];
 }
 
 - (IBAction)edit:(id)sender {
@@ -88,13 +73,9 @@
   [self showEditor];
 }
 
-- (IBAction)close:(id)sender {
-  if (node_) {
-    // no node_ if the bookmark was just removed
-    [self updateBookmarkNode];
-  }
-  [self closeWindow];
-  [delegate_ doneWithBubbleController:self];
+- (IBAction)ok:(id)sender {
+  [self updateBookmarkNode];
+  [self close];
 }
 
 // By implementing this, ESC causes the window to go away. If clicking the
@@ -105,7 +86,7 @@
     // |-remove:| calls |-close| so we don't have to bother.
     [self remove:sender];
   } else {
-    [self close:sender];
+    [self ok:sender];
   }
 }
 
@@ -113,7 +94,7 @@
   model_->SetURLStarred(node_->GetURL(), node_->GetTitle(), false);
   UserMetrics::RecordAction(L"BookmarkBubble_Unstar", model_->profile());
   node_ = NULL;  // no longer valid
-  [self close:self];
+  [self ok:sender];
 }
 
 // We are the delegate of the combo box so we can tell when "choose
@@ -128,18 +109,14 @@
 }
 
 // We are the delegate of our own window so we know when we lose key.
-// When we lose key status we close, mirroring Windows behaivor.
+// When we lose key status we close, mirroring Windows behavior.
 - (void)windowDidResignKey:(NSNotification*)notification {
+  DCHECK_EQ([notification object], [self window]);
 
-  // If we get here, we are done with this window and controller.  The
-  // call of close: may destroy us which destroys the window.  But the
-  // window is in the middle of processing resignKeyWindow.  We
-  // retain/autorelease the window to insure it lasts until the end of
-  // this event.
-  [[window_ retain] autorelease];
-
-  if ([window_ isVisible])
-    [self close:self];
+  // Can't call close from within a window delegate method. We can call
+  // close after it's finished though. So this will call close for us next
+  // time through the event loop.
+  [self performSelector:@selector(ok:) withObject:self afterDelay:0];
 }
 
 @end  // BookmarkBubbleController
@@ -147,20 +124,6 @@
 
 @implementation BookmarkBubbleController(ExposedForUnitTesting)
 
-// Create and return a retained NSWindow for this bubble.
-- (NSWindow*)createBubbleWindow {
-  NSRect contentRect = [[self view] frame];
-  NSPoint origin = topLeftForBubble_;
-  origin.y -= contentRect.size.height;  // since it'll be our bottom-left
-  contentRect.origin = origin;
-  // Now convert to global coordinates since it'll be used for a window.
-  contentRect.origin = [parentWindow_ convertBaseToScreen:contentRect.origin];
-  NSWindow* window = [[BookmarkBubbleWindow alloc]
-                         initWithContentRect:contentRect];
-  [window setDelegate:self];
-  [window setContentView:[self view]];
-  return window;
-}
 
 // Fill in all information related to the folder combo box.
 //
@@ -183,10 +146,6 @@
   [folderComboBox_ selectItemWithObjectValue:parentTitle];
 }
 
-- (BOOL)windowHasBeenClosed {
-  return ![window_ isVisible];
-}
-
 // For the given folder node, walk the tree and add folder names to
 // the given combo box.
 //
@@ -207,6 +166,8 @@
 // Look at the dialog; if the user has changed anything, update the
 // bookmark node to reflect this.
 - (void)updateBookmarkNode {
+  if (!node_) return;
+
   // First the title...
   NSString* oldTitle = base::SysWideToNSString(node_->GetTitle());
   NSString* newTitle = [nameTextField_ stringValue];
@@ -241,4 +202,8 @@
   return chooseAnotherFolder_.get();
 }
 
+- (NSComboBox*)folderComboBox {
+  return folderComboBox_;
+}
+
 @end  // implementation BookmarkBubbleController(ExposedForUnitTesting)

chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm

@@ -13,20 +13,26 @@
 #include "testing/platform_test.h"
 
 @interface BBDelegate : NSObject<BookmarkBubbleControllerDelegate> {
-  NSWindow* window_;  // weak
+ @private
+  BOOL windowClosed_;
   int edits_;
-  int dones_;
 }
+
 @property (readonly) int edits;
-@property (readonly) int dones;
-@property (readonly) NSWindow* window;
+@property (readonly) BOOL windowClosed;
+
 @end
 
 @implementation BBDelegate
 
 @synthesize edits = edits_;
-@synthesize window = window_;
-@synthesize dones = dones_;
+@synthesize windowClosed = windowClosed_;
+
+- (void)dealloc {
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  [nc removeObserver:self];
+  [super dealloc];
+}
 
 - (NSPoint)topLeftForBubble {
   return NSMakePoint(10, 300);
@@ -36,42 +42,54 @@
   edits_++;
 }
 
-- (void)doneWithBubbleController:(BookmarkBubbleController*)controller {
-  dones_++;
+- (void)setWindowController:(NSWindowController *)controller {
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  [nc removeObserver:self];
+  if (controller) {
+    [nc addObserver:self
+           selector:@selector(windowWillClose:)
+               name:NSWindowWillCloseNotification
+             object:[controller window]];
+  }
+  windowClosed_ = NO;
 }
 
-- (void)clear {
-  edits_ = 0;
-  dones_ = 0;
+- (void)windowWillClose:(NSNotification*)notification {
+  windowClosed_ = YES;
 }
-
 @end
 
 namespace {
 
-class BookmarkBubbleControllerTest : public PlatformTest {
+class BookmarkBubbleControllerTest : public CocoaTest {
  public:
-  CocoaTestHelper cocoa_helper_;  // Inits Cocoa, creates window, etc...
   BrowserTestHelper helper_;
   scoped_nsobject<BBDelegate> delegate_;
-  scoped_nsobject<BookmarkBubbleController> controller_;
+  BookmarkBubbleController* controller_;
 
-  BookmarkBubbleControllerTest() {
+  BookmarkBubbleControllerTest() : controller_(nil) {
     delegate_.reset([[BBDelegate alloc] init]);
   }
 
+  virtual void TearDown() {
+    [controller_ close];
+    CocoaTest::TearDown();
+  }
+
   // Returns a controller but ownership not transferred.
   // Only one of these will be valid at a time.
   BookmarkBubbleController* ControllerForNode(const BookmarkNode* node) {
-    controller_.reset([[BookmarkBubbleController alloc]
-                        initWithDelegate:delegate_.get()
-                            parentWindow:cocoa_helper_.window()
-                        topLeftForBubble:[delegate_ topLeftForBubble]
-                                   model:helper_.profile()->GetBookmarkModel()
-                                    node:node
-                       alreadyBookmarked:YES]);
-    [controller_ view];  // force nib load
-    return controller_.get();
+    DCHECK(controller_ == nil);
+    controller_ = [[BookmarkBubbleController alloc]
+                       initWithDelegate:delegate_.get()
+                       parentWindow:test_window()
+                       topLeftForBubble:[delegate_ topLeftForBubble]
+                       model:helper_.profile()->GetBookmarkModel()
+                       node:node
+                       alreadyBookmarked:YES];
+    EXPECT_TRUE([controller_ window]);
+    [delegate_ setWindowController:controller_];
+    return controller_;
   }
 
   BookmarkModel* GetBookmarkModel() {
@@ -89,9 +107,9 @@ TEST_F(BookmarkBubbleControllerTest, TestBubbleWindow) {
                                            GURL("http://www.google.com"));
   BookmarkBubbleController* controller = ControllerForNode(node);
   EXPECT_TRUE(controller);
-  NSWindow* window = [controller createBubbleWindow];
+  NSWindow* window = [controller window];
   EXPECT_TRUE(window);
-  EXPECT_TRUE(NSContainsRect([cocoa_helper_.window() frame],
+  EXPECT_TRUE(NSContainsRect([test_window() frame],
                              [window frame]));
 }
 
@@ -124,7 +142,7 @@ TEST_F(BookmarkBubbleControllerTest, TestFillInFolder) {
 }
 
 // Click on edit; bubble gets closed.
-TEST_F(BookmarkBubbleControllerTest, TestSimpleActions) {
+TEST_F(BookmarkBubbleControllerTest, TestEdit) {
   BookmarkModel* model = GetBookmarkModel();
   const BookmarkNode* node = model->AddURL(model->GetBookmarkBarNode(),
                                            0,
@@ -134,24 +152,27 @@ TEST_F(BookmarkBubbleControllerTest, TestSimpleActions) {
   EXPECT_TRUE(controller);
 
   EXPECT_EQ([delegate_ edits], 0);
-  EXPECT_EQ([delegate_ dones], 0);
-  EXPECT_FALSE([controller windowHasBeenClosed]);
+  EXPECT_FALSE([delegate_ windowClosed]);
   [controller edit:controller];
   EXPECT_EQ([delegate_ edits], 1);
-  EXPECT_EQ([delegate_ dones], 1);
-  EXPECT_TRUE([controller windowHasBeenClosed]);
+  EXPECT_TRUE([delegate_ windowClosed]);
+}
 
-  [delegate_ clear];
+// CallClose; bubble gets closed.
+TEST_F(BookmarkBubbleControllerTest, TestClose) {
+    BookmarkModel* model = GetBookmarkModel();
+    const BookmarkNode* node = model->AddURL(model->GetBookmarkBarNode(),
+                                             0,
+                                             L"Bookie markie title",
+                                             GURL("http://www.google.com"));
   EXPECT_EQ([delegate_ edits], 0);
-  EXPECT_EQ([delegate_ dones], 0);
 
-  controller = ControllerForNode(node);
+  BookmarkBubbleController* controller = ControllerForNode(node);
   EXPECT_TRUE(controller);
-  EXPECT_FALSE([controller windowHasBeenClosed]);
-  [controller close:controller];
+  EXPECT_FALSE([delegate_ windowClosed]);
+  [controller ok:controller];
   EXPECT_EQ([delegate_ edits], 0);
-  EXPECT_EQ([delegate_ dones], 1);
-  EXPECT_TRUE([controller windowHasBeenClosed]);
+  EXPECT_TRUE([delegate_ windowClosed]);
 }
 
 // User changes title and parent folder in the UI
@@ -189,8 +210,7 @@ TEST_F(BookmarkBubbleControllerTest, TestRemove) {
 
   [controller remove:controller];
   EXPECT_FALSE(model->IsBookmarked(gurl));
-  EXPECT_TRUE([controller windowHasBeenClosed]);
-  EXPECT_EQ([delegate_ dones], 1);
+  EXPECT_TRUE([delegate_ windowClosed]);
 }
 
 // Confirm picking "choose another folder" caused edit: to be called.
@@ -219,16 +239,16 @@ TEST_F(BookmarkBubbleControllerTest, EscapeRemovesNewBookmark) {
                                            0,
                                            L"Bookie markie title",
                                            gurl);
-  scoped_nsobject<BookmarkBubbleController> controller(
+  BookmarkBubbleController* controller =
       [[BookmarkBubbleController alloc]
           initWithDelegate:delegate_.get()
-              parentWindow:cocoa_helper_.window()
+              parentWindow:test_window()
           topLeftForBubble:[delegate_ topLeftForBubble]
                      model:helper_.profile()->GetBookmarkModel()
                       node:node
-         alreadyBookmarked:NO]);  // The last param is the key difference.
-  EXPECT_TRUE(controller);
-
+         alreadyBookmarked:NO];  // The last param is the key difference.
+  EXPECT_TRUE([controller window]);
+  // Calls release on controller.
   [controller cancel:nil];
   EXPECT_FALSE(model->IsBookmarked(gurl));
 }

chrome/browser/cocoa/bookmark_bubble_view_unittest.mm

@@ -2,35 +2,25 @@
 // 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/scoped_nsobject.h"
 #import "chrome/browser/cocoa/bookmark_bubble_view.h"
 #import "chrome/browser/cocoa/cocoa_test_helper.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "testing/platform_test.h"
 
 namespace {
 
-class BookmarkBubbleViewTest : public PlatformTest {
+class BookmarkBubbleViewTest : public CocoaTest {
  public:
   BookmarkBubbleViewTest() {
     NSRect frame = NSMakeRect(0, 0, 100, 30);
-    view_.reset([[BookmarkBubbleView alloc] initWithFrame:frame]);
-    [cocoa_helper_.contentView() addSubview:view_.get()];
+    scoped_nsobject<BookmarkBubbleView> view(
+        [[BookmarkBubbleView alloc] initWithFrame:frame]);
+    view_ = view.get();
+    [[test_window() contentView] addSubview:view_];
   }
 
-  CocoaTestHelper cocoa_helper_;  // Inits Cocoa, creates window, etc...
-  scoped_nsobject<BookmarkBubbleView> view_;
+  BookmarkBubbleView* view_;
 };
 
-// Test drawing and an add/remove from the view hierarchy to ensure
-// nothing leaks or crashes.
-TEST_F(BookmarkBubbleViewTest, AddRemoveDisplay) {
-  [view_ display];
-  EXPECT_EQ(cocoa_helper_.contentView(), [view_ superview]);
-  [view_.get() removeFromSuperview];
-  EXPECT_FALSE([view_ superview]);
-}
+TEST_VIEW(BookmarkBubbleViewTest, view_);
 
 }  // namespace

chrome/browser/cocoa/bookmark_bubble_window.h

@@ -6,6 +6,4 @@
 
 // Window for the bookmark bubble that comes up when you click on "STAR".
 @interface BookmarkBubbleWindow : NSWindow
-- (id)initWithContentRect:(NSRect)contentRect;
 @end
-

chrome/browser/cocoa/bookmark_bubble_window.mm

@@ -6,12 +6,14 @@
 
 @implementation BookmarkBubbleWindow
 
-- (id)initWithContentRect:(NSRect)contentRect {
+- (id)initWithContentRect:(NSRect)contentRect
+                styleMask:(NSUInteger)aStyle
+                  backing:(NSBackingStoreType)bufferingType
+                    defer:(BOOL)flag {
   if ((self = [super initWithContentRect:contentRect
                                styleMask:NSBorderlessWindowMask
-                                 backing:NSBackingStoreBuffered
-                                   defer:YES])) {
-    [self setReleasedWhenClosed:NO];
+                                 backing:bufferingType
+                                   defer:flag])) {
     [self setBackgroundColor:[NSColor clearColor]];
     [self setExcludedFromWindowsMenu:YES];
     [self setAlphaValue:1.0];

chrome/browser/cocoa/bookmark_bubble_window_unittest.mm

@@ -5,24 +5,21 @@
 #include "base/scoped_ptr.h"
 #include "chrome/browser/cocoa/cocoa_test_helper.h"
 #include "chrome/browser/cocoa/bookmark_bubble_window.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "testing/platform_test.h"
 
-class BookmarkBubbleWindowTest : public PlatformTest {
- public:
-  CocoaTestHelper cocoa_helper_;
+class BookmarkBubbleWindowTest : public CocoaTest {
 };
 
 TEST_F(BookmarkBubbleWindowTest, Basics) {
-  scoped_nsobject<BookmarkBubbleWindow> window_;
-  window_.reset([[BookmarkBubbleWindow alloc]
-                  initWithContentRect:NSMakeRect(0,0,10,10)]);
+  BookmarkBubbleWindow* window =
+      [[BookmarkBubbleWindow alloc] initWithContentRect:NSMakeRect(0, 0, 10, 10)
+                                              styleMask:NSBorderlessWindowMask
+                                                backing:NSBackingStoreBuffered
+                                                  defer:NO];
+  EXPECT_TRUE([window canBecomeKeyWindow]);
+  EXPECT_FALSE([window canBecomeMainWindow]);
 
-  EXPECT_TRUE([window_ canBecomeKeyWindow]);
-  EXPECT_FALSE([window_ canBecomeMainWindow]);
-
-  EXPECT_TRUE([window_ isExcludedFromWindowsMenu]);
-  EXPECT_FALSE([window_ isReleasedWhenClosed]);
+  EXPECT_TRUE([window isExcludedFromWindowsMenu]);
+  [window close];
 }
 
 

chrome/browser/cocoa/browser_window_controller.h

@@ -50,14 +50,9 @@ class TabStripModelObserverBridge;
   // The ordering of these members is important as it determines the order in
   // which they are destroyed. |browser_| needs to be destroyed last as most of
   // the other objects hold weak references to it or things it owns
-  // (tab/toolbar/bookmark models, profiles, etc). We hold a strong ref to the
-  // window so that it will live after the NSWindowController dealloc has run
-  // (which happens *before* these scoped pointers are torn down). Keeping it
-  // alive ensures that weak view or window pointers remain valid through
-  // their destruction sequence.
+  // (tab/toolbar/bookmark models, profiles, etc).
   scoped_ptr<Browser> browser_;
-  scoped_nsobject<ChromeBrowserWindow> window_;
-  scoped_nsobject<NSWindow> fullscreen_window_;
+  NSWindow* savedRegularWindow_;
   scoped_ptr<TabStripModelObserverBridge> tabObserver_;
   scoped_ptr<BrowserWindowCocoa> windowShim_;
   scoped_nsobject<ToolbarController> toolbarController_;
@@ -65,14 +60,21 @@ class TabStripModelObserverBridge;
   scoped_nsobject<TabStripController> tabStripController_;
   scoped_nsobject<FindBarCocoaController> findBarCocoaController_;
   scoped_nsobject<InfoBarContainerController> infoBarContainerController_;
-  scoped_ptr<StatusBubbleMac> statusBubble_;
   scoped_nsobject<DownloadShelfController> downloadShelfController_;
   scoped_nsobject<ExtensionShelfController> extensionShelfController_;
   scoped_nsobject<BookmarkBarController> bookmarkBarController_;
-  scoped_nsobject<BookmarkBubbleController> bookmarkBubbleController_;
+
+  // Strong. StatusBubble is a special case of a strong reference that
+  // we don't wrap in a scoped_ptr because it is acting the same
+  // as an NSWindowController in that it wraps a window that must
+  // 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_;
   scoped_nsobject<GTMTheme> theme_;
   BOOL ownsBrowser_;  // Only ever NO when testing
-  BOOL fullscreen_;
   CGFloat verticalOffsetForStatusBubble_;
 }
 

chrome/browser/cocoa/browser_window_controller_unittest.mm

@@ -6,6 +6,7 @@
 #include "base/scoped_nsautorelease_pool.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_window.h"
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #include "chrome/browser/cocoa/browser_window_controller.h"
 #include "chrome/browser/cocoa/cocoa_test_helper.h"
@@ -14,7 +15,6 @@
 #include "chrome/common/pref_service.h"
 #include "chrome/test/testing_browser_process.h"
 #include "chrome/test/testing_profile.h"
-#include "testing/gtest/include/gtest/gtest.h"
 
 @interface BrowserWindowController (JustForTesting)
 // Already defined in BWC.
@@ -57,21 +57,23 @@
 }
 @end
 
-class BrowserWindowControllerTest : public testing::Test {
+class BrowserWindowControllerTest : public CocoaTest {
+ public:
   virtual void SetUp() {
-    controller_.reset([[BrowserWindowController alloc]
-                        initWithBrowser:browser_helper_.browser()
-                          takeOwnership:NO]);
+    CocoaTest::SetUp();
+    Browser* browser = browser_helper_.browser();
+    controller_ = [[BrowserWindowController alloc] initWithBrowser:browser
+                                                     takeOwnership:NO];
+  }
+
+  virtual void TearDown() {
+    [controller_ close];
+    CocoaTest::TearDown();
   }
 
  public:
-  // Order is very important here.  We want the controller deleted
-  // before the pool, and want the pool deleted before
-  // BrowserTestHelper.
-  CocoaTestHelper cocoa_helper_;
   BrowserTestHelper browser_helper_;
-  base::ScopedNSAutoreleasePool pool_;
-  scoped_nsobject<BrowserWindowController> controller_;
+  BrowserWindowController* controller_;
 };
 
 TEST_F(BrowserWindowControllerTest, TestSaveWindowPosition) {
@@ -89,70 +91,12 @@ TEST_F(BrowserWindowControllerTest, TestSaveWindowPosition) {
   EXPECT_TRUE(prefs->GetDictionary(prefs::kBrowserWindowPlacement) != NULL);
 }
 
-@interface BrowserWindowControllerFakeFullscreen : BrowserWindowController {
- @private
-  // We release the window ourselves, so we don't have to rely on the unittest
-  // doing it for us.
-  scoped_nsobject<NSWindow> fullscreenWindow_;
-}
-@end
-@implementation BrowserWindowControllerFakeFullscreen
-// Override fullscreenWindow to return a dummy window.  This isn't needed to
-// pass the test, but because the dummy window is only 100x100, it prevents the
-// real fullscreen window from flashing up and taking over the whole screen..
-// We have to return an actual window because layoutSubviews: looks at the
-// window's frame.
-- (NSWindow*)fullscreenWindow {
-  if (fullscreenWindow_.get())
-    return fullscreenWindow_.get();
-
-  fullscreenWindow_.reset(
-      [[NSWindow alloc] initWithContentRect:NSMakeRect(0,0,400,400)
-                                   styleMask:NSBorderlessWindowMask
-                                     backing:NSBackingStoreBuffered
-                                       defer:NO]);
-  [fullscreenWindow_ setReleasedWhenClosed:NO];
-  return fullscreenWindow_.get();
-}
-@end
-
-TEST_F(BrowserWindowControllerTest, TestFullscreen) {
-  // Note use of "controller", not "controller_"
-  scoped_nsobject<BrowserWindowController> controller;
-  controller.reset([[BrowserWindowControllerFakeFullscreen alloc]
-                        initWithBrowser:browser_helper_.browser()
-                          takeOwnership:NO]);
-  EXPECT_FALSE([controller isFullscreen]);
-  [controller setFullscreen:YES];
-  EXPECT_TRUE([controller isFullscreen]);
-  [controller setFullscreen:NO];
-  EXPECT_FALSE([controller isFullscreen]);
-
-  // Confirm the real fullscreen command doesn't return nil
+TEST_F(BrowserWindowControllerTest, TestFullScreenWindow) {
+  // Confirm the fullscreen command doesn't return nil.
+  // See BrowserWindowFullScreenControllerTest for more fullscreen tests.
   EXPECT_TRUE([controller_ fullscreenWindow]);
 }
 
-TEST_F(BrowserWindowControllerTest, TestActivate) {
-  // Note use of "controller", not "controller_"
-  scoped_nsobject<BrowserWindowController> controller;
-  controller.reset([[BrowserWindowControllerFakeFullscreen alloc]
-                        initWithBrowser:browser_helper_.browser()
-                          takeOwnership:NO]);
-  EXPECT_FALSE([controller isFullscreen]);
-
-  [controller activate];
-  NSWindow* frontmostWindow = [[NSApp orderedWindows] objectAtIndex:0];
-  EXPECT_EQ(frontmostWindow, [controller window]);
-
-  [controller setFullscreen:YES];
-  [controller activate];
-  frontmostWindow = [[NSApp orderedWindows] objectAtIndex:0];
-  EXPECT_EQ(frontmostWindow, [controller fullscreenWindow]);
-
-  // We have to cleanup after ourselves by unfullscreening.
-  [controller setFullscreen:NO];
-}
-
 TEST_F(BrowserWindowControllerTest, TestNormal) {
   // Force the bookmark bar to be shown.
   browser_helper_.profile()->GetPrefs()->
@@ -163,18 +107,15 @@ TEST_F(BrowserWindowControllerTest, TestNormal) {
   EXPECT_TRUE([controller_ isBookmarkBarVisible]);
 
   // And make sure a controller for a pop-up window is not normal.
-  scoped_ptr<Browser> popup_browser(Browser::CreateForPopup(
-                                      browser_helper_.profile()));
-  controller_.reset([[BrowserWindowController alloc]
-                              initWithBrowser:popup_browser.get()
-                                takeOwnership:NO]);
-  EXPECT_FALSE([controller_ isNormalWindow]);
-  EXPECT_FALSE([controller_ isBookmarkBarVisible]);
-
-  // The created BrowserWindowController gets autoreleased, so make
-  // sure we don't also release it.
-  // (Confirmed with valgrind).
-  controller_.release();
+  // popup_browser will be owned by its window.
+  Browser *popup_browser(Browser::CreateForPopup(browser_helper_.profile()));
+  NSWindow *cocoaWindow = popup_browser->window()->GetNativeHandle();
+  BrowserWindowController* controller =
+      static_cast<BrowserWindowController*>([cocoaWindow windowController]);
+  ASSERT_TRUE([controller isKindOfClass:[BrowserWindowController class]]);
+  EXPECT_FALSE([controller isNormalWindow]);
+  EXPECT_FALSE([controller isBookmarkBarVisible]);
+  [controller close];
 }
 
 @interface GTMTheme (BrowserThemeProviderInitialization)
@@ -470,23 +411,93 @@ TEST_F(BrowserWindowControllerTest, TestZoomFrame) {
 
 TEST_F(BrowserWindowControllerTest, TestFindBarOnTop) {
   FindBarBridge bridge;
-  [controller_.get() addFindBar:bridge.find_bar_cocoa_controller()];
+  [controller_ addFindBar:bridge.find_bar_cocoa_controller()];
 
   // Test that the Z-order of the find bar is on top of everything.
-  NSArray* subviews = [[[controller_.get() window] contentView] subviews];
+  NSArray* subviews = [[[controller_ window] contentView] subviews];
   NSUInteger findBar_index =
-      [subviews indexOfObject:[controller_.get() findBarView]];
+      [subviews indexOfObject:[controller_ findBarView]];
   EXPECT_NE(NSNotFound, findBar_index);
   NSUInteger toolbar_index =
-      [subviews indexOfObject:[controller_.get() toolbarView]];
+      [subviews indexOfObject:[controller_ toolbarView]];
   EXPECT_NE(NSNotFound, toolbar_index);
   NSUInteger bookmark_index =
-      [subviews indexOfObject:[controller_.get() bookmarkView]];
+      [subviews indexOfObject:[controller_ bookmarkView]];
   EXPECT_NE(NSNotFound, bookmark_index);
 
   EXPECT_GT(findBar_index, toolbar_index);
   EXPECT_GT(findBar_index, bookmark_index);
 }
 
+@interface BrowserWindowControllerFakeFullscreen : BrowserWindowController {
+ @private
+  // We release the window ourselves, so we don't have to rely on the unittest
+  // doing it for us.
+  scoped_nsobject<NSWindow> fullscreenWindow_;
+}
+@end
+
+class BrowserWindowFullScreenControllerTest : public CocoaTest {
+ public:
+  virtual void SetUp() {
+    CocoaTest::SetUp();
+    Browser* browser = browser_helper_.browser();
+    controller_ =
+        [[BrowserWindowControllerFakeFullscreen alloc] initWithBrowser:browser
+                                                         takeOwnership:NO];
+  }
+
+  virtual void TearDown() {
+    [controller_ close];
+    CocoaTest::TearDown();
+  }
+
+ public:
+  BrowserTestHelper browser_helper_;
+  BrowserWindowController* controller_;
+};
+
+TEST_F(BrowserWindowFullScreenControllerTest, TestFullscreen) {
+  EXPECT_FALSE([controller_ isFullscreen]);
+  [controller_ setFullscreen:YES];
+  EXPECT_TRUE([controller_ isFullscreen]);
+  [controller_ setFullscreen:NO];
+  EXPECT_FALSE([controller_ isFullscreen]);
+}
+
+TEST_F(BrowserWindowFullScreenControllerTest, TestActivate) {
+  EXPECT_FALSE([controller_ isFullscreen]);
+
+  [controller_ activate];
+  NSWindow* frontmostWindow = [[NSApp orderedWindows] objectAtIndex:0];
+  EXPECT_EQ(frontmostWindow, [controller_ window]);
+
+  [controller_ setFullscreen:YES];
+  [controller_ activate];
+  frontmostWindow = [[NSApp orderedWindows] objectAtIndex:0];
+  EXPECT_EQ(frontmostWindow, [controller_ fullscreenWindow]);
+
+  // We have to cleanup after ourselves by unfullscreening.
+  [controller_ setFullscreen:NO];
+}
+
+@implementation BrowserWindowControllerFakeFullscreen
+// Override fullscreenWindow to return a dummy window.  This isn't needed to
+// pass the test, but because the dummy window is only 100x100, it prevents the
+// real fullscreen window from flashing up and taking over the whole screen..
+// We have to return an actual window because layoutSubviews: looks at the
+// window's frame.
+- (NSWindow*)fullscreenWindow {
+  if (fullscreenWindow_.get())
+    return fullscreenWindow_.get();
+
+  fullscreenWindow_.reset(
+      [[NSWindow alloc] initWithContentRect:NSMakeRect(0,0,400,400)
+                                  styleMask:NSBorderlessWindowMask
+                                    backing:NSBackingStoreBuffered
+                                      defer:NO]);
+  return fullscreenWindow_.get();
+}
+@end
 
 /* TODO(???): test other methods of BrowserWindowController */

chrome/browser/cocoa/bubble_view_unittest.mm

@@ -2,48 +2,34 @@
 // 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/scoped_nsobject.h"
 #import "chrome/browser/cocoa/bubble_view.h"
 #include "chrome/browser/cocoa/cocoa_test_helper.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "testing/platform_test.h"
 
-class BubbleViewTest : public PlatformTest {
+class BubbleViewTest : public CocoaTest {
  public:
   BubbleViewTest() {
     NSRect frame = NSMakeRect(0, 0, 50, 50);
-    view_.reset([[BubbleView alloc] initWithFrame:frame
-                                    themeProvider:cocoa_helper_.window()]);
-    [cocoa_helper_.contentView() addSubview:view_.get()];
+    scoped_nsobject<BubbleView> view(
+        [[BubbleView alloc] initWithFrame:frame themeProvider:test_window()]);
+    view_ = view.get();
+    [[test_window() contentView] addSubview:view_];
     [view_ setContent:@"Hi there, I'm a bubble view"];
   }
 
-  CocoaTestHelper cocoa_helper_;
-  scoped_nsobject<BubbleView> view_;
+  BubbleView* view_;
 };
 
-// Test adding/removing from the view hierarchy, mostly to ensure nothing
-// leaks or crashes.
-TEST_F(BubbleViewTest, AddRemove) {
-  EXPECT_EQ(cocoa_helper_.contentView(), [view_ superview]);
-  [view_.get() removeFromSuperview];
-  EXPECT_FALSE([view_ superview]);
-}
-
-// Test drawing, mostly to ensure nothing leaks or crashes.
-TEST_F(BubbleViewTest, Display) {
-  [view_ display];
-}
+TEST_VIEW(BubbleViewTest, view_);
 
 // Test a nil themeProvider in init.
 TEST_F(BubbleViewTest, NilThemeProvider) {
   NSRect frame = NSMakeRect(0, 0, 50, 50);
-  view_.reset([[BubbleView alloc] initWithFrame:frame
-                                  themeProvider:nil]);
-  [cocoa_helper_.contentView() addSubview:view_.get()];
-  [view_ display];
+  scoped_nsobject<BubbleView> view(
+      [[BubbleView alloc] initWithFrame:frame themeProvider:nil]);
+  [[test_window() contentView] addSubview:view.get()];
+  [view display];
 }
 
 // Make sure things don't go haywire when given invalid or long strings.

chrome/browser/cocoa/cocoa_test_helper.h

@@ -105,15 +105,15 @@ class CocoaTest : public PlatformTest {
 // displaying the view to make sure it won't crash, as well as removing it
 // from a window. All tests that work with NSView subclasses and/or
 // NSViewController subclasses should use it.
-#define TEST_VIEW(test_fixture, view_member_name) \
+#define TEST_VIEW(test_fixture, test_view) \
   TEST_F(test_fixture, AddRemove##test_fixture) { \
-    scoped_nsobject<NSView> view([view_member_name retain]); \
-    EXPECT_EQ([test_window() contentView], [view_member_name superview]); \
-    [view_member_name removeFromSuperview]; \
-    EXPECT_FALSE([view_member_name superview]); \
+    scoped_nsobject<NSView> view([test_view retain]); \
+    EXPECT_EQ([test_window() contentView], [view superview]); \
+    [view removeFromSuperview]; \
+    EXPECT_FALSE([view superview]); \
   } \
   TEST_F(test_fixture, Display##test_fixture) { \
-    [view_member_name display]; \
+    [test_view display]; \
   }
 
 // The classes below are deprecated and will be removed shortly. Do not write

chrome/browser/cocoa/sad_tab_view_unittest.mm

@@ -20,6 +20,6 @@ class SadTabViewTest : public CocoaTest {
   SadTabView* view_;  // Weak. Owned by the view hierarchy.
 };
 
-TEST_VIEW(SadTabViewTest, view_)
+TEST_VIEW(SadTabViewTest, view_);
 
 }  // namespace