Fix scrollbar buttons at hidpi when enable-use-zoom-for-dsf is on.

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 cc94eafb7b432d0af3abfcb0c9dcabb5b9cde16b..abd7edbe82b0ae9d904736b29df23dfd9be6c192 100644
--- a/ui/native_theme/native_theme_base.cc
+++ b/ui/native_theme/native_theme_base.cc
@@ -358,43 +358,49 @@ void NativeThemeBase::PaintArrow(SkCanvas* gc,
                                  const gfx::Rect& rect,
                                  Part direction,
                                  SkColor color) const {
-  int width_middle, length_middle;
-  if (direction == kScrollbarUpArrow || direction == kScrollbarDownArrow) {
-    width_middle = rect.width() / 2 + 1;
-    length_middle = rect.height() / 2 + 1;
-  } else {
-    length_middle = rect.width() / 2 + 1;
-    width_middle = rect.height() / 2 + 1;
-  }
+  int width_middle = rect.width() / 2 + 1;
+  int length_middle = rect.height() / 2 + 1;

[Evan Stade, 2016/04/25 02:35:59]

confusing var name -- why not height_middle?

[Bret, 2016/04/25 23:24:32]

No reason, it was named that before I messed with it. Renamed.

 
   SkPaint paint;
   paint.setColor(color);
   paint.setAntiAlias(false);
   paint.setStyle(SkPaint::kFill_Style);
 
+  int arrow_altitude;
+  if (direction == kScrollbarUpArrow || direction == kScrollbarDownArrow) {
+    arrow_altitude = length_middle / 2;
+  } else {

[Evan Stade, 2016/04/25 02:35:59]

nit: no curlies

also ternary assignment is probably preferable

[Bret, 2016/04/25 23:24:32]

Replaced with ternary.

+    arrow_altitude = width_middle / 2;
+  }
+  int arrow_base_length = arrow_altitude * 2 - 1;

[Evan Stade, 2016/04/25 02:35:59]

seems a bit silly to divide by 2 then multiply by 2. How about

  int lengthwise_middle = direction == kScrollbarUpArrow || direction ==
kScrollbarDownArrow ? height_middle : width_middle;
  ...

and I think translating the whole thing by 0.5, 0.5 will probably make a lot of
the magic constants go away

[Bret, 2016/04/25 23:24:32]

I guess it does look silly, but those are the values we want. Empirically
width_middle and height_middle are always odd, so middle/2*2 != middle, but I
don't want to assume that it is so necessarily. Since we want to save middle/2
for the arrow altitude anyway, this seems nicer than "arrow_base_length =
(middle % 2 == 0 ? middle : middle - 1) - 1"

I added a comment for clarity.

[Evan Stade, 2016/04/25 23:38:16]

oops, this last bit of the comment was supposed to be attached to the comment on
the switch.

+
   SkPath path;
-  // The constants in this block of code are hand-tailored to produce good
+  // The calculations in this block of code are hand-tailored to produce good
   // looking arrows without anti-aliasing.
   switch (direction) {

[Evan Stade, 2016/04/25 02:35:59]

what if you just calculated the path once (for an up arrow, for example) then
applied a rotation transform as needed?

[Bret, 2016/04/25 23:24:32]

Didn't work. The arrows still need to be centered differently, and the left
arrow is finicky for some reason, so the code was starting to get weirder than
the original block of special cases.

     case kScrollbarUpArrow:
-      path.moveTo(rect.x() + width_middle - 4, rect.y() + length_middle + 2);
-      path.rLineTo(7, 0);
-      path.rLineTo(-4, -4);
+      path.moveTo(rect.x() + width_middle - arrow_altitude,
+                  rect.y() + length_middle + (arrow_altitude / 2));

[Evan Stade, 2016/04/25 02:35:59]

the asymmetry here is weird. Why are you doing integer division sometimes and
float division sometimes?

[Bret, 2016/04/25 23:24:32]

I was making sure everything looked the same at 1x and looked fine at >1x. Doing
it this way happened to work out, but I'll change the integer divisions to
floored float divisions to avoid confusion.

+      path.rLineTo(arrow_base_length, 0);
+      path.rLineTo(-arrow_altitude, -arrow_altitude);
       break;
     case kScrollbarDownArrow:
-      path.moveTo(rect.x() + width_middle - 4, rect.y() + length_middle - 3);
-      path.rLineTo(7, 0);
-      path.rLineTo(-4, 4);
+      path.moveTo(rect.x() + width_middle - arrow_altitude,
+                  rect.y() + length_middle - ceil(arrow_altitude / 2.0));
+      path.rLineTo(arrow_base_length, 0);
+      path.rLineTo(-arrow_altitude, arrow_altitude);
       break;
     case kScrollbarRightArrow:
-      path.moveTo(rect.x() + length_middle - 3, rect.y() + width_middle - 4);
-      path.rLineTo(0, 7);
-      path.rLineTo(4, -4);
+      path.moveTo(rect.x() + width_middle - (arrow_altitude / 2),
+                  rect.y() + length_middle - arrow_altitude);
+      path.rLineTo(0, arrow_base_length);
+      path.rLineTo(arrow_altitude, -arrow_altitude);
       break;
     case kScrollbarLeftArrow:
-      path.moveTo(rect.x() + length_middle + 1, rect.y() + width_middle - 5);
-      path.rLineTo(0, 9);
-      path.rLineTo(-4, -4);
+      path.moveTo(rect.x() + width_middle + 1,
+                  rect.y() + length_middle - (arrow_altitude + 1));
+      path.rLineTo(0, arrow_base_length + 2);

[Evan Stade, 2016/04/25 02:35:59]

can you leave comments explaining the magic constants (+1, +2, etc.)

[Bret, 2016/04/25 23:24:32]

To be honest, I'm not really sure why these constants are needed. It looks right
with them and not without them. I added a comment to that effect.

To address your comment above, translating by 0.5, 0.5 might work, but since
left is the only weird one and this is essentially what the old code was doing,
I'd rather leave it alone.

[Evan Stade, 2016/04/25 23:38:16]

The new code is even kookier than the old, and I still have hopes we could do
something really simple with .5/.5 and rotation transforms

+      path.rLineTo(-arrow_altitude, -arrow_altitude);
       break;
     default:
       break;