Mac: Don't repaint scrollbars every frame

Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
TEST=
- Set the system to only show scrollbars while scrolling
- Open a page with a vertical scrollbar
- Scroll to make the scroll thumb appear
- Hover the mouse somewhere below the thumb
- The thumb should become thicker and the track should appear
- After some time, the thumb and the track should fade away
TEST=
- Open a page with a scrollbar and search (command-F) for some text
- Ensure that the ticks on the vertical scrollbar appear
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/a54da38a2d8b44643a70b1fbe8297f8fd2365f9c
Cr-Commit-Position: refs/heads/master@{#361938}

[ccameron, 2015-11-21 02:30:30]

Description was changed from

==========
Mac: Don't repaint scrollbars every frame

BUG=554234
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows what whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.
==========

[ccameron, 2015-11-23 06:52:31]

ccameron@chromium.org changed reviewers:
+ chrishtr@chromium.org, enne@chromium.org, pdr@chromium.org

[ccameron, 2015-11-23 06:52:32]

This is that thing we were discussing on Friday about the scrolling and the
sadness.

Adding enne@ for cc/ and pdr@ for Blink platform/ and chrishtr@ cause we were
discussing it together.

[ccameron, 2015-11-23 06:53:33]

Description was changed from

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows what whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2015-11-23 18:59:42]

This code needs tests. Please also add a bug.

Finally, please take a look at the non-mac implementations. Can they easily
benefit from the same optimizations?

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
(right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:82:
bool needsPaintTrack() const override;
Ditto about renaming methods to read more easily,

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:88: // The opacity of
the thumb (only non-opaque for Mac overlay scrollbars).
Don't specify Mac-specific semantics.

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:89: virtual float
thumbOpacity(const ScrollbarThemeClient*) const { return 1.0f; }
How is the client supposed to know whether the opacity is included in the
SkPicture painted by the scrollbar, or it's ok to animate it separately?

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollbarThemePainter.h (right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollbarThemePainter.h:70:
BLINK_PLATFORM_EXPORT bool needsPaintTrack() const;
trackBackgroudNeedsRepaint()
thumbNeedsRepaint()

[enne (OOO), 2015-11-23 19:05:24]

cc end of things looks reasonable to me.

https://codereview.chromium.org/1458703010/diff/20001/cc/blink/scrollbar_impl.cc
File cc/blink/scrollbar_impl.cc (right):

https://codereview.chromium.org/1458703010/diff/20001/cc/blink/scrollbar_impl...
cc/blink/scrollbar_impl.cc:76: if (part == cc::THUMB)
This should just return false and maybe also NOTREACHED, as it doesn't apply on
the compositor thread?

https://codereview.chromium.org/1458703010/diff/20001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.h (right):

https://codereview.chromium.org/1458703010/diff/20001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.h:32: const RendererCapabilities&
GetRendererCapabilities() const override;
What are these renderer capability changes for?

[ccameron, 2015-11-24 01:12:05]

Updated, added more tests.

From some looks, Aura should be able to use these -- the divergence on platforms
is in ui::, so a full implementation would require plumbing through there (esp
for Mac, when that becomes a full implementation). Let's keep the  patch focused
on this one platform implementation for now.

https://codereview.chromium.org/1458703010/diff/20001/cc/blink/scrollbar_impl.cc
File cc/blink/scrollbar_impl.cc (right):

https://codereview.chromium.org/1458703010/diff/20001/cc/blink/scrollbar_impl...
cc/blink/scrollbar_impl.cc:76: if (part == cc::THUMB)
On 2015/11/23 19:05:24, enne wrote:
> This should just return false and maybe also NOTREACHED, as it doesn't apply
on
> the compositor thread?

Mmh, this is called on the main thread -- the "Impl" in ScrollbarImpl is "the
implementation of the abstract class Scrollbar", not "on the impl thread".

https://codereview.chromium.org/1458703010/diff/20001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.h (right):

https://codereview.chromium.org/1458703010/diff/20001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.h:32: const RendererCapabilities&
GetRendererCapabilities() const override;
On 2015/11/23 19:05:24, enne wrote:
> What are these renderer capability changes for?

The issue is the call to get max texture size, from
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/painted_...

In principle, I could cut a bit closer (overriding the PaintedScrollbarLayer
method), but I felt this was a better.

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
(right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:82:
bool needsPaintTrack() const override;
On 2015/11/23 18:59:42, chrishtr wrote:
> Ditto about renaming methods to read more easily,

Done.

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:88: // The opacity of
the thumb (only non-opaque for Mac overlay scrollbars).
On 2015/11/23 18:59:42, chrishtr wrote:
> Don't specify Mac-specific semantics.

Removed (I'd prefer to say specify the use, at least with an "e.g" if not an
"only", though).

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:89: virtual float
thumbOpacity(const ScrollbarThemeClient*) const { return 1.0f; }
On 2015/11/23 18:59:42, chrishtr wrote:
> How is the client supposed to know whether the opacity is included in the
> SkPicture painted by the scrollbar, or it's ok to animate it separately?

Opacity is always taken into account when re-drawing (it has no cost to change,
unlike re-painting the SkPicture).

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollbarThemePainter.h (right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollbarThemePainter.h:70:
BLINK_PLATFORM_EXPORT bool needsPaintTrack() const;
On 2015/11/23 18:59:42, chrishtr wrote:
> trackBackgroudNeedsRepaint()
> thumbNeedsRepaint()

Done.

[enne (OOO), 2015-11-24 17:59:18]

cc lgtm

https://codereview.chromium.org/1458703010/diff/20001/cc/blink/scrollbar_impl.cc
File cc/blink/scrollbar_impl.cc (right):

https://codereview.chromium.org/1458703010/diff/20001/cc/blink/scrollbar_impl...
cc/blink/scrollbar_impl.cc:76: if (part == cc::THUMB)
On 2015/11/24 at 01:12:04, ccameron wrote:
> On 2015/11/23 19:05:24, enne wrote:
> > This should just return false and maybe also NOTREACHED, as it doesn't apply
on
> > the compositor thread?
> 
> Mmh, this is called on the main thread -- the "Impl" in ScrollbarImpl is "the
implementation of the abstract class Scrollbar", not "on the impl thread".

<_<
>_>

https://codereview.chromium.org/1458703010/diff/20001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.h (right):

https://codereview.chromium.org/1458703010/diff/20001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.h:32: const RendererCapabilities&
GetRendererCapabilities() const override;
On 2015/11/24 at 01:12:04, ccameron wrote:
> On 2015/11/23 19:05:24, enne wrote:
> > What are these renderer capability changes for?
> 
> The issue is the call to get max texture size, from
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/painted_...
> 
> In principle, I could cut a bit closer (overriding the PaintedScrollbarLayer
method), but I felt this was a better.

Ohhhh, max texture size.  Thanks for the explanation.

https://codereview.chromium.org/1458703010/diff/30028/cc/blink/scrollbar_impl.cc
File cc/blink/scrollbar_impl.cc (right):

https://codereview.chromium.org/1458703010/diff/30028/cc/blink/scrollbar_impl...
cc/blink/scrollbar_impl.cc:77: return painter_.trackBackgroudNeedsRepaint();
typo?

https://codereview.chromium.org/1458703010/diff/30028/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/1458703010/diff/30028/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer_unittest.cc:55: // yet bee initialized.
typo

[ccameron, 2015-11-24 18:05:47]

Description was changed from

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ccameron, 2015-11-24 18:48:19]

Thanks -- updated.

https://codereview.chromium.org/1458703010/diff/30028/cc/blink/scrollbar_impl.cc
File cc/blink/scrollbar_impl.cc (right):

https://codereview.chromium.org/1458703010/diff/30028/cc/blink/scrollbar_impl...
cc/blink/scrollbar_impl.cc:77: return painter_.trackBackgroudNeedsRepaint();
On 2015/11/24 17:59:18, enne wrote:
> typo?

Which one? (I think this is right -- I'm following the PaintPart style if "if
thumb else assume track").

https://codereview.chromium.org/1458703010/diff/30028/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/1458703010/diff/30028/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer_unittest.cc:55: // yet bee initialized.
On 2015/11/24 17:59:18, enne wrote:
> typo

Oops -- fixed.

[enne (OOO), 2015-11-24 18:55:13]

https://codereview.chromium.org/1458703010/diff/30028/cc/blink/scrollbar_impl.cc
File cc/blink/scrollbar_impl.cc (right):

https://codereview.chromium.org/1458703010/diff/30028/cc/blink/scrollbar_impl...
cc/blink/scrollbar_impl.cc:77: return painter_.trackBackgroudNeedsRepaint();
On 2015/11/24 at 18:48:19, ccameron wrote:
> On 2015/11/24 17:59:18, enne wrote:
> > typo?
> 
> Which one? (I think this is right -- I'm following the PaintPart style if "if
thumb else assume track").

It's hidden in the backgroud.

[chrishtr, 2015-11-24 19:02:07]

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):

https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:89: virtual float
thumbOpacity(const ScrollbarThemeClient*) const { return 1.0f; }
On 2015/11/24 at 01:12:05, ccameron wrote:
> On 2015/11/23 18:59:42, chrishtr wrote:
> > How is the client supposed to know whether the opacity is included in the
> > SkPicture painted by the scrollbar, or it's ok to animate it separately?
> 
> Opacity is always taken into account when re-drawing (it has no cost to
change, unlike re-painting the SkPicture).

What I mean is: what is the correct way to interpret this API from the cc
perspective? If opacity is < 1 then always apply it as an effect on top of the
SkPicture, and if opacity == 1 don't? And this works for non-Mac because there
is no transparency for the other themes right now?

https://codereview.chromium.org/1458703010/diff/70001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollbarThemePainter.h (right):

https://codereview.chromium.org/1458703010/diff/70001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollbarThemePainter.h:70:
BLINK_PLATFORM_EXPORT bool trackBackgroudNeedsRepaint() const;
typo in Background

[ccameron, 2015-11-24 19:23:39]

On 2015/11/24 19:02:07, chrishtr wrote:
>
https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):
> 
>
https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:89: virtual float
> thumbOpacity(const ScrollbarThemeClient*) const { return 1.0f; }
> On 2015/11/24 at 01:12:05, ccameron wrote:
> > On 2015/11/23 18:59:42, chrishtr wrote:
> > > How is the client supposed to know whether the opacity is included in the
> > > SkPicture painted by the scrollbar, or it's ok to animate it separately?
> > 
> > Opacity is always taken into account when re-drawing (it has no cost to
> change, unlike re-painting the SkPicture).
> 
> What I mean is: what is the correct way to interpret this API from the cc
> perspective? If opacity is < 1 then always apply it as an effect on top of the
> SkPicture, and if opacity == 1 don't? And this works for non-Mac because there
> is no transparency for the other themes right now?

Almost -- it's just "always apply opacity on top of the SkPicture". If opacity
== 1, then this operation is the identity.

>
https://codereview.chromium.org/1458703010/diff/70001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebScrollbarThemePainter.h (right):
> 
>
https://codereview.chromium.org/1458703010/diff/70001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebScrollbarThemePainter.h:70:
> BLINK_PLATFORM_EXPORT bool trackBackgroudNeedsRepaint() const;
> typo in Background

Fixed.

[chrishtr, 2015-11-24 21:08:29]

On 2015/11/24 at 19:23:39, ccameron wrote:
> On 2015/11/24 19:02:07, chrishtr wrote:
> >
https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h (right):
> > 
> >
https://codereview.chromium.org/1458703010/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h:89: virtual float
> > thumbOpacity(const ScrollbarThemeClient*) const { return 1.0f; }
> > On 2015/11/24 at 01:12:05, ccameron wrote:
> > > On 2015/11/23 18:59:42, chrishtr wrote:
> > > > How is the client supposed to know whether the opacity is included in
the
> > > > SkPicture painted by the scrollbar, or it's ok to animate it separately?
> > > 
> > > Opacity is always taken into account when re-drawing (it has no cost to
> > change, unlike re-painting the SkPicture).
> > 
> > What I mean is: what is the correct way to interpret this API from the cc
> > perspective? If opacity is < 1 then always apply it as an effect on top of
the
> > SkPicture, and if opacity == 1 don't? And this works for non-Mac because
there
> > is no transparency for the other themes right now?
> 
> Almost -- it's just "always apply opacity on top of the SkPicture". If opacity
== 1, then this operation is the identity.

Ok please document that explicitly in ThemePainter then.

> 
> >
https://codereview.chromium.org/1458703010/diff/70001/third_party/WebKit/publ...
> > File third_party/WebKit/public/platform/WebScrollbarThemePainter.h (right):
> > 
> >
https://codereview.chromium.org/1458703010/diff/70001/third_party/WebKit/publ...
> > third_party/WebKit/public/platform/WebScrollbarThemePainter.h:70:
> > BLINK_PLATFORM_EXPORT bool trackBackgroudNeedsRepaint() const;
> > typo in Background
> 
> Fixed.

[chrishtr, 2015-11-24 21:15:12]

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (left):

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:803: if
(ScrollbarThemeMacCommon::recommendedScrollerStyle() != NSScrollerStyleLegacy)
What do the changes here and on line 819 do?

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:649:
NSScrollerStateInvisible = 0,
Forgive my ignorance of Objective C. When is NSScrollerStateInvisible ever set?

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp (right):

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp:132:
TEST_F(ScrollableAreaTest, ScrollbarTrackAndThumbRepaint)
This doesn't test the mac code...

[ccameron, 2015-11-25 00:23:02]

Updated.

I un-did chrishtr's suggestion of saying "trackBackground" instead of "track",
because "trackBackground" is not accurate -- the various properties apply to the
entire track. Because of this, we can't actually use the "treat opacity
separately" behavior reliably on Mac, so I've remove the opacity property from
track and only left it with thumb (where it makes a very big difference).

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (left):

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:803: if
(ScrollbarThemeMacCommon::recommendedScrollerStyle() != NSScrollerStyleLegacy)
On 2015/11/24 21:15:12, chrishtr wrote:
> What do the changes here and on line 819 do?

I set the wrong upstream branch -- this is part of another commit (it's gone
now).

For reference, the comment comes in from a WebKit commit in 2012
(https://bugs.webkit.org/show_bug.cgi?id=76133), and is not true on 10.9+.
Again, no documentation, behavior changing between OS releases.

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:649:
NSScrollerStateInvisible = 0,
On 2015/11/24 21:15:12, chrishtr wrote:
> Forgive my ignorance of Objective C. When is NSScrollerStateInvisible ever
set?

That is a value of the newOverlayScrollerState parameter.

This (and the rest of the file) are undocumented private APIs that come from
WebKit (checked in back when this was common with Safari). The enum values of
newOverlayScrollerState are not documented -- I've found empirically that this
is called with the listed 3 states, and so I've added labels for them here.

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp (right):

https://codereview.chromium.org/1458703010/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollableAreaTest.cpp:132:
TEST_F(ScrollableAreaTest, ScrollbarTrackAndThumbRepaint)
On 2015/11/24 21:15:12, chrishtr wrote:
> This doesn't test the mac code...

You're referring to ScrollAnimatorMac? I put quite some time into looking at
what would be required to test the behavior for that. And, I don't think that
exercise should block this patch.

The APIs that ScrollAnimatorMac uses are undocumented and have different
behavior on different OSes (which complicates writing tests that aren't total
flakes). This is probably part of the reason that it has zero existing tests for
its behaviors (and in fact many tests explicitly avoid touching
ScrollAnimatorMac, see [1]).

ScrollAnimatorMac deserves some attention and cleanup (for starters it needs to
change how it animates -- it just fires a timer at 60 Hz -- that shouldn't be
done), and probably a rewrite. But that's an independent issue to this patch.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[chrishtr, 2015-11-25 00:28:49]

In the absence of other tests, can I assume you tested it manually on the
various mac configurations?

https://codereview.chromium.org/1458703010/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/1458703010/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:650: enum {
Please document that these are reverse-engineered. LOL

[ccameron, 2015-11-25 00:34:32]

Yeah, I've kicked the tires quite a bit. The other patch that this was sitting
on top of was a result of manual testing finding existing bugs.

I'll update the CL to include some manual tests as well.

https://codereview.chromium.org/1458703010/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/1458703010/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:650: enum {
On 2015/11/25 00:28:49, chrishtr wrote:
> Please document that these are reverse-engineered. LOL

Done.

[chrishtr, 2015-11-25 00:36:08]

lgtm

[ccameron, 2015-11-25 00:37:20]

Description was changed from

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
TEST=
- Set the system to only show scrollbars while scrolling
- Open a page with a vertical scrollbar
- Scroll to make the scroll thumb appear
- Hover the mouse somewhere below the thumb
- The thumb should become thicker and the track should appear
- After some time, the thumb and the track should fade away
TEST=
- Open a page with a scrollbar and search (command-F) for some text
- Ensure that the ticks on the vertical scrollbar appear
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ccameron, 2015-11-25 07:37:04]

pdr@, could you OWNERS-ify platform/

[chrishtr, 2015-11-25 17:29:26]

On 2015/11/25 at 07:37:04, ccameron wrote:
> pdr@, could you OWNERS-ify platform/

I'm an OWNER of all of Blink, my LGTM should suffice.

[ccameron, 2015-11-26 19:39:43]

Patchset #10 (id:170001) has been deleted

[ccameron, 2015-11-26 19:39:58]

Patchset #10 (id:190001) has been deleted

[ccameron, 2015-11-27 00:46:55]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-11-27 00:46:56]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1458703010/#ps210001
(title: "Rebase and resolve")

[commit-bot: I haz the power, 2015-11-27 00:47:19]

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

[commit-bot: I haz the power, 2015-11-27 00:52:43]

Description was changed from

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
TEST=
- Set the system to only show scrollbars while scrolling
- Open a page with a vertical scrollbar
- Scroll to make the scroll thumb appear
- Hover the mouse somewhere below the thumb
- The thumb should become thicker and the track should appear
- After some time, the thumb and the track should fade away
TEST=
- Open a page with a scrollbar and search (command-F) for some text
- Ensure that the ticks on the vertical scrollbar appear
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
TEST=
- Set the system to only show scrollbars while scrolling
- Open a page with a vertical scrollbar
- Scroll to make the scroll thumb appear
- Hover the mouse somewhere below the thumb
- The thumb should become thicker and the track should appear
- After some time, the thumb and the track should fade away
TEST=
- Open a page with a scrollbar and search (command-F) for some text
- Ensure that the ticks on the vertical scrollbar appear
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-11-27 00:52:44]

Committed patchset #10 (id:210001)

[commit-bot: I haz the power, 2015-11-27 00:54:06]

Description was changed from

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
TEST=
- Set the system to only show scrollbars while scrolling
- Open a page with a vertical scrollbar
- Scroll to make the scroll thumb appear
- Hover the mouse somewhere below the thumb
- The thumb should become thicker and the track should appear
- After some time, the thumb and the track should fade away
TEST=
- Open a page with a scrollbar and search (command-F) for some text
- Ensure that the ticks on the vertical scrollbar appear
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Mac: Don't repaint scrollbars every frame

The core issue here is that Blink, when it sees that it needs to
change the scrollbar in any way (even just moving the thumb), uses the
signal blink::Scrollbar::setNeedsPaintInvalidation(). This will trigger
a call to cc::PaintedScrollbarLayer::Update(), which will re-paint all
controls into textures and then emit quads the quads for the controls.

We often only need to re-arrange the quads for the controls of the
scrollbar, not re-paint them.

The system that knows whether or not the controls need to be
repainted is blink::ScrollbarTheme (because that's the code that knows
the theme that will be used to do the painting).

Add blink::ScrollbarTheme::shouldRepaintAllPartsOnInvalidation() to
indicate if a call to blink::Scrollbar::setNeedsPaintInvalidation()
should cause re-painting of all of the controls. If this returns false
for a given theme, then methods blink::Scrollbar::setNeedsPaintTrack()
and blink::Scrollbar::setNeedsPaintThumb() may be used to specify more
granular control.

Back in cc::PaintedScrollbarLayer::Update(), use the methods
cc::Scrollbar::NeedsPaintPart() to check if re-paint is needed (it is
hooked up to the bit that is set by the blink::Scrollbar methods).

While we're in the neighborhood, it is worth noting that most of the
repainting of scrollbars on Mac is due to the alpha of the thumb or the
track changing. Add methods to blink::Scrollbar to query the opacity
of the controls, so that we can do the blending at compositing time
instead of requiring a repaint.

And, while we're in that neighborhood, fix cc::CALayerOverlay's
FromTextureQuad function to correctly take into account per-vertex
opacity.

BUG=549277
TEST=
- Set the system to only show scrollbars while scrolling
- Open a page with a vertical scrollbar
- Scroll to make the scroll thumb appear
- Hover the mouse somewhere below the thumb
- The thumb should become thicker and the track should appear
- After some time, the thumb and the track should fade away
TEST=
- Open a page with a scrollbar and search (command-F) for some text
- Ensure that the ticks on the vertical scrollbar appear
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/a54da38a2d8b44643a70b1fbe8297f8fd2365f9c
Cr-Commit-Position: refs/heads/master@{#361938}
==========

[commit-bot: I haz the power, 2015-11-27 00:54:08]

Patchset 10 (id:??) landed as
https://crrev.com/a54da38a2d8b44643a70b1fbe8297f8fd2365f9c
Cr-Commit-Position: refs/heads/master@{#361938}