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

chrome/browser/cocoa/browser_window_controller.mm

 // Copyright (c) 2009 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.
 
 #include <Carbon/Carbon.h>
 
 #include "base/mac_util.h"
 #include "base/scoped_nsdisable_screen_updates.h"
 #import "base/scoped_nsobject.h"
 #include "base/sys_string_conversions.h"
@@ skipping 42 matching lines @@
 
 @interface GTMTheme (BrowserThemeProviderInitialization)
 + (GTMTheme*)themeWithBrowserThemeProvider:(BrowserThemeProvider*)provider
                             isOffTheRecord:(BOOL)offTheRecord;
 @end
 
 @interface NSWindow (NSPrivateApis)
 // Note: These functions are private, use -[NSObject respondsToSelector:]
 // before calling them.
 
-- (void)setAutorecalculatesContentBorderThickness:(BOOL)b
-                                          forEdge:(NSRectEdge)e;
-- (void)setContentBorderThickness:(CGFloat)b forEdge:(NSRectEdge)e;
-
 - (void)setBottomCornerRounded:(BOOL)rounded;
 
 - (NSRect)_growBoxRect;
 
 @end
 
 
 @interface BrowserWindowController(Private)
 
 // Saves the window's position in the local state preferences.
@@ skipping 32 matching lines @@
   // can override it in a unit test.
   NSString* nibpath = [mac_util::MainAppBundle()
                         pathForResource:@"BrowserWindow"
                                  ofType:@"nib"];
   if ((self = [super initWithWindowNibPath:nibpath owner:self])) {
     DCHECK(browser);
     browser_.reset(browser);
     ownsBrowser_ = ownIt;
     tabObserver_.reset(
         new TabStripModelObserverBridge(browser->tabstrip_model(), self));
-    windowShim_.reset(new BrowserWindowCocoa(browser, self, [self window]));
+    NSWindow* window = [self window];
+    windowShim_.reset(new BrowserWindowCocoa(browser, self, window));
 
-    // The window is now fully realized and |-windowDidLoad:| has been
-    // called. We shouldn't do much in wDL because |windowShim_| won't yet
-    // be initialized (as it's called in response to |[self window]| above).
-    // Retain it per the comment in the header.
-    window_.reset([[self window] retain]);
 
     // Sets the window to not have rounded corners, which prevents
     // the resize control from being inset slightly and looking ugly.
-    if ([window_ respondsToSelector:@selector(setBottomCornerRounded:)])
-      [window_ setBottomCornerRounded:NO];
+    if ([window respondsToSelector:@selector(setBottomCornerRounded:)])
+      [window setBottomCornerRounded:NO];
 
     [self setTheme];
 
     // Get the most appropriate size for the window, then enforce the
     // minimum width and height. The window shim will handle flipping
     // the coordinates for us so we can use it to save some code.
     // Note that this may leave a significant portion of the window
     // offscreen, but there will always be enough window onscreen to
     // drag the whole window back into view.
     NSSize minSize = [[self window] minSize];
@@ skipping 74 matching lines @@
                                         initWithBrowser:browser_.get()
                                          resizeDelegate:self]);
       [[[self window] contentView] addSubview:[extensionShelfController_ view]];
       [extensionShelfController_ wasInsertedIntoWindow];
     }
 
     // Force a relayout of all the various bars.
     [self layoutSubviews];
 
     // Create the bridge for the status bubble.
-    statusBubble_.reset(new StatusBubbleMac([self window], self));
+    statusBubble_ = new StatusBubbleMac([self window], self);
   }
   return self;
 }
 
 - (void)dealloc {
   browser_->CloseAllTabs();
   [downloadShelfController_ exiting];
 
   // Under certain testing configurations we may not actually own the browser.
   if (ownsBrowser_ == NO)
     browser_.release();
-  // Since |window_| outlives our obj-c shutdown sequence, clear out the
-  // delegate so nothing tries to call us back in the meantime as part of
-  // window destruction.
-  [window_ setDelegate:nil];
 
   [super dealloc];
 }
 
 // Access the C++ bridge between the NSWindow and the rest of Chromium
 - (BrowserWindow*)browserWindow {
   return windowShim_.get();
 }
 
 - (void)destroyBrowser {
   [NSApp removeWindowsItem:[self window]];
 
   // We need the window to go away now.
-  [self autorelease];
+  // 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.
+  [self performSelector:@selector(autorelease)
+             withObject:nil
+             afterDelay:0];
 }
 
 // Called when the window meets the criteria to be closed (ie,
 // |-windowShouldClose:| returns YES). We must be careful to preserve the
 // semantics of BrowserWindow::Close() and not call the Browser's dtor directly
 // from this method.
 - (void)windowWillClose:(NSNotification*)notification {
-  // Don't update the window any longer.
-  // TODO(shess,dmaclach): This is a work-around for some cases where
-  // the window's views were living longer than this controller, and
-  // were then being re-displayed.  Better would be to have it live
-  // the appropriate amount of time, at which point we can remove
-  // this.  [And perhaps the funky -autorelease below can be fixed.]
-  [window_ setAutodisplay:NO];
-
+  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 use call it after a zero-length delay, which gets us back
   // to the main event loop.
   [self performSelector:@selector(autorelease)
              withObject:nil
              afterDelay:0];
 }
 
 - (void)attachConstrainedWindow:(ConstrainedWindowMac*)window {
@@ skipping 61 matching lines @@
       rwhv->SetActive(true);
   }
 }
 
 // Called when we are deactivated (when we lose focus).
 - (void)windowDidResignKey:(NSNotification*)notification {
   // If our app is still active and we're still the key window, ignore this
   // message, since it just means that a menu extra (on the "system status bar")
   // was activated; we'll get another |-windowDidResignKey| if we ever really
   // lose key window status.
-  if ([NSApp isActive] &&
-      ([NSApp keyWindow] == static_cast<NSWindow*>(window_)))
+  if ([NSApp isActive] && ([NSApp keyWindow] == [self window]))
     return;
 
   // We need to deactivate the controls (in the "WebView"). To do this, get the
   // selected TabContents's RenderWidgetHostView and tell it to deactivate.
   if (TabContents* contents = browser_->GetSelectedTabContents()) {
     if (RenderWidgetHostView* rwhv = contents->render_widget_host_view())
       rwhv->SetActive(false);
   }
 }
 
@@ skipping 52 matching lines @@
     frame.origin.x = frame.origin.x + frame.size.width - zoomedWidth;
   else
     frame.origin.x = currentFrame.origin.x;
 
   // Set the width. Don't touch y or height.
   frame.size.width = zoomedWidth;
 
   return frame;
 }
 
+- (void)activate {
+  [[self window] makeKeyAndOrderFront:self];
+}
+
 // Determine whether we should let a window zoom/unzoom to the given |newFrame|.
 // We avoid letting unzoom move windows between screens, because it's really
 // strange and unintuitive.
 - (BOOL)windowShouldZoom:(NSWindow*)window toFrame:(NSRect)newFrame {
   // Figure out which screen |newFrame| is on.
   NSScreen* newScreen = nil;
   CGFloat newScreenOverlapArea = 0.0;
   for (NSScreen* screen in [NSScreen screens]) {
     NSRect overlap = NSIntersectionRect(newFrame, [screen frame]);
     CGFloat overlapArea = overlap.size.width * overlap.size.height;
@@ skipping 193 matching lines @@
 
 - (GTMWindowSheetController*)sheetController {
   return [tabStripController_ sheetController];
 }
 
 - (LocationBar*)locationBar {
   return [toolbarController_ locationBar];
 }
 
 - (StatusBubbleMac*)statusBubble {
-  return statusBubble_.get();
+  return statusBubble_;
 }
 
 - (void)updateToolbarWithContents:(TabContents*)tab
                shouldRestoreState:(BOOL)shouldRestore {
   [toolbarController_ updateToolbarWithContents:tab
                              shouldRestoreState:shouldRestore];
 }
 
 - (void)setStarredState:(BOOL)isStarred {
   [toolbarController_ setStarredState:isStarred];
 }
 
 // Return the rect, in WebKit coordinates (flipped), of the window's grow box
 // in the coordinate system of the content area of the currently selected tab.
 // |windowGrowBox| needs to be in the window's coordinate system.
 - (NSRect)selectedTabGrowBoxRect {
-  if (![window_ respondsToSelector:@selector(_growBoxRect)])
+  NSWindow* window = [self window];
+  if (![window respondsToSelector:@selector(_growBoxRect)])
     return NSZeroRect;
 
   // Before we return a rect, we need to convert it from window coordinates
   // to tab content area coordinates and flip the coordinate system.
   NSRect growBoxRect =
-      [[self tabContentArea] convertRect:[window_ _growBoxRect] fromView:nil];
+      [[self tabContentArea] convertRect:[window _growBoxRect] fromView:nil];
   growBoxRect.origin.y =
       [[self tabContentArea] frame].size.height - growBoxRect.size.height -
       growBoxRect.origin.y;
   return growBoxRect;
 }
 
 // Accept tabs from a BrowserWindowController with the same Profile.
 - (BOOL)canReceiveFrom:(TabWindowController*)source {
   if (![source isKindOfClass:[BrowserWindowController class]]) {
     return NO;
@@ skipping 82 matching lines @@
 }
 
 - (TabStripController*)tabStripController {
   return tabStripController_;
 }
 
 - (void)setIsLoading:(BOOL)isLoading {
   [toolbarController_ setIsLoading:isLoading];
 }
 
-- (void)activate {
-  // TODO(rohitrao): Figure out the proper activation behavior for fullscreen
-  // windows.  When there is only one window open, this code makes sense, but
-  // what should we do if we need to activate a non-fullscreen background window
-  // while a fullscreen window has focus?  http://crbug.com/24893
-  if (fullscreen_)
-    [fullscreen_window_ makeKeyAndOrderFront:self];
-  else
-    [window_ makeKeyAndOrderFront:self];
-}
-
 // Make the location bar the first responder, if possible.
 - (void)focusLocationBar {
   [toolbarController_ focusLocationBar];
 }
 
 - (void)layoutTabs {
   [tabStripController_ layoutTabs];
 }
 
 - (TabWindowController*)detachTabToNewWindow:(TabView*)tabView {
@@ skipping 134 matching lines @@
 
 - (NSWindow*)fullscreenWindow {
   return [[[FullscreenWindow alloc] initForScreen:[[self window] screen]]
            autorelease];
 }
 
 - (void)setFullscreen:(BOOL)fullscreen {
   if (![self supportsFullscreen])
       return;
 
-  fullscreen_ = fullscreen;
+  NSWindow* window = [self window];
+
+  // Retain the contentView while we remove it from its superview.
+  scoped_nsobject<NSView> content([[window contentView] retain]);
+
+  // Disable autoresizing of subviews while we move views around.  This
+  // prevents spurious renderer resizes.
+  [content setAutoresizesSubviews:NO];
+  [content removeFromSuperview];
+
+  NSWindow* dstWindow = nil;
   if (fullscreen) {
-    // Move content to a new fullscreen window
-    NSView* content = [[self window] contentView];
-    // Disable autoresizing of subviews while we move views around.  This
-    // prevents spurious renderer resizes.
-    [content setAutoresizesSubviews:NO];
-    fullscreen_window_.reset([[self fullscreenWindow] retain]);
-    [content removeFromSuperview];
-    [fullscreen_window_ setContentView:content];
-    [self setWindow:fullscreen_window_.get()];
-    [window_ setWindowController:nil];
-    [window_ setDelegate:nil];
-    // Required for proper event dispatch.
-    [fullscreen_window_ setWindowController:self];
-    [fullscreen_window_ setDelegate:self];
+    DCHECK(!savedRegularWindow_);
+    savedRegularWindow_ = [window retain];
+    dstWindow = [self fullscreenWindow];
+  } else {
+    DCHECK(savedRegularWindow_);
+    dstWindow = [savedRegularWindow_ autorelease];
+    savedRegularWindow_ = nil;
+  }
 
-    // Minimize our UI.  This call triggers a relayout, so it needs to come
-    // after we move the contentview to the new window.
-    [self adjustUIForFullscreen:fullscreen];
-    // Show one window, hide the other.
-    [fullscreen_window_ makeKeyAndOrderFront:self];
-    [content setAutoresizesSubviews:YES];
-    [content setNeedsDisplay:YES];
-    [window_ orderOut:self];
-  } else {
-    NSView* content = [fullscreen_window_ contentView];
-    // Disable autoresizing of subviews while we move views around.  This
-    // prevents spurious renderer resizes.
-    [content setAutoresizesSubviews:NO];
-    [content removeFromSuperview];
-    [window_ setContentView:content];
-    [fullscreen_window_ setDelegate:nil];
-    [fullscreen_window_ setWindowController:nil];
-    [window_ setWindowController:self];
-    [window_ setDelegate:self];
-    [self setWindow:window_.get()];
-    // This call triggers a relayout, so it needs to come after we move the
-    // contentview to the new window.
-    [self adjustUIForFullscreen:fullscreen];
-    [content setAutoresizesSubviews:YES];
-    [content setNeedsDisplay:YES];
+  // With this call, valgrind yells at me about "Conditional jump or
+  // move depends on uninitialised value(s)".  The error happens in
+  // -[NSThemeFrame drawOverlayRect:].  I'm pretty convinced this is
+  // an Apple bug, but there is no visual impact.  I have been
+  // unable to tickle it away with other window or view manipulation
+  // Cocoa calls.  Stack added to suppressions_mac.txt.

[Scott Hess - ex-Googler, 2009/10/26 19:45:21]

Just checking - does this comment now apply to -setAutoresizesSubviews:?  Before
it was on -makeKeyAndOrderFront:.

+  [content setAutoresizesSubviews:YES];

[Scott Hess - ex-Googler, 2009/10/26 19:45:21]

Previously this line was way down the list.  The comment at disable makes it
sounds like some of these methods caused those extra resizes ... maybe
-adjustUIForFullScreen: could cause resizing?

+  [dstWindow setContentView:content];
+  [window setWindowController:nil];
+  [window setDelegate:nil];
+  [self setWindow:dstWindow];
+  [dstWindow setWindowController:self];
+  [dstWindow setDelegate:self];
+  [self adjustUIForFullscreen:fullscreen];
+  [dstWindow makeKeyAndOrderFront:self];
 
-    // With this call, valgrind yells at me about "Conditional jump or
-    // move depends on uninitialised value(s)".  The error happens in
-    // -[NSThemeFrame drawOverlayRect:].  I'm pretty convinced this is
-    // an Apple bug, but there is no visual impact.  I have been
-    // unable to tickle it away with other window or view manipulation
-    // Cocoa calls.  Stack added to suppressions_mac.txt.
-    [window_ makeKeyAndOrderFront:self];
-
-    [fullscreen_window_ close];
-    fullscreen_window_.reset(nil);
-  }
+  [window orderOut:self];
 }
 
 - (BOOL)isFullscreen {
-  return fullscreen_;
+  return savedRegularWindow_ != nil;
 }
 
 // Called by the bookmark bar to open a URL.
 - (void)openBookmarkURL:(const GURL&)url
             disposition:(WindowOpenDisposition)disposition {
   browser_->OpenURL(url, GURL(), disposition, PageTransition::AUTO_BOOKMARK);
 }
 
 - (NSInteger)numberOfTabs {
   return browser_->tabstrip_model()->count();
@@ skipping 82 matching lines @@
   // Adjust top-left based on our knowledge of how the view looks.
   p.x -= 2;
   p.y += 7;
 
   return p;
 }
 
 // Show the bookmark bubble (e.g. user just clicked on the STAR).
 - (void)showBookmarkBubbleForURL:(const GURL&)url
                alreadyBookmarked:(BOOL)alreadyBookmarked {
-  BookmarkModel* model = browser_->profile()->GetBookmarkModel();
-  const BookmarkNode* node = model->GetMostRecentlyAddedNodeForURL(url);
+  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];
+    NSWindow* bookmarkBubbleWindow = [bookmarkBubbleController_ window];
+    NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+    [nc addObserver:self
+           selector:@selector(bubbleWindowWillClose:)
+               name:NSWindowWillCloseNotification
+             object:bookmarkBubbleWindow];
+    [bookmarkBubbleController_ showWindow:self];
+  }
+}
 
-  // Bring up the bubble.  But clicking on STAR while the bubble is
-  // open should make it go away.
-  if (bookmarkBubbleController_.get()) {
-    [self doneWithBubbleController:bookmarkBubbleController_.get()];
-  } else {
-    bookmarkBubbleController_.reset([[BookmarkBubbleController alloc]
-                                      initWithDelegate:self
-                                          parentWindow:[self window]
-                                      topLeftForBubble:[self topLeftForBubble]
-                                                 model:model
-                                                  node:node
-                                     alreadyBookmarked:alreadyBookmarked]);
-    [bookmarkBubbleController_ showWindow];
-  }
+// Notification sent when our bubble window is closed.
+- (void)bubbleWindowWillClose:(NSNotification*)notification {
+  DCHECK([[notification object] windowController] == bookmarkBubbleController_);
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  [nc removeObserver:self
+                name:NSWindowWillCloseNotification
+              object:[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];
 }
 
-// Implement BookmarkBubbleControllerDelegate
-- (void)doneWithBubbleController:(BookmarkBubbleController*)controller {
-  bookmarkBubbleController_.reset(nil);
-}
 
 // Delegate method called when window is resized.
 - (void)windowDidResize:(NSNotification*)notification {
   // Resize (and possibly move) the status bubble. Note that we may get called
   // when the status bubble does not exist.
-  if (statusBubble_.get())
+  if (statusBubble_) {
     statusBubble_->UpdateSizeAndPosition();
+  }
 }
 
 @end
 
 @implementation BrowserWindowController (Private)
 
 // If the browser is in incognito mode, install the image view to decorate
 // the window at the upper right. Use the same base y coordinate as the
 // tab strip.
 - (void)installIncognitoBadge {
@@ skipping 124 matching lines @@
 }
 
 // Private method to layout browser window subviews.  Positions the toolbar and
 // the infobar above the tab content area.  Positions the download shelf below
 // the tab content area.  If the toolbar is not a child of the contentview, this
 // method will not leave room for it.  If we are currently running in fullscreen
 // mode, or if the tabstrip is not a descendant of the window, this method fills
 // the entire content area.  Otherwise, this method places the topmost view
 // directly beneath the tabstrip.
 - (void)layoutSubviews {
-  NSView* contentView = fullscreen_ ? [fullscreen_window_ contentView]
-                                    : [[self window] contentView];
+  NSWindow* window = [self window];
+  NSView* contentView = [window contentView];
   NSRect contentFrame = [contentView frame];
   int maxY = NSMaxY(contentFrame);
   int minY = NSMinY(contentFrame);
-  if (!fullscreen_ && [self isNormalWindow]) {
+  if (![self isFullscreen] && [self isNormalWindow]) {
     maxY = NSMinY([[self tabStripView] frame]);
   }
   DCHECK_GE(maxY, minY);
 
   // Suppress title drawing for normal windows (popups use normal
   // window title bars).
-  [window_ setShouldHideTitle:[self isNormalWindow]];
+  if ([window respondsToSelector:@selector(setShouldHideTitle:)]) {
+    [(id)window setShouldHideTitle:[self isNormalWindow]];
+  }
 
   // Place the toolbar at the top of the reserved area, but only if we're not in
   // fullscreen mode.
   NSView* toolbarView = [toolbarController_ view];
   NSRect toolbarFrame = [toolbarView frame];
-  if (!fullscreen_) {
+  if (![self isFullscreen]) {
     // The toolbar is present in the window, so we make room for it.
     toolbarFrame.origin.x = 0;
     toolbarFrame.origin.y = maxY - NSHeight(toolbarFrame);
     toolbarFrame.size.width = NSWidth(contentFrame);
     maxY -= NSHeight(toolbarFrame);
   }
   [toolbarView setFrame:toolbarFrame];
 
   if ([bookmarkBarController_ isAlwaysVisible]) {
     NSView* bookmarkBarView = [bookmarkBarController_ view];
@@ skipping 223 matching lines @@
   if (frameOverlayInactiveImage) {
     [theme setValue:frameOverlayInactiveImage
        forAttribute:@"overlay"
               style:GTMThemeStyleWindow
               state:GTMThemeStateInactiveWindow];
   }
 
   return theme;
 }
 @end