Draw nicer arrows when the scrollbar buttons are not square.

ui/native_theme/native_theme_base.cc

Index: ui/native_theme/native_theme_base.cc
diff --git a/ui/native_theme/native_theme_base.cc b/ui/native_theme/native_theme_base.cc
index 548b4a5cf695869c665d8eee425948b31e0d60af..11f19b5713a5aa09e74e25621cfd01f7100f8401 100644
--- a/ui/native_theme/native_theme_base.cc
+++ b/ui/native_theme/native_theme_base.cc
@@ -368,12 +368,8 @@ void NativeThemeBase::PaintArrow(SkCanvas* gc,
 
 SkPath NativeThemeBase::PathForArrow(const gfx::Rect& rect,
                                      Part direction) const {
-  gfx::Rect bounding_rect(rect);
-  const int padding_width = ceil(rect.width() / 4.f);
-  const int padding_height = ceil(rect.height() / 4.f);
-  bounding_rect.Inset(padding_width, padding_height);
-  const gfx::Point center = bounding_rect.CenterPoint();
-
+  gfx::RectF bounding_rect = BoundingRectForArrow(rect);
+  const gfx::PointF center = bounding_rect.CenterPoint();
   SkPath path;
   SkMatrix transform;
   transform.setIdentity();
@@ -385,7 +381,6 @@ SkPath NativeThemeBase::PathForArrow(const gfx::Rect& rect,
     path.close();
     path.offset(0, -arrow_altitude / 2 + 1);
     if (direction == kScrollbarDownArrow) {
-      path.offset(0, -1);
       transform.setScale(1, -1, center.x(), center.y());
     }
   } else {
@@ -396,7 +391,6 @@ SkPath NativeThemeBase::PathForArrow(const gfx::Rect& rect,
     path.close();
     path.offset(arrow_altitude / 2, 0);
     if (direction == kScrollbarLeftArrow) {
-      path.offset(-1, 0);
       transform.setScale(-1, 1, center.x(), center.y());
     }
   }
@@ -405,6 +399,24 @@ SkPath NativeThemeBase::PathForArrow(const gfx::Rect& rect,
   return path;
 }
 
+gfx::RectF NativeThemeBase::BoundingRectForArrow(const gfx::Rect& rect) const {
+  const std::pair<int, int> rect_sides =
+      std::minmax(rect.width(), rect.height());
+  const int side_length_inset = 2 * std::ceil(rect_sides.second / 4.f);
+  const int side_length =
+      std::min(rect_sides.first, rect_sides.second - side_length_inset);
+  gfx::RectF bounding_rect(rect.x() + (rect.width() - side_length) / 2.f,
+                           rect.y() + (rect.height() - side_length) / 2.f,
+                           side_length, side_length);
+
+  // Snap bounding_rect to whole pixels.
+  bounding_rect.set_x(ceil(bounding_rect.x()));
+  bounding_rect.set_y(ceil(bounding_rect.y()));

[Peter Kasting, 2016/06/11 02:19:27]

After thinking about this for a while, I think the ceil() calls here are
equivalent to round() calls.  This is because the X and Y coordinates are either
on .0 or .5.  Right?

Also, it seems odd to return a RectF here when we want an int-snapped rect, just
because the caller wants to call CenterPoint (*).  That's sort of a caller-side
detail.

I might do this:

  ...
  const int side_length = ...;
  // When there are an odd number of pixels, put the extra on the top/left.
  return gfx::Rect(rect.x() + (rect.width() - side_length + 1) / 2,
                   rect.y() + (rect.height() - side_length + 1) / 2,
                   side_length, side_length);
}

This is not only shorter, the integer division is a lot clearer to me about what
kind of rounding can practically end up occurring.  Maybe that's just me.

Then on the caller side:

  const gfx::PointF center = gfx::RectF(bounding_rect).CenterPoint();

(*) One other complication is the "rLineTo(... / 2.0f)" calls.  It seems like
the right thing ought to be doable with ints here, but I'm not sure.

[Bret, 2016/06/13 18:02:51]

Ok, I like this suggestion.

+  // Width and height are always aligned to whole pixels.
+
+  return bounding_rect;
+}
+
 void NativeThemeBase::PaintScrollbarTrack(SkCanvas* canvas,
     Part part,
     State state,