Change status bubble rendering at hidpi and when there is no client edge

chrome/browser/ui/views/status_bubble_views.cc

Index: chrome/browser/ui/views/status_bubble_views.cc
diff --git a/chrome/browser/ui/views/status_bubble_views.cc b/chrome/browser/ui/views/status_bubble_views.cc
index 036f791a4fd1b735cd67e7ccaf7e7921b31c693a..fde8312d438874474d2962ce91d1804fd46f74cb 100644
--- a/chrome/browser/ui/views/status_bubble_views.cc
+++ b/chrome/browser/ui/views/status_bubble_views.cc
@@ -135,7 +135,8 @@ class StatusBubbleViews::StatusView : public views::View {
   };
 
   StatusView(views::Widget* popup,
-             const ui::ThemeProvider* theme_provider);
+             const ui::ThemeProvider* theme_provider,
+             const BrowserView* browser_view);
   ~StatusView() override;
 
   // Set the bubble text to a certain value, hides the bubble if text is
@@ -196,6 +197,8 @@ class StatusBubbleViews::StatusView : public views::View {
   // Holds the theme provider of the frame that created us.
   const ui::ThemeProvider* theme_provider_;
 
+  const BrowserView* browser_view_;
+
   base::WeakPtrFactory<StatusBubbleViews::StatusView> timer_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(StatusView);
@@ -203,12 +206,14 @@ class StatusBubbleViews::StatusView : public views::View {
 
 StatusBubbleViews::StatusView::StatusView(
     views::Widget* popup,
-    const ui::ThemeProvider* theme_provider)
+    const ui::ThemeProvider* theme_provider,
+    const BrowserView* browser_view)
     : state_(BUBBLE_HIDDEN),
       style_(STYLE_STANDARD),
       animation_(new StatusViewAnimation(this, 0, 0)),
       popup_(popup),
       theme_provider_(theme_provider),
+      browser_view_(browser_view),
       timer_factory_(this) {}
 
 StatusBubbleViews::StatusView::~StatusView() {
@@ -366,15 +371,13 @@ const char* StatusBubbleViews::StatusView::GetClassName() const {
 }
 
 void StatusBubbleViews::StatusView::OnPaint(gfx::Canvas* canvas) {
-  SkPaint paint;
-  paint.setStyle(SkPaint::kFill_Style);
-  paint.setAntiAlias(true);
-  SkColor toolbar_color = theme_provider_->GetColor(
-      ThemeProperties::COLOR_TOOLBAR);
-  paint.setColor(toolbar_color);
 
   gfx::Rect popup_bounds = popup_->GetWindowBoundsInScreen();

[Peter Kasting, 2016/08/13 06:13:07]

Nit: I would avoid GetWindowBoundsInScreen() here if you can, because it does a
lossy conversion to DIPs.  Plus you only care about the width and height anyway,
not the origin, so it seems like just using size() or some similar thing
(popup_->size()?  I dunno) ought to work.

This is actually true of all three places in this file that use
GetWindowBoundsInScreen().  I don't know why we're using that at all.

[Bret, 2016/08/18 18:58:04]

I played around with this and I think GetWindowBoundsInScreen is actually
correct. There's no Widget::Size or otherwise simpler getter, and the size_
field from StatusBubbleViews doesn't respond to changes (e.g. when it expands to
fit a large URL). I agree that it's confusing (especially because it raises the
question of what size_ is used for) but untangling it is out of scope for this
patch.

 
+  canvas->Save();
+  float scale = canvas->UndoDeviceScaleFactor();
+  const int radius = std::round(kBubbleCornerRadius * scale);

[Peter Kasting, 2016/08/13 06:13:07]

Seems like we should just use a float, not round.

[Bret, 2016/08/18 18:58:03]

Done.

+
   SkScalar rad[8] = {};
 
   // Top Edges - if the bubble is in its bottom position (sticking downwards),
@@ -386,12 +389,12 @@ void StatusBubbleViews::StatusView::OnPaint(gfx::Canvas* canvas) {
       // The text is RtL or the bubble is on the right side (but not both).
 
       // Top Left corner.
-      rad[0] = kBubbleCornerRadius;
-      rad[1] = kBubbleCornerRadius;
+      rad[0] = radius;
+      rad[1] = radius;
     } else {
       // Top Right corner.
-      rad[2] = kBubbleCornerRadius;
-      rad[3] = kBubbleCornerRadius;
+      rad[2] = radius;
+      rad[3] = radius;
     }
   }
 
@@ -399,54 +402,68 @@ void StatusBubbleViews::StatusView::OnPaint(gfx::Canvas* canvas) {
   // position (sticking upward).
   if (style_ != STYLE_STANDARD && style_ != STYLE_STANDARD_RIGHT) {
     // Bottom Right Corner.
-    rad[4] = kBubbleCornerRadius;
-    rad[5] = kBubbleCornerRadius;
+    rad[4] = radius;
+    rad[5] = radius;
 
     // Bottom Left Corner.
-    rad[6] = kBubbleCornerRadius;
-    rad[7] = kBubbleCornerRadius;
+    rad[6] = radius;
+    rad[7] = radius;
+  }
+
+  const int width = std::round(popup_bounds.width() * scale);
+  const int height = std::round(popup_bounds.height() * scale);

[Peter Kasting, 2016/08/13 06:13:07]

Similarly, I think we should use floats here and RectFs below instead of
rounding.

[Bret, 2016/08/18 18:58:04]

Done.

+
+  if (!browser_view_->HasClientEdge()) {

[Peter Kasting, 2016/08/13 06:13:07]

This function's return value isn't going to change over the life of the status
bubble, so rather than passing in |browser_view_| so it can call this, pass in
this bool directly.

(This sort of pattern of passing in the direct state values where possible is
generally encouraged because it makes designs more testable, since tests don't
have to create an entire fake object to pass in.)

[Bret, 2016/08/18 18:58:04]

Ok, I was thinking the value could change over the life of the status bubble,
due to changing the frame by installing a theme for example. But you seem to be
saying this object would get destroyed in that case, so I changed the plumbing.

In the same spirit I removed ThemeProvider and just passed in the colors
directly.

+    // Clip out the edges when the bubble is docked so that it doesn't draw over
+    // the window border, since there's no client edge. But we do want the
+    // border when the bubble is floating so it looks complete.
+    const int clip_left = style_ == STYLE_STANDARD ? 1 : 0;
+    const bool clip_right = style_ == STYLE_STANDARD_RIGHT ? 1 : 0;
+    const bool clip_bottom = clip_left || clip_right ? 1 : 0;
+    gfx::Rect clip_rect(clip_left, 0, width - clip_right, height - clip_bottom);
+    canvas->ClipRect(clip_rect);
   }
 
-  // Draw the bubble's shadow.
-  int width = popup_bounds.width();
-  int height = popup_bounds.height();
-  gfx::Rect rect(gfx::Rect(popup_bounds.size()));
+  // Draw the shadow that borders the bubble.
+  gfx::Rect bubble_rect(width, height);
+  SkRRect rrect;
+  rrect.setRectRadii(RectToSkRect(bubble_rect), (const SkVector*)rad);

[Peter Kasting, 2016/08/13 06:13:07]

The reason you're having layout issues at higher DPI factors is because you're
painting the "shadow" flush inside the outside edge of the bubble's bounds.  But
you're painting a 1 px shadow, while the outdent of those bounds (set in
Reposition() below) is 1 dip.  A good clue that there are problems here is that
in the function here you use kShadowThickness as a px value, but later in the
file it's used as a dip value.  This means there's a unit mismatch.

Instead, you need to paint the 1 px shadow flush outside the "inside" of this 1
dip outdent.  This is basically what BackgroundWith1PxBorder::Paint() does,
although I don't _think_ you want the floor() call it has in there (but I'm not
totally sure; testing in e.g. 1.5x would probably make it clear).  (Hopefully we
don't get bitten by how the window itself is positioned using DIPs, resulting in
one of those scenarios where at higher scales, the status bubble is offset
versus the main window depending on the screen position of the main window.)

[Bret, 2016/08/18 18:58:04]

I changed the painting to work the same way as BackgroundWith1PxBorder which
makes the roundrect look better, but the layout issues I was referring to is
exactly what you're saying in your parenthetical. I managed to find an easy hack
that makes it look okay at factional dpi. It still hangs off the edge of the
window a little bit, but I won't try to fix that now.

The code ended up a bit duplicated with BackgroundWith1PxBorder but I'm not sure
there's a good way to refactor it.

I kept kShadowThickness even though the shadow thickness is now 1 pixel instead
of 1 dip because there's no good way to actually lay the bubble out in pixels,
so I think it just has to live on as a misnomer for a while. I thought about
changing its name but I can't think of anything else that makes sense.

+
   SkPaint shadow_paint;

[Peter Kasting, 2016/08/13 06:13:07]

Nit: No need to use a different SkPaint here than below.  Just declare one, and
change the color of it in between.

[Bret, 2016/08/18 18:58:04]

Done.

   shadow_paint.setAntiAlias(true);
   shadow_paint.setColor(kShadowColor);
-
-  SkRRect rrect;
-  rrect.setRectRadii(RectToSkRect(rect), (const SkVector*)rad);
   canvas->sk_canvas()->drawRRect(rrect, shadow_paint);
 
-  const int shadow_size = 2 * kShadowThickness;
   // Draw the bubble.
-  rect.SetRect(SkIntToScalar(kShadowThickness), SkIntToScalar(kShadowThickness),
-               SkIntToScalar(width - shadow_size),
-               SkIntToScalar(height - shadow_size));
-  rrect.setRectRadii(RectToSkRect(rect), (const SkVector*)rad);
+  bubble_rect.Inset(kShadowThickness, kShadowThickness);
+  rrect.setRectRadii(RectToSkRect(bubble_rect), (const SkVector*)rad);

[Peter Kasting, 2016/08/13 06:13:07]

This is another place that's incorrect, and should be doing what
BackgroundWith1PxBorder::Paint() does.  Instead of drawing rrects with the same
radii inside each other, we should compute the fill region by subtracting the
stroke's fill path from the fill's.  Drawing equal-radius rects inside each
other like we do here means the outer curvature of the inner rrect won't meet up
exactly with the inner curvature of the outer rrect.  Computing the fill path
based on the stroke path, by contrast, means they'll fit each other exactly. 
(This gets more noticeable as you scale up; running on a 1x screen with
--force-device-scale-factor=4 makes it fairly obvious.)

[Bret, 2016/08/18 18:58:04]

Done. See above comment.

+
+  SkPaint paint;
+  paint.setStyle(SkPaint::kFill_Style);
+  paint.setAntiAlias(true);
+  SkColor toolbar_color =
+      theme_provider_->GetColor(ThemeProperties::COLOR_TOOLBAR);
+  paint.setColor(toolbar_color);
   canvas->sk_canvas()->drawRRect(rrect, paint);
 
+  canvas->Restore();
+
   // Compute text bounds.
-  const gfx::FontList font_list;
-  int text_width =
-      std::min(gfx::GetStringWidth(text_, font_list),
-               width - shadow_size - kTextPositionX - kTextHorizPadding);
-  int text_height = height - shadow_size;
-  gfx::Rect text_bounds(kShadowThickness + kTextPositionX,
-                        kShadowThickness,
-                        std::max(0, text_width),
-                        std::max(0, text_height));
+  gfx::Rect text_rect(popup_bounds.width() - kTextHorizPadding,
+                      popup_bounds.height());
+  text_rect.Inset(kShadowThickness, kShadowThickness);
+  text_rect.Inset(kTextPositionX, 0);
   // Make sure the text is aligned to the right on RTL UIs.
-  text_bounds.set_x(GetMirroredXForRect(text_bounds));
+  text_rect.set_x(GetMirroredXForRect(text_rect));
 
   // Text color is the foreground tab text color at 60% alpha.
   SkColor text_color = color_utils::AlphaBlend(
       theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT), toolbar_color,
       0x99);
+  const gfx::FontList font_list;

[Peter Kasting, 2016/08/13 06:13:07]

Nit: Can we just pass FontList() in the call below?

[Bret, 2016/08/18 18:58:04]

Done.

   canvas->DrawStringRect(
       text_, font_list,
-      color_utils::GetReadableColor(text_color, toolbar_color), text_bounds);
+      color_utils::GetReadableColor(text_color, toolbar_color), text_rect);
 }
 
 
@@ -569,9 +586,11 @@ void StatusBubbleViews::StatusViewExpander::SetBubbleWidth(int width) {
 
 const int StatusBubbleViews::kShadowThickness = 1;
 
-StatusBubbleViews::StatusBubbleViews(views::View* base_view)
+StatusBubbleViews::StatusBubbleViews(const BrowserView* browser_view,
+                                     views::View* base_view)
     : contains_mouse_(false),
       offset_(0),
+      browser_view_(browser_view),
       base_view_(base_view),
       view_(NULL),
       download_shelf_is_visible_(false),
@@ -591,7 +610,8 @@ void StatusBubbleViews::Init() {
     popup_.reset(new views::Widget);
     views::Widget* frame = base_view_->GetWidget();
     if (!view_)
-      view_ = new StatusView(popup_.get(), frame->GetThemeProvider());
+      view_ = new StatusView(popup_.get(), frame->GetThemeProvider(),
+                             browser_view_);
     if (!expand_view_.get())
       expand_view_.reset(new StatusViewExpander(this, view_));
 
@@ -626,8 +646,8 @@ void StatusBubbleViews::Init() {
 }
 
 void StatusBubbleViews::Reposition() {
-  // In restored mode, the client area has a client edge between it and the
-  // frame.
+  // Overlap the client edge that's shown in restored mode, or when there is no
+  // client edge this makes the bubble snug with the corner of the window.
   int overlap = kShadowThickness;
   int height = GetPreferredSize().height();
   int base_view_height = base_view()->bounds().height();