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

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

Previously on Windows the scrollbar buttons were always sized at 1x
and would leave ugly black space between them at the edge of the
window.

BUG=612874
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x
device scales with the #enable-use-zoom-for-dsf flag on.

Committed: https://crrev.com/6e7c308532450f3a8fedef962ac3d3e14bb80880
Cr-Commit-Position: refs/heads/master@{#394893}

[Bret, 2016-04-22 17:43:09]

Description was changed from

==========
Fix scrollbar buttons at hidpi when enable-use-zoom-for-dsf is on.
Previously on Windows and Linux the scrollbar buttons were always sized
at 1x and would leave ugly black space between them at the edge of the
window.

BUG=485650
TEST=Scrollbar buttons on Windows/Linux should look right at 1.5x and 2x device
scales with the #enable-use-zoom-for-dsf flag on.
==========

to

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

Previously on Windows the scrollbar buttons were always sized at 1x and would
leave ugly black space between them at the edge of the window.

BUG=485650
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x device scales
with the #enable-use-zoom-for-dsf flag on.
==========

[Bret, 2016-04-22 17:43:09]

bsep@chromium.org changed reviewers:
+ esprehn@chromium.org, estade@chromium.org

[esprehn, 2016-04-23 02:08:13]

Tests?

[Evan Stade, 2016-04-25 02:36:00]

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:362: int length_middle = rect.height() / 2
+ 1;
confusing var name -- why not height_middle?

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:372: } else {
nit: no curlies

also ternary assignment is probably preferable

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:375: int arrow_base_length = arrow_altitude
* 2 - 1;
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

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:380: switch (direction) {
what if you just calculated the path once (for an up arrow, for example) then
applied a rotation transform as needed?

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:383: rect.y() + length_middle +
(arrow_altitude / 2));
the asymmetry here is weird. Why are you doing integer division sometimes and
float division sometimes?

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:402: path.rLineTo(0, arrow_base_length +
2);
can you leave comments explaining the magic constants (+1, +2, etc.)

[esprehn, 2016-04-25 23:12:11]

https://codereview.chromium.org/1911973002/diff/1/content/child/webthemeengin...
File content/child/webthemeengine_impl_default.cc (left):

https://codereview.chromium.org/1911973002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.cc:24: int32_t
g_vertical_scroll_bar_width = 17;
what sets these values now?

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

I haven't forgotten about tests but I wanted to reply to the other comments.

https://codereview.chromium.org/1911973002/diff/1/content/child/webthemeengin...
File content/child/webthemeengine_impl_default.cc (left):

https://codereview.chromium.org/1911973002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.cc:24: int32_t
g_vertical_scroll_bar_width = 17;
On 2016/04/25 23:12:11, esprehn wrote:
> what sets these values now?

Nothing, they were only used in the vestigial pre-hidpi case that I fixed in
ScrollbarThemeAura.

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:362: int length_middle = rect.height() / 2
+ 1;
On 2016/04/25 02:35:59, Evan Stade wrote:
> confusing var name -- why not height_middle?

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

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:372: } else {
On 2016/04/25 02:35:59, Evan Stade wrote:
> nit: no curlies
> 
> also ternary assignment is probably preferable

Replaced with ternary.

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:375: int arrow_base_length = arrow_altitude
* 2 - 1;
On 2016/04/25 02:35:59, Evan Stade wrote:
> 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

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.

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:380: switch (direction) {
On 2016/04/25 02:35:59, Evan Stade wrote:
> what if you just calculated the path once (for an up arrow, for example) then
> applied a rotation transform as needed?

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.

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:383: rect.y() + length_middle +
(arrow_altitude / 2));
On 2016/04/25 02:35:59, Evan Stade wrote:
> the asymmetry here is weird. Why are you doing integer division sometimes and
> float division sometimes?

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.

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:402: path.rLineTo(0, arrow_base_length +
2);
On 2016/04/25 02:35:59, Evan Stade wrote:
> can you leave comments explaining the magic constants (+1, +2, etc.)

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:17]

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:375: int arrow_base_length = arrow_altitude
* 2 - 1;
> and I think translating the whole thing by 0.5, 0.5 will probably make a lot
of
> the magic constants go away

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

https://codereview.chromium.org/1911973002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_base.cc:402: path.rLineTo(0, arrow_base_length +
2);
On 2016/04/25 23:24:32, Bret Sepulveda wrote:
> On 2016/04/25 02:35:59, Evan Stade wrote:
> > can you leave comments explaining the magic constants (+1, +2, etc.)
> 
> 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.

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

[Bret, 2016-04-29 00:52:17]

Okay I completely rewrote the arrow drawing to just poke pixels into an arrow
shape instead of bothering with paths, since I wasn't getting anywhere with
transformations. I think it ended up less unpleasant.

Also added tests.

[Evan Stade, 2016-04-29 16:27:44]

On 2016/04/29 00:52:17, Bret Sepulveda wrote:
> Okay I completely rewrote the arrow drawing to just poke pixels into an arrow
> shape instead of bothering with paths, since I wasn't getting anywhere with
> transformations. I think it ended up less unpleasant.
> 
> Also added tests.

here's what I get when I force device scale=3 with this patch:

https://screenshot.googleplex.com/siwxLjryPEh.png

[Bret, 2016-04-29 21:15:48]

On 2016/04/29 16:27:44, Evan Stade wrote:
> On 2016/04/29 00:52:17, Bret Sepulveda wrote:
> > Okay I completely rewrote the arrow drawing to just poke pixels into an
arrow
> > shape instead of bothering with paths, since I wasn't getting anywhere with
> > transformations. I think it ended up less unpleasant.
> > 
> > Also added tests.
> 
> here's what I get when I force device scale=3 with this patch:
> 
> https://screenshot.googleplex.com/siwxLjryPEh.png

Okay that's pretty odd... that only happens with the enable-use-zoom-for-dsf
flag off. I guess I assumed that skia was scaling up the fully drawn scrollbar
instead of recording the draw commands and then replaying them with scaled
coordinates.

In that case I can't draw points with the flag off. So there's 3 options:
1. Branch on the flag and do the old thing when it's off. I don't like having
vestigial code around, but at least it'll go away eventually.
2. Draw 1 pixel squares instead of points. It'll look blocky at higher dsfs with
the flag off, so I don't like that one either.
3. Go back to the wonky path code in had in patch set 2. Seems like the best
compromise, but drawing pixels is what we actually want in the end.

What do you think?

[Evan Stade, 2016-04-29 22:33:17]

On 2016/04/29 21:15:48, Bret Sepulveda wrote:
> On 2016/04/29 16:27:44, Evan Stade wrote:
> > On 2016/04/29 00:52:17, Bret Sepulveda wrote:
> > > Okay I completely rewrote the arrow drawing to just poke pixels into an
> arrow
> > > shape instead of bothering with paths, since I wasn't getting anywhere
with
> > > transformations. I think it ended up less unpleasant.
> > > 
> > > Also added tests.
> > 
> > here's what I get when I force device scale=3 with this patch:
> > 
> > https://screenshot.googleplex.com/siwxLjryPEh.png
> 
> Okay that's pretty odd... that only happens with the enable-use-zoom-for-dsf
> flag off. I guess I assumed that skia was scaling up the fully drawn scrollbar
> instead of recording the draw commands and then replaying them with scaled
> coordinates.
> 
> In that case I can't draw points with the flag off. So there's 3 options:
> 1. Branch on the flag and do the old thing when it's off. I don't like having
> vestigial code around, but at least it'll go away eventually.
> 2. Draw 1 pixel squares instead of points. It'll look blocky at higher dsfs
with
> the flag off, so I don't like that one either.
> 3. Go back to the wonky path code in had in patch set 2. Seems like the best
> compromise, but drawing pixels is what we actually want in the end.
> 
> What do you think?

I'm still not convinced that sane paths + half pixel offsets are not going to
work.

[Bret, 2016-04-30 00:44:24]

On 2016/04/29 22:33:17, Evan Stade wrote:
> On 2016/04/29 21:15:48, Bret Sepulveda wrote:
> > On 2016/04/29 16:27:44, Evan Stade wrote:
> > > On 2016/04/29 00:52:17, Bret Sepulveda wrote:
> > > > Okay I completely rewrote the arrow drawing to just poke pixels into an
> > arrow
> > > > shape instead of bothering with paths, since I wasn't getting anywhere
> with
> > > > transformations. I think it ended up less unpleasant.
> > > > 
> > > > Also added tests.
> > > 
> > > here's what I get when I force device scale=3 with this patch:
> > > 
> > > https://screenshot.googleplex.com/siwxLjryPEh.png
> > 
> > Okay that's pretty odd... that only happens with the enable-use-zoom-for-dsf
> > flag off. I guess I assumed that skia was scaling up the fully drawn
scrollbar
> > instead of recording the draw commands and then replaying them with scaled
> > coordinates.
> > 
> > In that case I can't draw points with the flag off. So there's 3 options:
> > 1. Branch on the flag and do the old thing when it's off. I don't like
having
> > vestigial code around, but at least it'll go away eventually.
> > 2. Draw 1 pixel squares instead of points. It'll look blocky at higher dsfs
> with
> > the flag off, so I don't like that one either.
> > 3. Go back to the wonky path code in had in patch set 2. Seems like the best
> > compromise, but drawing pixels is what we actually want in the end.
> > 
> > What do you think?
> 
> I'm still not convinced that sane paths + half pixel offsets are not going to
> work.

Here's some info on what's going on when I try transformations and bumping
things by half pixels:
https://docs.google.com/a/google.com/document/d/1CFg4rguAhI97ZrlDOvls4Ul3TaT9...

[Evan Stade, 2016-05-04 03:43:09]

On 2016/04/30 00:44:24, Bret Sepulveda wrote:
> On 2016/04/29 22:33:17, Evan Stade wrote:
> > On 2016/04/29 21:15:48, Bret Sepulveda wrote:
> > > On 2016/04/29 16:27:44, Evan Stade wrote:
> > > > On 2016/04/29 00:52:17, Bret Sepulveda wrote:
> > > > > Okay I completely rewrote the arrow drawing to just poke pixels into
an
> > > arrow
> > > > > shape instead of bothering with paths, since I wasn't getting anywhere
> > with
> > > > > transformations. I think it ended up less unpleasant.
> > > > > 
> > > > > Also added tests.
> > > > 
> > > > here's what I get when I force device scale=3 with this patch:
> > > > 
> > > > https://screenshot.googleplex.com/siwxLjryPEh.png
> > > 
> > > Okay that's pretty odd... that only happens with the
enable-use-zoom-for-dsf
> > > flag off. I guess I assumed that skia was scaling up the fully drawn
> scrollbar
> > > instead of recording the draw commands and then replaying them with scaled
> > > coordinates.
> > > 
> > > In that case I can't draw points with the flag off. So there's 3 options:
> > > 1. Branch on the flag and do the old thing when it's off. I don't like
> having
> > > vestigial code around, but at least it'll go away eventually.
> > > 2. Draw 1 pixel squares instead of points. It'll look blocky at higher
dsfs
> > with
> > > the flag off, so I don't like that one either.
> > > 3. Go back to the wonky path code in had in patch set 2. Seems like the
best
> > > compromise, but drawing pixels is what we actually want in the end.
> > > 
> > > What do you think?
> > 
> > I'm still not convinced that sane paths + half pixel offsets are not going
to
> > work.
> 
> Here's some info on what's going on when I try transformations and bumping
> things by half pixels:
>
https://docs.google.com/a/google.com/document/d/1CFg4rguAhI97ZrlDOvls4Ul3TaT9...

great doc, thanks. I think part of your problem is the style --- you have fill
instead of stroke and fill. This might be leading to weirdness. I monkeyed
around and this seems to work properly for me, modulo exact centering (which
should be easy to fix). By "works properly" I mean it looks the same pointing
right and pointing left (with and without the transform part).

void NativeThemeBase::PaintArrow(SkCanvas* gc,
                                 const gfx::Rect& rect,
                                 Part direction,
                                 SkColor color) const {

  SkPaint paint;
  paint.setColor(color);
  paint.setAntiAlias(false);
  paint.setStyle(SkPaint::kStrokeAndFill_Style);

  gfx::Rect bounding_rect(rect);
  int padding = rect.width() / 4;
  bounding_rect.Inset(gfx::Insets(padding));

  // start off drawing a right-facing arrow
  SkPath path;
  path.moveTo(bounding_rect.x(), bounding_rect.y());
  path.rLineTo(0, bounding_rect.height());
  path.rLineTo(bounding_rect.width() / 2.f, -bounding_rect.height() / 2.f);
  path.close();
  path.offset(0.5, 0.5);

  // to the left, to the left
  SkMatrix transform;
  gfx::Point center = bounding_rect.CenterPoint();
  transform.setRotate(180, center.x(), center.y());
  path.transform(transform);

  gc->drawPath(path, paint);
}

[esprehn, 2016-05-04 03:56:06]

esprehn@chromium.org changed reviewers:
+ ericrk@chromium.org, jbroman@chromium.org

[esprehn, 2016-05-04 03:56:06]

jbroman@ and ericrk@ can you help them come up with something here that's fast
and simple? I worry doing something too fancy is just going to be slow and
scrollbar painting needs to be fast. We really need to get the Views drawing
faster too, every time the native UI rasters (even for small things like typing
a single letter in the omnibox) I see giant paint slices in the browser process.

[Bret, 2016-05-05 22:44:36]

On 2016/05/04 03:43:09, Evan Stade wrote:
> On 2016/04/30 00:44:24, Bret Sepulveda wrote:
> > On 2016/04/29 22:33:17, Evan Stade wrote:
> > > On 2016/04/29 21:15:48, Bret Sepulveda wrote:
> > > > On 2016/04/29 16:27:44, Evan Stade wrote:
> > > > > On 2016/04/29 00:52:17, Bret Sepulveda wrote:
> > > > > > Okay I completely rewrote the arrow drawing to just poke pixels into
> an
> > > > arrow
> > > > > > shape instead of bothering with paths, since I wasn't getting
anywhere
> > > with
> > > > > > transformations. I think it ended up less unpleasant.
> > > > > > 
> > > > > > Also added tests.
> > > > > 
> > > > > here's what I get when I force device scale=3 with this patch:
> > > > > 
> > > > > https://screenshot.googleplex.com/siwxLjryPEh.png
> > > > 
> > > > Okay that's pretty odd... that only happens with the
> enable-use-zoom-for-dsf
> > > > flag off. I guess I assumed that skia was scaling up the fully drawn
> > scrollbar
> > > > instead of recording the draw commands and then replaying them with
scaled
> > > > coordinates.
> > > > 
> > > > In that case I can't draw points with the flag off. So there's 3
options:
> > > > 1. Branch on the flag and do the old thing when it's off. I don't like
> > having
> > > > vestigial code around, but at least it'll go away eventually.
> > > > 2. Draw 1 pixel squares instead of points. It'll look blocky at higher
> dsfs
> > > with
> > > > the flag off, so I don't like that one either.
> > > > 3. Go back to the wonky path code in had in patch set 2. Seems like the
> best
> > > > compromise, but drawing pixels is what we actually want in the end.
> > > > 
> > > > What do you think?
> > > 
> > > I'm still not convinced that sane paths + half pixel offsets are not going
> to
> > > work.
> > 
> > Here's some info on what's going on when I try transformations and bumping
> > things by half pixels:
> >
>
https://docs.google.com/a/google.com/document/d/1CFg4rguAhI97ZrlDOvls4Ul3TaT9...
> 
> great doc, thanks. I think part of your problem is the style --- you have fill
> instead of stroke and fill. This might be leading to weirdness. I monkeyed
> around and this seems to work properly for me, modulo exact centering (which
> should be easy to fix). By "works properly" I mean it looks the same pointing
> right and pointing left (with and without the transform part).
> 
> void NativeThemeBase::PaintArrow(SkCanvas* gc,
>                                  const gfx::Rect& rect,
>                                  Part direction,
>                                  SkColor color) const {
> 
>   SkPaint paint;
>   paint.setColor(color);
>   paint.setAntiAlias(false);
>   paint.setStyle(SkPaint::kStrokeAndFill_Style);
> 
>   gfx::Rect bounding_rect(rect);
>   int padding = rect.width() / 4;
>   bounding_rect.Inset(gfx::Insets(padding));
> 
>   // start off drawing a right-facing arrow
>   SkPath path;
>   path.moveTo(bounding_rect.x(), bounding_rect.y());
>   path.rLineTo(0, bounding_rect.height());
>   path.rLineTo(bounding_rect.width() / 2.f, -bounding_rect.height() / 2.f);
>   path.close();
>   path.offset(0.5, 0.5);
> 
>   // to the left, to the left
>   SkMatrix transform;
>   gfx::Point center = bounding_rect.CenterPoint();
>   transform.setRotate(180, center.x(), center.y());
>   path.transform(transform);
> 
>   gc->drawPath(path, paint);
> }

Okay I fiddled with your thing and it seems like the magic sauce wasn't the
StrokeAndFill or the 0.5 offset but the fact that you were setting the point of
the triangle to precisely half instead of integer-division-half like I was. So I
added a new patch set that draws everything with no magic numbers based on your
code. Unfortunately rotating by 90 degrees was still giving weird results, so I
split into two cases: vertical and horizontal arrows. I think it reads a lot
better, and as a bonus with the flag off in hidpi the arrows don't look QUITE as
crappy.

[esprehn, 2016-05-06 00:41:05]

How does this compare with DetailsMarkerPainter? Can we put the code to draw a
triangle in some shared place? :P

[jbroman, 2016-05-06 15:40:17]

For what it's worth, this seems reasonable to me at a high level, and I'm glad
to see the move back to drawPath (using drawPoint felt like reimplementing
Skia's rasterizer). 

The new version also seems to do much better with GPU raster than the old.

I've translated this to pure Skia (faithfully, I hope) and written both versions
as a fiddle (see parameters at top):
https://fiddle.skia.org/c/29ecb0e30f063216a9d11e19c3e378ae

As far as performance goes, I only know about scrollbars for web content: in the
composited-scrollbar case we already re-raster the arrow buttons very seldom,
and in the non-composited path I'm pretty confident re-drawing the scrolled
contents overwhelms the cost of filling a triangle in Skia, so I don't think
there's anything to worry about.

I'd be surprised if drawing-a-triangle part were a significant part of the cost
in UI; I don't know, but I have to imagine that there's other overhead there
that's causing the slowness you're seeing.

[Bret, 2016-05-06 18:23:02]

On 2016/05/06 00:41:05, esprehn wrote:
> How does this compare with DetailsMarkerPainter? Can we put the code to draw a
> triangle in some shared place? :P

If I'm reading the code correctly DetailsMarkerPainter is for the <details> tag?
That's a different-looking triangle, and it's not anti-aliased. It might be
worth abstracting them together but it's not a slam dunk so it seems out of
scope for this change.

[Bret, 2016-05-11 17:46:49]

Any more thoughts on this change? As far as I know it's ready to go.

[jbroman, 2016-05-11 21:24:08]

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:531:
"scroll/ScrollbarThemeAuraTest.cpp",
The condition for this should probably match that of ScrollbarThemeAura.cpp.
Suggest:

if (use_default_render_theme) {
  sources += [ "scroll/ScrollbarThemeAuraTest.cpp" ]
}

(use_default_render_theme = use_aura && !is_android, for context)

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/blink_platform.gypi:1322:
'scroll/ScrollbarThemeAuraTest.cpp',
similarly, this should be guarded by use_default_render_theme==1

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp
(right):

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp:17:
EXPECT_CALL(*mock, layerForHorizontalScrollbar())
Is it important for these tests that this be a non-composited thing? If not, I'd
suggest using ON_CALL instead:

ON_CALL(*mock, layerForHorizontalScrollbar())
    .WillByDefault(Return(nullptr));

...that is, if that isn't already the default. It seems wrong to have an
expectation that sizing buttons checks whether scrollbars have a layer.

[Bret, 2016-05-12 00:18:12]

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:531:
"scroll/ScrollbarThemeAuraTest.cpp",
On 2016/05/11 21:24:08, jbroman wrote:
> The condition for this should probably match that of ScrollbarThemeAura.cpp.
> Suggest:
> 
> if (use_default_render_theme) {
>   sources += [ "scroll/ScrollbarThemeAuraTest.cpp" ]
> }
> 
> (use_default_render_theme = use_aura && !is_android, for context)

Done.

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/blink_platform.gypi:1322:
'scroll/ScrollbarThemeAuraTest.cpp',
On 2016/05/11 21:24:08, jbroman wrote:
> similarly, this should be guarded by use_default_render_theme==1

Done.

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp
(right):

https://codereview.chromium.org/1911973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeAuraTest.cpp:17:
EXPECT_CALL(*mock, layerForHorizontalScrollbar())
On 2016/05/11 21:24:08, jbroman wrote:
> Is it important for these tests that this be a non-composited thing? If not,
I'd
> suggest using ON_CALL instead:
> 
> ON_CALL(*mock, layerForHorizontalScrollbar())
>     .WillByDefault(Return(nullptr));
> 
> ...that is, if that isn't already the default. It seems wrong to have an
> expectation that sizing buttons checks whether scrollbars have a layer.

Hmm, okay I deleted this block. I added it originally to get rid of the gmock
warnings, but based on what they say and what you said I guess it's better to
ignore them. Seems weird to just leave warnings though.

[Evan Stade, 2016-05-12 01:10:59]

NatievThemeBase seems reasonable enough to me, lgtm

https://codereview.chromium.org/1911973002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/1911973002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_base.cc:363: paint.setAntiAlias(false);
i think anti aliasing is false by default

https://codereview.chromium.org/1911973002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_base.cc:383: path.moveTo(bounding_rect.x(),
bounding_rect.y() + bounding_rect.height());
nit: bottom()

[Bret, 2016-05-12 21:59:13]

https://codereview.chromium.org/1911973002/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/1911973002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_base.cc:363: paint.setAntiAlias(false);
On 2016/05/12 01:10:59, Evan Stade wrote:
> i think anti aliasing is false by default

*squints* Hmm, you are correct. Removed.

https://codereview.chromium.org/1911973002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_base.cc:383: path.moveTo(bounding_rect.x(),
bounding_rect.y() + bounding_rect.height());
On 2016/05/12 01:10:59, Evan Stade wrote:
> nit: bottom()

Done.

[jbroman, 2016-05-13 17:35:50]

platform lgtm

https://codereview.chromium.org/1911973002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h (right):

https://codereview.chromium.org/1911973002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
This file is not listed in the build files; please add it (probably to
platform_test_support_files in blink_platform.gypi).

[Bret, 2016-05-13 22:11:00]

https://codereview.chromium.org/1911973002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h (right):

https://codereview.chromium.org/1911973002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/05/13 17:35:49, jbroman wrote:
> This file is not listed in the build files; please add it (probably to
> platform_test_support_files in blink_platform.gypi).

Done. The other test support files like ScrollbarThemeMock.h are under
platform_files, but that kinda seems wrong so I did what you said.

[Bret, 2016-05-16 17:50:07]

The CQ bit was checked by bsep@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 17:50:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1911973002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1911973002/140001

[commit-bot: I haz the power, 2016-05-16 19:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-16 19:51:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Bret, 2016-05-18 17:57:00]

Description was changed from

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

Previously on Windows the scrollbar buttons were always sized at 1x and would
leave ugly black space between them at the edge of the window.

BUG=485650
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x device scales
with the #enable-use-zoom-for-dsf flag on.
==========

to

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

Previously on Windows the scrollbar buttons were always sized at 1x and would
leave ugly black space between them at the edge of the window.

BUG=612874
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x device scales
with the #enable-use-zoom-for-dsf flag on.
==========

[Bret, 2016-05-19 20:48:16]

On 2016/05/16 19:51:44, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

FYI: My change to ScrollbarThemeAura seemed to make the scrollbar layout
slightly different in the webkit layout tests so it broke a ton of them. I
reviewed them all and they look okay otherwise, and I believe my change is the
behavior we actually want, so I went ahead and marked them for rebaselining.

[esprehn, 2016-05-19 21:22:45]

Description was changed from

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

Previously on Windows the scrollbar buttons were always sized at 1x and would
leave ugly black space between them at the edge of the window.

BUG=612874
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x device scales
with the #enable-use-zoom-for-dsf flag on.
==========

to

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

Previously on Windows the scrollbar buttons were always sized at 1x
and would leave ugly black space between them at the edge of the
window.

BUG=612874
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x
device scales with the #enable-use-zoom-for-dsf flag on.
==========

[esprehn, 2016-05-19 21:24:18]

rs lgtm

[Bret, 2016-05-19 21:26:48]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-05-19 21:26:48]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1911973002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-19 21:27:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1911973002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1911973002/200001

[commit-bot: I haz the power, 2016-05-19 23:04:38]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-19 23:06:05]

Description was changed from

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

Previously on Windows the scrollbar buttons were always sized at 1x
and would leave ugly black space between them at the edge of the
window.

BUG=612874
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x
device scales with the #enable-use-zoom-for-dsf flag on.
==========

to

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

Previously on Windows the scrollbar buttons were always sized at 1x
and would leave ugly black space between them at the edge of the
window.

BUG=612874
TEST=Scrollbar buttons on Windows should look right at 1.5x and 2x
device scales with the #enable-use-zoom-for-dsf flag on.

Committed: https://crrev.com/6e7c308532450f3a8fedef962ac3d3e14bb80880
Cr-Commit-Position: refs/heads/master@{#394893}
==========

[commit-bot: I haz the power, 2016-05-19 23:06:06]

Patchset 11 (id:??) landed as
https://crrev.com/6e7c308532450f3a8fedef962ac3d3e14bb80880
Cr-Commit-Position: refs/heads/master@{#394893}

[hiroshige, 2016-05-20 07:37:23]

On 2016/05/19 23:06:06, commit-bot: I haz the power wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/6e7c308532450f3a8fedef962ac3d3e14bb80880
> Cr-Commit-Position: refs/heads/master@{#394893}

transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html
is failing after this CL. I'll mark the test as failing. Please take a look.