Reland performance fixes for StatusBubbleMac.

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"
+#include "base/debug/stack_trace.h"
 #include "base/mac/mac_util.h"
 #include "base/mac/scoped_block.h"
 #include "base/mac/sdk_forward_declarations.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #import "chrome/browser/ui/cocoa/bubble_view.h"
 #include "chrome/browser/ui/elide_url.h"
 #include "net/base/net_util.h"
@@ skipping 114 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 15 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;
 
-  NSRect frame = [window_ frame];
-
-  // Reset frame size when bubble is hidden.
+  CGFloat bubble_width = NSWidth([window_ frame]);
   if (state_ == kBubbleHidden) {
-    is_expanded_ = false;
-    frame.size.width = NSWidth(CalculateWindowFrame(/*expand=*/false));
-    [window_ setFrame:frame display:NO];
+    // TODO(rohitrao): The window size is expected to be (1,1) whenever the
+    // window is hidden, but the GPU bots are hitting cases where this is not
+    // true.  Instead of enforcing this invariant with a DCHECK, add temporary
+    // logging to try and debug it and fix up the window size if needed.
+    // This logging is temporary and should be removed: crbug.com/467998
+    NSRect frame = [window_ frame];
+    if (!CGSizeEqualToSize(frame.size, ui::kWindowSizeDeterminedLater.size)) {
+      LOG(ERROR) << "Window size should be (1,1), but is instead ("
+                 << frame.size.width << "," << frame.size.height << ")";
+      LOG(ERROR) << base::debug::StackTrace().ToString();
+      frame.size = ui::kWindowSizeDeterminedLater.size;
+      [window_ setFrame:frame display:NO];
+    }
+    bubble_width = NSWidth(CalculateWindowFrame(/*expand=*/false));
   }
 
-  int text_width = static_cast<int>(NSWidth(frame) -
+  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];
   text_width = static_cast<int>(scaled_width.width);
   NSFont* font = [[window_ contentView] font];
   gfx::FontList font_list_chr(
       gfx::Font(base::SysNSStringToUTF8([font fontName]), [font pointSize]));
@@ skipping 53 matching lines @@
 
   bool show = true;
   if ([*main length] > 0)
     [[window_ contentView] setContent:*main];
   else if ([*backup length] > 0)
     [[window_ contentView] setContent:*backup];
   else
     show = false;
 
   if (show) {
+    // Call StartShowing() first to update the current bubble state before
+    // calculating a new size.
+    StartShowing();
     UpdateSizeAndPosition();
-    StartShowing();
   } else {
     StartHiding();
   }
 }
 
 void StatusBubbleMac::Hide() {
   CancelTimer();
   CancelExpandTimer();
   is_expanded_ = false;
 
   bool fade_out = false;
   if (state_ == kBubbleHidingFadeOut || state_ == kBubbleShowingFadeIn) {
     SetState(kBubbleHidingFadeOut);
 
     if (!immediate_) {
       // An animation is in progress.  Cancel it by starting a new animation.
       // Use kMinimumTimeInterval to set the opacity as rapidly as possible.
       fade_out = true;
       AnimateWindowAlpha(0.0, kMinimumTimeInterval);
     }
   }
 
+  NSRect frame = CalculateWindowFrame(/*expand=*/false);
   if (!fade_out) {
     // No animation is in progress, so the opacity can be set directly.
     [window_ setAlphaValue:0.0];
     SetState(kBubbleHidden);
+    frame.size = ui::kWindowSizeDeterminedLater.size;
   }
 
   // Stop any width animation and reset the bubble size.
   if (!immediate_) {
     [NSAnimationContext beginGrouping];
     [[NSAnimationContext currentContext] setDuration:kMinimumTimeInterval];
-    [[window_ animator] setFrame:CalculateWindowFrame(/*expand=*/false)
-                         display:NO];
+    [[window_ animator] setFrame:frame display:NO];
     [NSAnimationContext endGrouping];
   } else {
-    [window_ setFrame:CalculateWindowFrame(/*expand=*/false) display:NO];
+    [window_ setFrame:frame display:NO];
   }
 
   [status_text_ release];
   status_text_ = nil;
   [url_text_ release];
   url_text_ = nil;
 }
 
 void StatusBubbleMac::SetFrameAvoidingMouse(
     NSRect window_frame, const gfx::Point& mouse_pos) {
@@ skipping 127 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?
+  NSRect frame = [window_ frame];
+  frame.size = ui::kWindowSizeDeterminedLater.size;
+  if (state_ != kBubbleHidden) {
+    frame = CalculateWindowFrame(/*expand=*/false);
+  }
+  [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;
+
     // 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;
 
+  // There is no need to update the size if the bubble is hidden.
+  if (state_ == kBubbleHidden) {
+    // Verify that hidden bubbles always have size equal to
+    // ui::kWindowSizeDeterminedLater.
+
+    // TODO(rohitrao): The GPU bots are hitting cases where this is not true.
+    // Instead of enforcing this invariant with a DCHECK, add temporary logging
+    // to try and debug it and fix up the window size if needed.
+    // This logging is temporary and should be removed: crbug.com/467998
+    NSRect frame = [window_ frame];
+    if (!CGSizeEqualToSize(frame.size, ui::kWindowSizeDeterminedLater.size)) {
+      LOG(ERROR) << "Window size should be (1,1), but is instead ("
+                 << frame.size.width << "," << frame.size.height << ")";
+      LOG(ERROR) << base::debug::StackTrace().ToString();
+      frame.size = ui::kWindowSizeDeterminedLater.size;
+    }
+
+    // During the fullscreen animation, the parent window's origin may change
+    // without updating the status bubble.  To avoid animation glitches, always
+    // update the bubble's origin to match the parent's, even when hidden.
+    frame.origin = [parent_ convertBaseToScreen:([parent_ frame].origin)];

[rohitrao (ping after 24h), 2015/04/27 22:26:09]

I realized that the actual origin doesn't matter quite so much, as long as it's
within the parent window somewhere.  The SetFrameAvoidingMouse() code has logic
to handle the download shelf -- we don't really care about that, because the
bubble is hidden anyways.  Simply using the parent's origin should be just fine
(from the perspective of AppKit, it doesn't care if the download shelf is
present or absent anyways).

+    [window_ setFrame:frame display:YES];

[erikchen, 2015/04/27 22:33:26]

display:YES forces a redraw, even if the origin hasn't changed. Maybe we should
only perform a redraw if the frame has actually changed?

+    return;
+  }
+
   SetFrameAvoidingMouse(CalculateWindowFrame(/*expand=*/false),
                         GetMouseLocation());
 }
 
 void StatusBubbleMac::SwitchParentWindow(NSWindow* parent) {
   DCHECK(parent);
   DCHECK(is_attached());
 
   Detach();
   parent_ = parent;
@@ skipping 45 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;
 }