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..02040ca284f12bad2b39c2b10e86cebe7f4f0281 100644
--- a/chrome/browser/ui/views/status_bubble_views.cc
+++ b/chrome/browser/ui/views/status_bubble_views.cc
@@ -19,7 +19,8 @@
 #include "components/url_formatter/elide_url.h"
 #include "components/url_formatter/url_formatter.h"
 #include "third_party/skia/include/core/SkPaint.h"
-#include "third_party/skia/include/core/SkRRect.h"
+#include "third_party/skia/include/core/SkPath.h"
+#include "third_party/skia/include/pathops/SkPathOps.h"
 #include "ui/base/theme_provider.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ -135,7 +136,9 @@ class StatusBubbleViews::StatusView : public views::View {
   };
 
   StatusView(views::Widget* popup,
-             const ui::ThemeProvider* theme_provider);
+             SkColor bubble_color,
+             SkColor bubble_text_color,
+             bool has_client_edge);
   ~StatusView() override;
 
   // Set the bubble text to a certain value, hides the bubble if text is
@@ -193,22 +196,26 @@ class StatusBubbleViews::StatusView : public views::View {
   // The currently-displayed text.
   base::string16 text_;
 
-  // Holds the theme provider of the frame that created us.
-  const ui::ThemeProvider* theme_provider_;
+  const SkColor bubble_color_;
+  const SkColor bubble_text_color_;
+  const bool has_client_edge_;
 
   base::WeakPtrFactory<StatusBubbleViews::StatusView> timer_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(StatusView);
 };
 
-StatusBubbleViews::StatusView::StatusView(
-    views::Widget* popup,
-    const ui::ThemeProvider* theme_provider)
+StatusBubbleViews::StatusView::StatusView(views::Widget* popup,
+                                          SkColor bubble_color,
+                                          SkColor bubble_text_color,
+                                          bool has_client_edge)
     : state_(BUBBLE_HIDDEN),
       style_(STYLE_STANDARD),
       animation_(new StatusViewAnimation(this, 0, 0)),
       popup_(popup),
-      theme_provider_(theme_provider),
+      bubble_color_(bubble_color),
+      bubble_text_color_(bubble_text_color),
+      has_client_edge_(has_client_edge),
       timer_factory_(this) {}
 
 StatusBubbleViews::StatusView::~StatusView() {
@@ -366,15 +373,12 @@ 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();
 
+  canvas->Save();
+  float scale = canvas->UndoDeviceScaleFactor();
+  const float radius = kBubbleCornerRadius * scale;
+
   SkScalar rad[8] = {};
 
   // Top Edges - if the bubble is in its bottom position (sticking downwards),
@@ -386,12 +390,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 +403,73 @@ 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;
   }
 
-  // Draw the bubble's shadow.
-  int width = popup_bounds.width();
-  int height = popup_bounds.height();
-  gfx::Rect rect(gfx::Rect(popup_bounds.size()));
-  SkPaint shadow_paint;
-  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);
-  canvas->sk_canvas()->drawRRect(rrect, paint);
+  const float width = popup_bounds.width() * scale;
+  const float height = popup_bounds.height() * scale;
+
+  if (!has_client_edge_) {
+    // 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);
+  }
+
+  // At non-integral device scale factors the size we get from
+  // GetWindowBoundsInScreen is slightly too high and the edge of our rectangle
+  // will get clipped off. The error never seems to be more than 3 pixels.
+  const int scale_error_adjustment = scale - std::floor(scale) > 0 ? 3 : 0;

[Peter Kasting, 2016/08/20 00:42:42]

Wat.

3 px seems entirely magic.  I would expect that number to be governed by the
scale factor you're at, not clamped to 3.

And what is actually clipping you here?  Why is this not a problem for the old
code as well?

[Bret, 2016/08/24 22:04:47]

It is a problem for the old code, I just got a bit frustrated with it before.

I investigated this more thoroughly and part of the problem was that we weren't
snapping the width and height to pixels, so sometimes the border line would be
aliased and it looks similar to clipping (I fixed that). But the real problem
was that GetWindowBoundsInScreen is using ScreenWin::DIPToScreenRect which uses
ScaleToEnclosingRect to coerce to integers, so it's going to be an overestimate
on the width most of the time. That means if we draw to that width we'll go
outside the layer bounds and get clipped. So I changed the plumbing to just use
the original size_ in this file (it wasn't as problematic as I thought earlier).

+  gfx::RectF bubble_rect(width - scale_error_adjustment, height);
+  bubble_rect.Inset(0.5, 0.5);

[Peter Kasting, 2016/08/20 00:42:42]

You're still overlapping by 1 DIP in Reposition() and then painting in the
outermost px here.  This is still going to result in incorrect positioning at
DSF > 1.  You need to point in the innermost px of that outer DIP, not the
outermost.

Something akin to:

  // The left and bottom edges are outdented by kShadowThickness DIPs in
  // Reposition().  We want to paint on the inner edge of that outdent.  We
floor
  // the scaled outdent to snap the border coordinates to the pixel grid.
  float aligned_scale = std::floor(kShadowThickness * scale);
  bubble_rect.Inset(aligned_scale - 0.5, 0.5, 0.5, aligned_scale - 0.5);

[Bret, 2016/08/24 22:04:47]

Ok I misunderstood what you meant before. The relative position is annoyingly
dependent on the absolute position so it's not really possible to be pixel
perfect without fixing that. I added a big comment, hopefully it's clear enough.

+
+  SkPath path;
+  path.addRoundRect(gfx::RectFToSkRect(bubble_rect), rad);
+
+  SkPaint paint;
+  paint.setStyle(SkPaint::kStroke_Style);
+  paint.setStrokeWidth(1);
+  paint.setAntiAlias(true);
+
+  SkPath stroke_path;
+  paint.getFillPath(path, &stroke_path);
+
+  SkPath fill_path;
+  Op(path, stroke_path, kDifference_SkPathOp, &fill_path);
+  paint.setStyle(SkPaint::kFill_Style);
+  paint.setColor(bubble_color_);
+  canvas->sk_canvas()->drawPath(fill_path, paint);
+
+  paint.setColor(kShadowColor);
+  canvas->sk_canvas()->drawPath(stroke_path, 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 -
+                          std::ceil(scale_error_adjustment / scale),
+                      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);
+  SkColor blended_text_color =
+      color_utils::AlphaBlend(bubble_text_color_, bubble_color_, 0x99);
   canvas->DrawStringRect(
-      text_, font_list,
-      color_utils::GetReadableColor(text_color, toolbar_color), text_bounds);
+      text_, gfx::FontList(),
+      color_utils::GetReadableColor(blended_text_color, bubble_color_),
+      text_rect);
 }
 
 
@@ -569,9 +592,11 @@ void StatusBubbleViews::StatusViewExpander::SetBubbleWidth(int width) {
 
 const int StatusBubbleViews::kShadowThickness = 1;
 
-StatusBubbleViews::StatusBubbleViews(views::View* base_view)
+StatusBubbleViews::StatusBubbleViews(views::View* base_view,
+                                     bool has_client_edge)
     : contains_mouse_(false),
       offset_(0),
+      has_client_edge_(has_client_edge),
       base_view_(base_view),
       view_(NULL),
       download_shelf_is_visible_(false),
@@ -590,8 +615,14 @@ void StatusBubbleViews::Init() {
   if (!popup_.get()) {
     popup_.reset(new views::Widget);
     views::Widget* frame = base_view_->GetWidget();
-    if (!view_)
-      view_ = new StatusView(popup_.get(), frame->GetThemeProvider());
+    if (!view_) {
+      const ui::ThemeProvider* theme_provider = frame->GetThemeProvider();
+      view_ = new StatusView(
+          popup_.get(),
+          theme_provider->GetColor(ThemeProperties::COLOR_TOOLBAR),
+          theme_provider->GetColor(ThemeProperties::COLOR_TAB_TEXT),
+          has_client_edge_);
+    }
     if (!expand_view_.get())
       expand_view_.reset(new StatusViewExpander(this, view_));
 
@@ -626,8 +657,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();
@@ -872,7 +903,6 @@ bool StatusBubbleViews::IsFrameMaximized() {
 void StatusBubbleViews::ExpandBubble() {
   // Elide URL to maximum possible size, then check actual length (it may
   // still be too long to fit) before expanding bubble.
-  gfx::Rect popup_bounds = popup_->GetWindowBoundsInScreen();
   int max_status_bubble_width = GetMaxStatusBubbleWidth();
   const gfx::FontList font_list;
   url_text_ = url_formatter::ElideUrl(url_, font_list, max_status_bubble_width);
@@ -883,6 +913,7 @@ void StatusBubbleViews::ExpandBubble() {
                             kTextHorizPadding + 1,
                         max_status_bubble_width));
   is_expanded_ = true;
+  gfx::Rect popup_bounds = popup_->GetWindowBoundsInScreen();
   expand_view_->StartExpansion(url_text_, popup_bounds.width(),
                                expanded_bubble_width);
 }