[Mac] Cleans up the StatusBubbleMac code.

chrome/browser/ui/cocoa/status_bubble_mac.mm

 // 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.
 
 #include "chrome/browser/ui/cocoa/status_bubble_mac.h"
 
 #include <limits>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 124 matching lines @@
     changes([NSAnimationContext currentContext]);
     // At this point, -animationForKey should have been called by CoreAnimation
     // to set up the animation to run. Verify this.
     DCHECK(completionHandler_ == nil);
     [NSAnimationContext endGrouping];
   }
 }
 
 @end
 
+// Mac implementation of the status bubble.
+//
+// Child windows interact with Spaces in interesting ways, so this code has to
+// follow these rules:
+//
+// 1) NSWindows cannot have zero size.  At times when the status bubble window
+//    has no specific size (for example, when hidden), its size is set to
+//    ui::kWindowSizeDeterminedLater.
+//
+// 2) Child window frames are in the coordinate space of the screen, not of the
+//    parent window.  If a child window has its origin at (0, 0), Spaces will
+//    position it in the corner of the screen but group it with the parent
+//    window in Spaces.  This causes Chrome windows to have a large (mostly
+//    blank) area in Spaces.  To avoid this, child windows always have their
+//    origin set to the lower-left corner of the window.
+//
+// 3) Detached child windows may show up as top-level windows in Spaces.  To
+//    avoid this, once the status bubble is Attach()ed to the parent, it is
+//    never detached (except in rare cases when reparenting to a fullscreen
+//    window).
+//
+// 4) To avoid unnecessary redraws, if a bubble is in the kBubbleHidden state,
+//    its size is always set to ui::kWindowSizeDeterminedLater.  The proper
+//    width for the current URL or status text is not calculated until the
+//    bubble leaves the kBubbleHidden state.
+
 StatusBubbleMac::StatusBubbleMac(NSWindow* parent, id delegate)
     : parent_(parent),
       delegate_(delegate),
       window_(nil),
       status_text_(nil),
       url_text_(nil),
       state_(kBubbleHidden),
       immediate_(false),
       is_expanded_(false),
       timer_factory_(this),
@@ skipping 16 matching lines @@
 
 void StatusBubbleMac::SetStatus(const base::string16& status) {
   SetText(status, false);
 }
 
 void StatusBubbleMac::SetURL(const GURL& url, const std::string& languages) {
   url_ = url;
   languages_ = languages;
 
   CGFloat bubble_width = NSWidth([window_ frame]);
-
-  // Reset frame size when bubble is hidden.
   if (state_ == kBubbleHidden) {
-    is_expanded_ = false;
-    NSRect frame = [window_ frame];
-    frame.size = ui::kWindowSizeDeterminedLater.size;
-    [window_ setFrame:frame display:NO];
+    DCHECK_EQ(ui::kWindowSizeDeterminedLater.size.width,
+              [window_ frame].size.width);
+    DCHECK_EQ(ui::kWindowSizeDeterminedLater.size.height,
+              [window_ frame].size.height);
     bubble_width = NSWidth(CalculateWindowFrame(/*expand=*/false));
   }
 
   int text_width = static_cast<int>(bubble_width -
                                     kBubbleViewTextPositionX -
                                     kTextPadding);
 
   // Scale from view to window coordinates before eliding URL string.
   NSSize scaled_width = NSMakeSize(text_width, 0);
   scaled_width = [[parent_ contentView] convertSize:scaled_width fromView:nil];
@@ skipping 246 matching lines @@
   [window_ orderFront:nil];
   [parent_ addChildWindow:window_ ordered:NSWindowAbove];
 
   [[window_ contentView] setThemeProvider:parent_];
 }
 
 void StatusBubbleMac::Detach() {
   DCHECK(is_attached());
 
   // Magic setFrame: See http://crbug.com/58506 and http://crrev.com/3564021 .
-  [window_ setFrame:CalculateWindowFrame(/*expand=*/false) display:NO];
+  // TODO(rohitrao): Does the frame size actually matter here?  Can we always
+  // set it to kWindowSizeDeterminedLater?

[rohitrao (ping after 24h), 2015/03/03 15:54:00]

I think we can probably get away with always using kWindowSizeDeterminedLater
here.  Even if the bubble is currently showing, it's almost certainly got the
wrong size for the new window, so it won't matter if it's (1, 1) or some other
larger size.  In either case, we still have the call to UpdateSizeAndPosition()
after reparenting.

I made the most conservative change I could, but I think we could simplify this
further if we wanted.

+  NSRect frame = CalculateWindowFrame(/*expand=*/false);
+  if (state_ == kBubbleHidden) {
+    frame.size = ui::kWindowSizeDeterminedLater.size;

[erikchen, 2015/03/03 18:19:50]

"""
NSRect frame = ui::kWindowSizeDeterminedLater.size;
if (state_ != kBubbleHidden)
  frame = CalculateWindowFrame(/*expand=*/false);
"""

saves a call to CalculateWindowFrame.

[rohitrao (ping after 24h), 2015/03/03 18:53:51]

Done.

+  }
+  [window_ setFrame:frame display:NO];
   [parent_ removeChildWindow:window_];  // See crbug.com/28107 ...
   [window_ orderOut:nil];               // ... and crbug.com/29054.
 
   [[window_ contentView] setThemeProvider:nil];
 }
 
 void StatusBubbleMac::AnimationDidStop() {
   DCHECK([NSThread isMainThread]);
   DCHECK(state_ == kBubbleShowingFadeIn || state_ == kBubbleHidingFadeOut);
   DCHECK(is_attached());
 
   if (state_ == kBubbleShowingFadeIn) {
     DCHECK_EQ([[window_ animator] alphaValue], kBubbleOpacity);
     SetState(kBubbleShown);
   } else {
     DCHECK_EQ([[window_ animator] alphaValue], 0.0);
     SetState(kBubbleHidden);
   }
 }
 
 void StatusBubbleMac::SetState(StatusBubbleState state) {
   if (state == state_)
     return;
 
   if (state == kBubbleHidden) {
+    is_expanded_ = false;

[erikchen, 2015/03/03 18:19:50]

is_expanded_ gets set to false in several locations:

SetURL()
Hide()
and now here.

This is confusing. There's almost certainly some overlap of work, and it's
unclear to me when exactly is_expanded_ is supposed to be set to false.

[rohitrao (ping after 24h), 2015/03/03 18:53:51]

Agreed.  I don't fully understand when it needs to be set.  I moved this call
from SetURL() to here because it didn't seem to belong in SetURL().  I suspect
that this call is enough on its own, but not positive.

[rohitrao (ping after 24h), 2015/03/03 18:53:51]

Agreed.  I don't fully understand when it needs to be set.  I moved this call
from SetURL() to here because it didn't seem to belong in SetURL().  I suspect
that this call is enough on its own, but not positive.

+
     // When hidden (with alpha of 0), make the window have the minimum size,
     // while still keeping the same origin. It's important to not set the
     // origin to 0,0 as that will cause the window to use more space in
     // Expose/Mission Control. See http://crbug.com/81969.
     //
     // Also, doing it this way instead of detaching the window avoids bugs with
     // Spaces and Cmd-`. See http://crbug.com/31821 and http://crbug.com/61629.
     NSRect frame = [window_ frame];
     frame.size = ui::kWindowSizeDeterminedLater.size;
     [window_ setFrame:frame display:YES];
@@ skipping 225 matching lines @@
   [NSAnimationContext beginGrouping];
   [[NSAnimationContext currentContext] setDuration:kExpansionDurationSeconds];
   [[window_ animator] setFrame:actual_window_frame display:YES];
   [NSAnimationContext endGrouping];
 }
 
 void StatusBubbleMac::UpdateSizeAndPosition() {
   if (!window_)
     return;
 
-  // Hidden bubbles always have size equal to ui::kWindowSizeDeterminedLater.
+  // There is no need to update the size if the bubble is hidden.
   if (state_ == kBubbleHidden) {
-    NSRect frame = [window_ frame];
-    frame.size = ui::kWindowSizeDeterminedLater.size;
-    [window_ setFrame:frame display:YES];
+    // Verify that hidden bubbles always have size equal to
+    // ui::kWindowSizeDeterminedLater.
+    DCHECK_EQ(ui::kWindowSizeDeterminedLater.size.width,
+              [window_ frame].size.width);
+    DCHECK_EQ(ui::kWindowSizeDeterminedLater.size.height,
+              [window_ frame].size.height);
     return;
   }
 
   SetFrameAvoidingMouse(CalculateWindowFrame(/*expand=*/false),
                         GetMouseLocation());
 }
 
 void StatusBubbleMac::SwitchParentWindow(NSWindow* parent) {
   DCHECK(parent);
   DCHECK(is_attached());
@@ skipping 48 matching lines @@
     }
 
     // Round the top corners when the bubble is below the parent window.
     if (NSMinY(window_frame) < NSMinY(parent_frame)) {
       corner_flags |= kRoundedTopLeftCorner | kRoundedTopRightCorner;
     }
   }
 
   return corner_flags;
 }