Use nine-patch resource for drawing Aura overlay scrollbar thumb.

Use nine-patch resource for drawing Aura overlay scrollbar thumb.

This patch factors out the nine-patch generation from NinePatchLayer
into a NinePatchGenerator and uses it to create a nine-patch scrollbar
layer that can change size without a repaint. This is used only in the
compositor. Scrollbars painted in Blink paint to the full required size.

The new scrollbar type is implemented in OverlayScrollbarLayer and Impl.
We add methods to NativeTheme to draw 9-patch versions of a theme part,
currently implemented for the scrollbar thumb for overlays in
NativeThemeAura. The NativeTheme provides two methods:

NinePatchCanvasSize() returns the size of the smallest canvas onto which
a nine patch part can be drawn.

NinePatchAperture() returns the rect in the canvas, whose size is
provided by the method above, which will be used as the middle patch in
the resource. Resizing the part will stretch only the middle patch in
both directions.

This is used mainly for the thinning animation in Aura overlay
scrollbars. We still need to repaint due to hover and pressed effects.
Technically, we could avoid repainting on size changes too but this would
require some additional work.

BUG=669670
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2591863003
Cr-Commit-Position: refs/heads/master@{#454413}
Committed: https://chromium.googlesource.com/chromium/src/+/e7a058aa7db4c0fc172200fd16c800ec1b94d8e6

[bokan, 2016-12-21 01:46:22]

Description was changed from

==========
Use nine-patch resource for drawing Aura overlay scrollbar thumb.

BUG=669670
==========

to

==========
Use nine-patch resource for drawing Aura overlay scrollbar thumb.

BUG=669670
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[bokan, 2016-12-21 01:46:40]

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

[commit-bot: I haz the power, 2016-12-21 01:47:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 02:04:27]

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

[commit-bot: I haz the power, 2016-12-21 02:04:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-12-21 02:07:36]

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

[commit-bot: I haz the power, 2016-12-21 02:08:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 02:24:37]

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

[commit-bot: I haz the power, 2016-12-21 02:24:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bokan, 2016-12-21 13:47:05]

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

[commit-bot: I haz the power, 2016-12-21 13:47:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 14:12:49]

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

[commit-bot: I haz the power, 2016-12-21 14:12:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-12-21 15:11:45]

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

[commit-bot: I haz the power, 2016-12-21 15:12:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 17:13:12]

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

[commit-bot: I haz the power, 2016-12-21 17:13:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-12-21 17:33:31]

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

[commit-bot: I haz the power, 2016-12-21 17:34:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2016-12-21 18:37:40]

Description was changed from

==========
Use nine-patch resource for drawing Aura overlay scrollbar thumb.

BUG=669670
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use nine-patch resource for drawing Aura overlay scrollbar thumb.

This patch factors out the nine-patch generation from NinePatchLayer
into a NinePatchGenerator and uses it to create a nine-patch scrollbar
layer that can change size without a repaint. This is used only in the
compositor. Scrollbars painted in Blink paint to the full required size.

The new scrollbar type is implemented in OverlayScrollbarLayer and Impl.
We add methods to NativeTheme to draw 9-patch versions of a theme part,
currently implemented for the scrollbar thumb for overlays in
NativeThemeAura. The NativeTheme provides two methods:

NinePatchCanvasSize() returns the size of the smallest canvas onto which
a nine patch part can be drawn.

NinePatchAperture() returns the rect in the canvas, whose size is
provided by the method above, which will be used as the middle patch in
the resource. Resizing the part will stretch only the middle patch in
both directions.

This is used mainly for the thinning animation in Aura overlay
scrollbars. We still need to repaint due to hover and pressed effects.
Technically, we could avoid repainting on size changes too but this would
require some additional work.

BUG=669670
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2016-12-21 19:43:35]

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

[commit-bot: I haz the power, 2016-12-21 19:43:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2016-12-21 23:36:09]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2016-12-21 23:36:10]

Alexandre, when you get back, please take an initial look. The patch is big but
it's mostly plumbing since I'd like to keep painting neatly packaged inside
NativeTheme.

I still need to add tests and there's a persistent failure on the linux_android
bot which I'm trying to figure out how this can cause as this shouldn't affect
Android but it should be fairly complete.

This doesn't yet have your desire of a "paint once per CC instance" behavior,
nor my "once per scrollbar", since after your comment on the design doc I
realized we still need to go back to blink to repaint for hover and press
effects. Until we get rid of non-composited scrollers, we'll need to maintain
the scrollbar event handling path in Blink so I don't want to duplicate that
logic. Once we get SPv2 we can move scrollbar eventing to the compositor and
then CC can paint each state of the scrollbar once and keep it for its lifetime,
swapping resources when needed.

[aelias_OOO_until_Jul13, 2017-01-04 03:22:27]

Looks OK overall.

https://codereview.chromium.org/2591863003/diff/100001/cc/layers/overlay_scro...
File cc/layers/overlay_scrollbar_layer.h (right):

https://codereview.chromium.org/2591863003/diff/100001/cc/layers/overlay_scro...
cc/layers/overlay_scrollbar_layer.h:18: class CC_EXPORT OverlayScrollbarLayer :
public ScrollbarLayerInterface,
What's the planned relationship with SolidColorScrollbarLayer?  If
SolidColorScrollbarLayer continues to exist, the naming is a bit confusing since
it's also a kind of overlay scrollbar.  Maybe PaintedOverlayScrollbarLayer
instead until we manage to merge them?

[bokan, 2017-02-28 20:15:46]

On 2017/01/04 03:22:27, aelias wrote:
> Looks OK overall.
> 
>
https://codereview.chromium.org/2591863003/diff/100001/cc/layers/overlay_scro...
> File cc/layers/overlay_scrollbar_layer.h (right):
> 
>
https://codereview.chromium.org/2591863003/diff/100001/cc/layers/overlay_scro...
> cc/layers/overlay_scrollbar_layer.h:18: class CC_EXPORT OverlayScrollbarLayer
:
> public ScrollbarLayerInterface,
> What's the planned relationship with SolidColorScrollbarLayer?  If
> SolidColorScrollbarLayer continues to exist, the naming is a bit confusing
since
> it's also a kind of overlay scrollbar.  Maybe PaintedOverlayScrollbarLayer
> instead until we manage to merge them?

Ok, renamed to PaintedOverlayScrollbarLayer for now

[bokan, 2017-02-28 20:18:38]

Resurrecting this since overlay work has restarted. I've made your suggested
change and added some basic unit tests to the nine patch generator.

It'd be good to add some pixel tests against the scrollbar but I'm not sure
where the best place to do that is -- LayoutTests nominally but they're poorly
suited for animated tests. Any better ideas?

[enne (OOO), 2017-02-28 20:22:04]

What about cc::LayerTreePixelTest? Or do you need ui/Blink as well to test the
output?

[bokan, 2017-02-28 20:30:44]

On 2017/02/28 20:22:04, enne wrote:
> What about cc::LayerTreePixelTest? Or do you need ui/Blink as well to test the
> output?

Thanks, I didn't know about that. I'll investigate if that'll do.

[bokan, 2017-02-28 20:32:16]

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

[commit-bot: I haz the power, 2017-02-28 20:33:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 21:09:20]

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

[commit-bot: I haz the power, 2017-02-28 21:09:21]

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

[aelias_OOO_until_Jul13, 2017-02-28 23:23:51]

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
File cc/layers/painted_overlay_scrollbar_layer.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:113: if (!host || host !=
layer_tree_host()) {
Nit: "!host ||" is no-op, can be removed.

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:115: }
nit: no {}

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:127: base::AutoReset<bool>
ignore_set_needs_commit(&ignore_set_needs_commit_,
It looks like this is copied from PaintedScrollbarLayer, but I'm not sure what
it's for.  Could you either delete it or add a comment explaining it?

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
File cc/layers/painted_overlay_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer_impl.cc:78: void
PaintedOverlayScrollbarLayerImpl::AppendQuads(
There's still a bit of code duplication here with NinePatchLayerImpl, is there
some way of sharing code for some of this method too?

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
File cc/quads/nine_patch_generator.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
cc/quads/nine_patch_generator.cc:13: #include "cc/quads/texture_draw_quad.h"
Some of these includes aren't used (unless you move some of AppendQuads here as
I suggested above).

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
File cc/quads/nine_patch_generator_unittest.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
cc/quads/nine_patch_generator_unittest.cc:53: EXPECT_EQ(gfx::RectF(0.1, 0.1,
0.8, 0.8), patches[8].normalized_image_rect);
According to trybot, this doesn't compile on Windows:
"e:\b\c\b\win\src\cc\quads\nine_patch_generator_unittest.cc(53): warning C4305:
'argument': truncation from 'double' to 'float'"

[bokan, 2017-03-01 00:44:20]

bokan@chromium.org changed reviewers:
+ enne@chromium.org

[bokan, 2017-03-01 00:44:23]

Comments addressed + added a pixel test as enne@ suggested.

+enne@, could you comment on whether this was needed?
https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
File cc/layers/painted_overlay_scrollbar_layer.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:113: if (!host || host !=
layer_tree_host()) {
On 2017/02/28 23:23:51, aelias wrote:
> Nit: "!host ||" is no-op, can be removed.

Done.

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:115: }
On 2017/02/28 23:23:51, aelias wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:127: base::AutoReset<bool>
ignore_set_needs_commit(&ignore_set_needs_commit_,
On 2017/02/28 23:23:51, aelias wrote:
> It looks like this is copied from PaintedScrollbarLayer, but I'm not sure what
> it's for.  Could you either delete it or add a comment explaining it?

This flag was added to Layer in:
https://chromium.googlesource.com/chromium/src/+/29493a1e29003b24d41a8cb44c6e...

And then here in:
https://chromium.googlesource.com/chromium/src/+/467829ba34bd4244b8415e35149e...

Looks like some kind of optimization I don't fully understand. I've removed it
but perhaps enne@ can elaborate.

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
File cc/layers/painted_overlay_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer_impl.cc:78: void
PaintedOverlayScrollbarLayerImpl::AppendQuads(
On 2017/02/28 23:23:51, aelias wrote:
> There's still a bit of code duplication here with NinePatchLayerImpl, is there
> some way of sharing code for some of this method too?

Ok, I've moved as much as I could into NinePatchGenerator::AppendQuads. WDYT?

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
File cc/quads/nine_patch_generator.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
cc/quads/nine_patch_generator.cc:13: #include "cc/quads/texture_draw_quad.h"
On 2017/02/28 23:23:51, aelias wrote:
> Some of these includes aren't used (unless you move some of AppendQuads here
as
> I suggested above).

Done and pared this down to what's needed.

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
File cc/quads/nine_patch_generator_unittest.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/quads/nine_patch_ge...
cc/quads/nine_patch_generator_unittest.cc:53: EXPECT_EQ(gfx::RectF(0.1, 0.1,
0.8, 0.8), patches[8].normalized_image_rect);
On 2017/02/28 23:23:51, aelias wrote:
> According to trybot, this doesn't compile on Windows:
> "e:\b\c\b\win\src\cc\quads\nine_patch_generator_unittest.cc(53): warning
C4305:
> 'argument': truncation from 'double' to 'float'"

Fixed

[bokan, 2017-03-01 00:44:32]

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

[commit-bot: I haz the power, 2017-03-01 00:44:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-03-01 00:52:21]

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
File cc/layers/painted_overlay_scrollbar_layer.cc (right):

https://codereview.chromium.org/2591863003/diff/140001/cc/layers/painted_over...
cc/layers/painted_overlay_scrollbar_layer.cc:127: base::AutoReset<bool>
ignore_set_needs_commit(&ignore_set_needs_commit_,
On 2017/03/01 at 00:44:21, bokan wrote:
> On 2017/02/28 23:23:51, aelias wrote:
> > It looks like this is copied from PaintedScrollbarLayer, but I'm not sure
what
> > it's for.  Could you either delete it or add a comment explaining it?
> 
> This flag was added to Layer in:
https://chromium.googlesource.com/chromium/src/+/29493a1e29003b24d41a8cb44c6e...
> 
> And then here in:
>
https://chromium.googlesource.com/chromium/src/+/467829ba34bd4244b8415e35149e...
> 
> Looks like some kind of optimization I don't fully understand. I've removed it
but perhaps enne@ can elaborate.

If you're in the middle of an Update before a commit, then any additional
SetNeedsCommit will cause another commit to happen afterwards due to logic in
LTH.  So, in cases where layers like ContentScalingLayer wanted to do some
updates that would cause invalidations (namely changing its internal contents
scale) then the code didn't want invalidations to cause more commits.

If this code isn't doing any invalidations via SetNeedsDisplay inside of update,
then you don't need the auto reset and ignore dance.  It looks like it's fine to
drop.

[aelias_OOO_until_Jul13, 2017-03-01 00:59:01]

lgtm modulo removing commit optimization, thanks.

[commit-bot: I haz the power, 2017-03-01 03:09:59]

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

[commit-bot: I haz the power, 2017-03-01 03:10:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2017-03-01 13:53:23]

On 2017/03/01 00:59:01, aelias wrote:
> lgtm modulo removing commit optimization, thanks.

Just to confirm -- I removed the commit optimisation in the latest patch already
so the patch you stamped should be good.

[bokan, 2017-03-01 13:56:21]

bokan@chromium.org changed reviewers:
+ estade@chromium.org, jbroman@chromium.org, nasko@chromium.org,
sadrul@chromium.org

[bokan, 2017-03-01 13:56:22]

+jbroman@ for WebKit/Source/Platform
+nasko@ for content/child
+estade@ for native_theme
+sadrul@ for ui/views

[jbroman, 2017-03-01 15:27:18]

Source/platform/ lgtm

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:199: // For a
nine-patch resource, the aperture defines the size of the center
Just the size? I'd expect it to define the center patch (both its size and its
position).

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp
(right):

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:252:
WebThemeEngine::Part part = WebThemeEngine::PartScrollbarHorizontalThumb;
super-nit: for something this simple why not a ternary?

WebThemeEngine::Part part = scrollbar.orientation() == VerticalScrollbar
    ? WebThemeEngine::PartScrollbarVerticalThumb
    : WebThemeEngine::PartScrollbarHorizontalThumb;

Seems a little clearer to me than assigning and then reassigning the variable,
but up to you.

[sadrul, 2017-03-01 15:38:55]

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
ui/native_theme/native_theme.h:272: virtual bool SupportsNinePatch(Part part)
const = 0;
Can you document this?

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:664: return false;
We are using the same compositor on windows. Why would it not support nine-patch
layer?

[bokan, 2017-03-01 16:22:43]

bokan@chromium.org changed reviewers:
+ alexmos@chromium.org
- nasko@chromium.org

[bokan, 2017-03-01 16:22:45]

-nasko@ since he's marked (Slow) +alexmos@ in his place for content/child.

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:199: // For a
nine-patch resource, the aperture defines the size of the center
On 2017/03/01 15:27:18, jbroman wrote:
> Just the size? I'd expect it to define the center patch (both its size and its
> position).

You're right, this was an error in the comment. Fixed.

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp
(right):

https://codereview.chromium.org/2591863003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:252:
WebThemeEngine::Part part = WebThemeEngine::PartScrollbarHorizontalThumb;
On 2017/03/01 15:27:18, jbroman wrote:
> super-nit: for something this simple why not a ternary?
> 
> WebThemeEngine::Part part = scrollbar.orientation() == VerticalScrollbar
>     ? WebThemeEngine::PartScrollbarVerticalThumb
>     : WebThemeEngine::PartScrollbarHorizontalThumb;
> 
> Seems a little clearer to me than assigning and then reassigning the variable,
> but up to you.

Done.

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
ui/native_theme/native_theme.h:272: virtual bool SupportsNinePatch(Part part)
const = 0;
On 2017/03/01 15:38:55, sadrul wrote:
> Can you document this?

Done.

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2591863003/diff/180001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:664: return false;
On 2017/03/01 15:38:55, sadrul wrote:
> We are using the same compositor on windows. Why would it not support
nine-patch
> layer?

Only the scrollbar thumb for overlay scrollbars will now be a nine-patch.
That'll only be used on CrOS in stable, but even on windows if enabled via flag,
the overlay scrollbar thumb is painted by native_theme_aura (as is all Web
content). I've added a comment here to make that clearer.

[sadrul, 2017-03-01 16:30:27]

Thanks. lgtm

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:665: // paitned by NativeThemeAura on
Windows.
*painted

[Evan Stade, 2017-03-01 16:45:50]

(perhaps a TODO) Have you tested this at fractional scales, such as 1.25?

https://codereview.chromium.org/2591863003/diff/220001/cc/layers/nine_patch_l...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/2591863003/diff/220001/cc/layers/nine_patch_l...
cc/layers/nine_patch_layer_impl.cc:69:
gfx::RectF(gfx::ToFlooredRectDeprecated(patch.output_rect));
seems like a function call you shouldn't be adding.

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:330: // 2 pixel border with 1 pixel center
patch.
2 pixels or 2 dip? Why 2 pixels if OverlayScrollbarStrokeWidth is 1?

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:331: return gfx::Size(5, 5);
can you pull out these values into named constants?

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:340: return gfx::Rect(2, 2, 1, 1);
ditto

[bokan, 2017-03-01 17:02:31]

https://codereview.chromium.org/2591863003/diff/220001/cc/layers/nine_patch_l...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/2591863003/diff/220001/cc/layers/nine_patch_l...
cc/layers/nine_patch_layer_impl.cc:69:
gfx::RectF(gfx::ToFlooredRectDeprecated(patch.output_rect));
On 2017/03/01 16:45:49, Evan Stade wrote:
> seems like a function call you shouldn't be adding.

It's existing, moved from inside the loop this is replacing. I suspect we could
get rid of this flooring, but given that I don't really know what this code is
supporting I'm reluctant to change it.

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:330: // 2 pixel border with 1 pixel center
patch.
On 2017/03/01 16:45:49, Evan Stade wrote:
> 2 pixels or 2 dip? Why 2 pixels if OverlayScrollbarStrokeWidth is 1?

It's to prevent color interpolation between the patches. The scrollbar stroke is
the outer pixel, the inner pixel should match the color of the inner patch so
that we don't get a smooth transition from the stroke color to the fill color.
Added a clarifying comment.

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:331: return gfx::Size(5, 5);
On 2017/03/01 16:45:49, Evan Stade wrote:
> can you pull out these values into named constants?

Done.

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:340: return gfx::Rect(2, 2, 1, 1);
On 2017/03/01 16:45:49, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/2591863003/diff/220001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:665: // paitned by NativeThemeAura on
Windows.
On 2017/03/01 16:30:27, sadrul wrote:
> *painted

Done.

[bokan, 2017-03-01 17:04:37]

On 2017/03/01 16:45:50, Evan Stade wrote:
> (perhaps a TODO) Have you tested this at fractional scales, such as 1.25?

Do you mean devicePixelRatio? Not yet, but I've already got a bug
(http://crbug.com/669675) to make sure non-1 DPRs are painted correctly so I'll
address it there. If you mean for the thickness scaling, the pixel test I added
has a case for scaling to 0.4.

[alexmos, 2017-03-01 17:44:14]

content/child LGTM

[bokan, 2017-03-01 18:17:14]

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

[commit-bot: I haz the power, 2017-03-01 18:17:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 20:41:52]

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

[commit-bot: I haz the power, 2017-03-01 20:41:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2017-03-01 22:30:37]

On 2017/03/01 17:04:37, bokan wrote:
> On 2017/03/01 16:45:50, Evan Stade wrote:
> > (perhaps a TODO) Have you tested this at fractional scales, such as 1.25?
> 
> Do you mean devicePixelRatio? Not yet, but I've already got a bug
> (http://crbug.com/669675) to make sure non-1 DPRs are painted correctly so
I'll
> address it there. If you mean for the thickness scaling, the pixel test I
added
> has a case for scaling to 0.4.

I think so. devicePixelRatio may be one name for it. The name in Chrome code is
"device scale factor" and the CL flag is --device-scale-factor. There is also a
"page scale factor" which I believe is only relevant to web content/Blink.

Making sure a 2x scale (a la Pixel) works is very important, of course. But
fractional scales like 1.25x are usually harder to get right. Often it's a
matter of choosing the right combination of scaling and rounding/enclosing. For
example, according to the bug you linked, the stroke should be 1px. The overall
width of the bar is probably expressed in dip though. If that overall width is,
say, 13dip, that will come to an integral number of physical pixels for integral
scale factors like 1 and 2, but would be a fractional number of pixels at 1.25x,
so we want/need to be sure to round appropriately in that case.

[bokan, 2017-03-01 23:14:53]

On 2017/03/01 22:30:37, Evan Stade wrote:
> On 2017/03/01 17:04:37, bokan wrote:
> > On 2017/03/01 16:45:50, Evan Stade wrote:
> > > (perhaps a TODO) Have you tested this at fractional scales, such as 1.25?
> > 
> > Do you mean devicePixelRatio? Not yet, but I've already got a bug
> > (http://crbug.com/669675) to make sure non-1 DPRs are painted correctly so
> I'll
> > address it there. If you mean for the thickness scaling, the pixel test I
> added
> > has a case for scaling to 0.4.
> 
> I think so. devicePixelRatio may be one name for it. The name in Chrome code
is
> "device scale factor" and the CL flag is --device-scale-factor. There is also
a
> "page scale factor" which I believe is only relevant to web content/Blink.
> 
> Making sure a 2x scale (a la Pixel) works is very important, of course. But
> fractional scales like 1.25x are usually harder to get right. Often it's a
> matter of choosing the right combination of scaling and rounding/enclosing.
For
> example, according to the bug you linked, the stroke should be 1px. The
overall
> width of the bar is probably expressed in dip though. If that overall width
is,
> say, 13dip, that will come to an integral number of physical pixels for
integral
> scale factors like 1 and 2, but would be a fractional number of pixels at
1.25x,
> so we want/need to be sure to round appropriately in that case.

Ok, gave it a whirl with a couple different DSFs of various fractions and
everything seems to work correctly. Tom/Sebastien had some changes they wanted
in that bug related to DSF but that's more design than correctness so I'll add
DSF related tests there. Ok to land?

[Evan Stade, 2017-03-02 02:32:01]

https://codereview.chromium.org/2591863003/diff/240001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2591863003/diff/240001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:37: const gfx::Size
kOverlayScrollbarNinePatchCanvasSize(5, 5);
sorry, what I actually meant here was something like

constexpr int kBorderStroke = 2;
constexpr int kCenterPatchSize = 1;

then down in the functions below

return gfx::Size(kBorderStroke * 2 + kCenterPatchSize, kBorderStroke * 2 +
kCenterPatchSize);

return gfx::Rect(kBorderStroke, kBorderStroke, kCenterPatchSize,
kCenterPatchSize);

[Evan Stade, 2017-03-02 02:42:08]

On 2017/03/02 02:32:01, Evan Stade wrote:
>
https://codereview.chromium.org/2591863003/diff/240001/ui/native_theme/native...
> File ui/native_theme/native_theme_aura.cc (right):
> 
>
https://codereview.chromium.org/2591863003/diff/240001/ui/native_theme/native...
> ui/native_theme/native_theme_aura.cc:37: const gfx::Size
> kOverlayScrollbarNinePatchCanvasSize(5, 5);
> sorry, what I actually meant here was something like
> 
> constexpr int kBorderStroke = 2;
> constexpr int kCenterPatchSize = 1;
> 
> then down in the functions below
> 
> return gfx::Size(kBorderStroke * 2 + kCenterPatchSize, kBorderStroke * 2 +
> kCenterPatchSize);
> 
> return gfx::Rect(kBorderStroke, kBorderStroke, kCenterPatchSize,
> kCenterPatchSize);

with that, lgtm

[bokan, 2017-03-02 13:55:36]

https://codereview.chromium.org/2591863003/diff/240001/ui/native_theme/native...
File ui/native_theme/native_theme_aura.cc (right):

https://codereview.chromium.org/2591863003/diff/240001/ui/native_theme/native...
ui/native_theme/native_theme_aura.cc:37: const gfx::Size
kOverlayScrollbarNinePatchCanvasSize(5, 5);
On 2017/03/02 02:32:01, Evan Stade wrote:
> sorry, what I actually meant here was something like
> 
> constexpr int kBorderStroke = 2;
> constexpr int kCenterPatchSize = 1;
> 
> then down in the functions below
> 
> return gfx::Size(kBorderStroke * 2 + kCenterPatchSize, kBorderStroke * 2 +
> kCenterPatchSize);
> 
> return gfx::Rect(kBorderStroke, kBorderStroke, kCenterPatchSize,
> kCenterPatchSize);

Ah, that does sound better. Done (names slightly altered).

[bokan, 2017-03-02 13:55:52]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-03-02 13:55:52]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
jbroman@chromium.org, sadrul@chromium.org, alexmos@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2591863003/#ps210046
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-02 13:56:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 15:32:32]

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

[commit-bot: I haz the power, 2017-03-02 15:32:34]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bokan, 2017-03-02 15:37:49]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2017-03-02 15:38:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 17:07:14]

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

[commit-bot: I haz the power, 2017-03-02 17:07:16]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2017-03-02 20:53:36]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-03-02 20:53:37]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
sadrul@chromium.org, jbroman@chromium.org, alexmos@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2591863003/#ps290001
(title: "Move CheckGeometryLimitations back to where it used to be
(AppendQuads)")

[commit-bot: I haz the power, 2017-03-02 20:54:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 22:42:55]

CQ is committing da patch.

Bot data: {"patchset_id": 290001, "attempt_start_ts": 1488488016458870,
"parent_rev": "924779f9f8c9c9c3dfea4f52577432fb6b656690", "commit_rev":
"e7a058aa7db4c0fc172200fd16c800ec1b94d8e6"}

[commit-bot: I haz the power, 2017-03-02 22:43:42]

Description was changed from

==========
Use nine-patch resource for drawing Aura overlay scrollbar thumb.

This patch factors out the nine-patch generation from NinePatchLayer
into a NinePatchGenerator and uses it to create a nine-patch scrollbar
layer that can change size without a repaint. This is used only in the
compositor. Scrollbars painted in Blink paint to the full required size.

The new scrollbar type is implemented in OverlayScrollbarLayer and Impl.
We add methods to NativeTheme to draw 9-patch versions of a theme part,
currently implemented for the scrollbar thumb for overlays in
NativeThemeAura. The NativeTheme provides two methods:

NinePatchCanvasSize() returns the size of the smallest canvas onto which
a nine patch part can be drawn.

NinePatchAperture() returns the rect in the canvas, whose size is
provided by the method above, which will be used as the middle patch in
the resource. Resizing the part will stretch only the middle patch in
both directions.

This is used mainly for the thinning animation in Aura overlay
scrollbars. We still need to repaint due to hover and pressed effects.
Technically, we could avoid repainting on size changes too but this would
require some additional work.

BUG=669670
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use nine-patch resource for drawing Aura overlay scrollbar thumb.

This patch factors out the nine-patch generation from NinePatchLayer
into a NinePatchGenerator and uses it to create a nine-patch scrollbar
layer that can change size without a repaint. This is used only in the
compositor. Scrollbars painted in Blink paint to the full required size.

The new scrollbar type is implemented in OverlayScrollbarLayer and Impl.
We add methods to NativeTheme to draw 9-patch versions of a theme part,
currently implemented for the scrollbar thumb for overlays in
NativeThemeAura. The NativeTheme provides two methods:

NinePatchCanvasSize() returns the size of the smallest canvas onto which
a nine patch part can be drawn.

NinePatchAperture() returns the rect in the canvas, whose size is
provided by the method above, which will be used as the middle patch in
the resource. Resizing the part will stretch only the middle patch in
both directions.

This is used mainly for the thinning animation in Aura overlay
scrollbars. We still need to repaint due to hover and pressed effects.
Technically, we could avoid repainting on size changes too but this would
require some additional work.

BUG=669670
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2591863003
Cr-Commit-Position: refs/heads/master@{#454413}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7a058aa7db4c0fc172200fd16c8...
==========

[commit-bot: I haz the power, 2017-03-02 22:43:44]

Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/e7a058aa7db4c0fc172200fd16c8...