Draw nicer arrows when the scrollbar buttons are not square.

ui/native_theme/native_theme_base.cc

 // 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 "ui/native_theme/native_theme_base.h"
 
 #include <limits>
 #include <memory>
 
 #include "base/command_line.h"
@@ skipping 350 matching lines @@
   SkPaint paint;
   paint.setColor(color);
 
   SkPath path = PathForArrow(rect, direction);
 
   gc->drawPath(path, paint);
 }
 
 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();
   if (direction == kScrollbarUpArrow || direction == kScrollbarDownArrow) {
     int arrow_altitude = bounding_rect.height() / 2 + 1;
     path.moveTo(bounding_rect.x(), bounding_rect.bottom());
     path.rLineTo(bounding_rect.width(), 0);
     path.rLineTo(-bounding_rect.width() / 2.0f, -arrow_altitude);
     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 {
     int arrow_altitude = bounding_rect.width() / 2 + 1;
     path.moveTo(bounding_rect.x(), bounding_rect.y());
     path.rLineTo(0, bounding_rect.height());
     path.rLineTo(arrow_altitude, -bounding_rect.height() / 2.0f);
     path.close();
     path.offset(arrow_altitude / 2, 0);
     if (direction == kScrollbarLeftArrow) {
-      path.offset(-1, 0);
       transform.setScale(-1, 1, center.x(), center.y());
     }
   }
   path.transform(transform);
 
   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,
     const ScrollbarTrackExtraParams& extra_params,
     const gfx::Rect& rect) const {
   SkPaint paint;
   SkIRect skrect;
 
   skrect.set(rect.x(), rect.y(), rect.right(), rect.bottom());
   SkScalar track_hsv[3];
@@ skipping 590 matching lines @@
   SkScalar min_diff = Clamp((hsv1[1] + hsv2[1]) * 1.2f, 0.28f, 0.5f);
   SkScalar diff = Clamp(fabs(hsv1[2] - hsv2[2]) / 2, min_diff, 0.5f);
 
   if (hsv1[2] + hsv2[2] > 1.0)
     diff = -diff;
 
   return SaturateAndBrighten(hsv2, -0.2f, diff);
 }
 
 }  // namespace ui