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..1844cccbb829a19693793b85d680c4b7eef6435c 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,10 @@ class StatusBubbleViews::StatusView : public views::View {
   };
 
   StatusView(views::Widget* popup,
-             const ui::ThemeProvider* theme_provider);
+             const gfx::Size& popup_size_,
+             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
@@ -164,6 +168,8 @@ class StatusBubbleViews::StatusView : public views::View {
   // not.
   void OnAnimationEnded();
 
+  void SetWidth(int new_width);
+
  private:
   class InitialTimer;
 
@@ -193,22 +199,30 @@ 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_;
+  gfx::Size popup_size_;
+
+  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,
+                                          const gfx::Size& popup_size,
+                                          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),
+      popup_size_(popup_size),
+      bubble_color_(bubble_color),
+      bubble_text_color_(bubble_text_color),
+      has_client_edge_(has_client_edge),
       timer_factory_(this) {}
 
 StatusBubbleViews::StatusView::~StatusView() {
@@ -361,19 +375,18 @@ void StatusBubbleViews::StatusView::OnAnimationEnded() {
   }
 }
 
+void StatusBubbleViews::StatusView::SetWidth(int new_width) {
+  popup_size_.set_width(new_width);
+}
+
 const char* StatusBubbleViews::StatusView::GetClassName() const {
   return "StatusBubbleViews::StatusView";
 }
 
 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] = {};
 
@@ -386,12 +399,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 +412,100 @@ 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;
+  }
+
+  // Snap to pixels to avoid shadow blurriness.
+  const float width = std::round(popup_size_.width() * scale);
+  const float height = std::round(popup_size_.height() * scale);

[Peter Kasting, 2016/08/24 22:53:24]

Why are we rounding if we're storing in floats?  If rounding is correct, then
don't we want ints?  And is rounding really what we want?  I'd think we'd want
to floor these, if leaving them as the original values is wrong.

[Bret, 2016/08/25 18:05:38]

The types are just an oversight. I didn't think there was a reason to floor so I
just rounded by default. It doesn't seem to cause any problems. I can floor
instead if you think it's important.

+
+#if defined(OS_WIN)
+  // In Reposition() below we move the bubble 1 dip to the bottom/left so that
+  // the bubble shadow will overlap the client edge/window frame. But we draw
+  // the shadow 1 pixel thick, which means we need to draw at the inner-most
+  // pixel of that dip for a proper overlap.
+  //
+  // Unfortunately the bubble's position relative to the window is dependent on
+  // the bubble's absolute screen position we can't position the bubble
+  // precisely enough to guarantee exactly what we want. Having the bubble "pull

[Peter Kasting, 2016/08/24 22:53:24]

I think you meant to divide this first sentence into two, or add conjunctions or
something.

[Bret, 2016/08/25 18:05:37]

Done.

+  // away" from the frame (leaving a gap where the web content shows through)
+  // looks a lot worse than allowing the bubble to hang off the edge of the
+  // window. Plus the possible positioning imprecision increases at higher dsfs.

[Peter Kasting, 2016/08/24 22:53:24]

Are you saying that sometimes (for example) the bubble is offset to the right,
relative to where we think it should be, by more than 1 px, so if we draw on
what seems like the inner pixel of the client edge, that will be at least 2 px
inside that edge, and there will be at least a 1 px gap?  And that's an argument
for insetting less than we otherwise would?  How much less?  What would you be
doing if this wasn't an issue?

Because it seems like the calculation I think would be correct is _less_ than
the values you're computing below.

I really want to see screenshots of the different positioning errors and how
they look with the code you have here as well as the code I proposed.

[Bret, 2016/08/25 18:05:37]

To your first question: you have the basic idea right. It depends on the
absolute screen coordinates. It's hard for me to tell exactly what the possible
error is, at 1.5x it can be 1-2 pixels, for example. But at 3x it's in the range
of 2-4 pixels. It seems to always error to the left, so that's why I can add a
couple pixels and not have it "pull away" from the frame. I'm guessing it's
converting back and forth from dips somewhere. For everything <3x this at least
keeps it inside the visible bounds of the window.

I don't know what I'd be doing if this wasn't an issue because I can't play with
it. Probably something like you suggested.

I can send you some screenshots.

+  // So these values are very conservative but still look better than not
+  // compensating at all.
+  const int subdip_layout_inset = scale > 1.25 ? (scale > 2 ? 2 : 1) : 0;

[Peter Kasting, 2016/08/24 22:53:24]

It clearly isn't right to just use "2" for all scale factors larger than 2. 
Scale factor 10, for example, is not going to want "2".

I realize that we don't _have_ many larger scale factors, but the point is that
the code is less comprehensible this way than if you use a formula that computes
the right offset based directly on the scale, instead of using a bunch of
conditionals.

[Bret, 2016/08/25 18:05:37]

I'm not claiming this isn't a hack, basically, but I'm not sure what else to do.
You're right that it's probably not correct for very large dsfs but I'm not
going to test everything to see what looks good. This is ok for 1, 1.25, 1.5, 2,
2.25, and 2.5, which are our most common scale factors. And like I said above it
always errors to the left, so if this number is too low then it at least doesn't
look worse than doing nothing.

I could also remove this and come back to fixing the root positioning problem
later. We need to fix this and the omnibox popup anyway.

+#else
+  // FIXME: Since the dip error is dependent on platform-specific code we need

[Peter Kasting, 2016/08/24 22:53:24]

Which platform-specific code?  The DIP-vs.px positioning problems are common to
all views targets.  It doesn't seem like an ifdef here is right.

[Bret, 2016/08/25 18:05:38]

ScreenWin::DIPToScreenRect is what I was thinking of. This function was calling
GetWindowRect before I refactored it too. So I don't trust that the positioning
error is going to work the same way (or even exist) on other platforms.

+  // to figure out appropriate values for non-Windows platforms.
+  const int subdip_layout_inset = 0;
+#endif
+
+  if (!has_client_edge_) {
+    const int clip_size = subdip_layout_inset + 1;
+    // 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 ? clip_size : 0;
+    const int clip_right = style_ == STYLE_STANDARD_RIGHT ? clip_size : 0;
+    const int clip_bottom = clip_left || clip_right ? clip_size : 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()));
-  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);
+  gfx::RectF bubble_rect(width, height);
+  const int error_left =
+      style_ != STYLE_STANDARD_RIGHT ? subdip_layout_inset : 0;
+  const int error_right =
+      style_ == STYLE_STANDARD_RIGHT ? subdip_layout_inset : 0;
+  // The bottom edge is very close to the top of the client edge, so trying to
+  // inset it at all causes it to pull away from the frame.

[Peter Kasting, 2016/08/24 22:53:24]

I don't understand why this is different than the left edge.  Both were
overlapped by the same value in Reposition(), so it seems like both should be
handled the same way.

[Bret, 2016/08/25 18:05:38]

I'm not sure either, tbh. It might have something to do with the shenanigans we
do with the non-client area in the glass frame. But applying the correction to
the bottom edge looks really bad in opaque, so I turned it off.

+  const int error_bottom = has_client_edge_ ? 0 : subdip_layout_inset;
+  bubble_rect.Inset(error_left + 0.5, 0.5, error_right + 0.5,
+                    error_bottom + 0.5);
+
+  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);
+
+  // Get the fill path by subtracting the shadow so they algin neatly.

[Peter Kasting, 2016/08/24 22:53:24]

Nit: align

[Bret, 2016/08/25 18:05:38]

Done.

+  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(kTextPositionX, 0,
+                      popup_size_.width() - kTextHorizPadding,
+                      popup_size_.height());
+  text_rect.Inset(kShadowThickness, kShadowThickness);
   // 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);
 }
 
 
@@ -505,9 +564,6 @@ class StatusBubbleViews::StatusViewExpander : public gfx::LinearAnimation,
                       int current_width,
                       int expansion_end);
 
-  // Set width of fully expanded bubble.
-  void SetExpandedWidth(int expanded_width);
-
  private:
   // Animation functions.
   int GetCurrentBubbleWidth();
@@ -569,13 +625,15 @@ 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),
       base_view_(base_view),
       view_(NULL),
       download_shelf_is_visible_(false),
       is_expanded_(false),
+      has_client_edge_(has_client_edge),
       expand_timer_factory_(this) {
   expand_view_.reset();
 }
@@ -590,8 +648,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(), size_,
+          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 +690,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();
@@ -642,6 +706,7 @@ void StatusBubbleViews::RepositionPopup() {
     // popup window's position is consistent with the base_view_'s window.
     views::View::ConvertPointToScreen(base_view_, &top_left);
 
+    view_->SetWidth(size_.width());
     popup_->SetBounds(gfx::Rect(top_left.x() + position_.x(),
                                 top_left.y() + position_.y(),
                                 size_.width(), size_.height()));
@@ -706,9 +771,8 @@ void StatusBubbleViews::SetURL(const GURL& url) {
   }
 
   // Set Elided Text corresponding to the GURL object.
-  gfx::Rect popup_bounds = popup_->GetWindowBoundsInScreen();
-  int text_width = static_cast<int>(popup_bounds.width() -
-      (kShadowThickness * 2) - kTextPositionX - kTextHorizPadding - 1);
+  int text_width = static_cast<int>(size_.width() - (kShadowThickness * 2) -
+                                    kTextPositionX - kTextHorizPadding - 1);
   url_text_ =
       url_formatter::ElideUrl(url, gfx::FontList(), text_width);
 
@@ -872,7 +936,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,8 +946,7 @@ void StatusBubbleViews::ExpandBubble() {
                             kTextHorizPadding + 1,
                         max_status_bubble_width));
   is_expanded_ = true;
-  expand_view_->StartExpansion(url_text_, popup_bounds.width(),
-                               expanded_bubble_width);
+  expand_view_->StartExpansion(url_text_, size_.width(), expanded_bubble_width);
 }
 
 int StatusBubbleViews::GetStandardStatusBubbleWidth() {