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

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 340 matching lines @@
   paint.setColor(OutlineColor(trackHSV, thumbHSV));
   canvas->drawPath(outline, paint);
 
   PaintArrow(canvas, rect, direction, GetArrowColor(state));
 }
 
 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;
   }
   path.close();
 
   gc->drawPath(path, paint);
 }
 
 void NativeThemeBase::PaintScrollbarTrack(SkCanvas* canvas,
@@ skipping 617 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