[Mac] Cleans up the StatusBubbleMac code.

chrome/browser/ui/cocoa/status_bubble_mac.mm

Index: chrome/browser/ui/cocoa/status_bubble_mac.mm
diff --git a/chrome/browser/ui/cocoa/status_bubble_mac.mm b/chrome/browser/ui/cocoa/status_bubble_mac.mm
index 7273b0d6f889b9fcfdcf7c44e830325aa2e9a599..f7329be7610a8f08f78a08a35b9ae45b8287d7e2 100644
--- a/chrome/browser/ui/cocoa/status_bubble_mac.mm
+++ b/chrome/browser/ui/cocoa/status_bubble_mac.mm
@@ -142,6 +142,32 @@ const CGFloat kExpansionDurationSeconds = 0.125;
 
 @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),
@@ -178,13 +204,11 @@ void StatusBubbleMac::SetURL(const GURL& url, const std::string& languages) {
   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));
   }
 
@@ -451,7 +475,13 @@ 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.
 
@@ -477,6 +507,8 @@ void StatusBubbleMac::SetState(StatusBubbleState 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
@@ -722,11 +754,14 @@ 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;
   }