Blink Support for Overlay Scrollbar Animation

Blink Support for Overlay Scrollbar Animation

Add class ScrollbarStateTransitionAnimator (similar to ScrollAnimator)
to add support for size shrink/expand and opacity change animator when
mouse is over/away from overlay scrollbars.

These properties then pass to native theme for painting.

Depended on chromium side change 238723004 (landed).

BUG=307578
TEST=webkit_unit_tests

[weiliangc, 2014-06-05 22:58:36]

Sending this out to get opinion first. Will follow up with testing.

[trchen, 2014-06-05 23:10:28]

On 2014/06/05 22:58:36, weiliangc wrote:
> Sending this out to get opinion first. Will follow up with testing.

Won't this cause lots of repainting during animation?

We have similar problem with Android before, and we implemented our fade-in/out
animation in cc to get reasonable performance. The drawback, of course, is that
our scrollbars can't hide in non-compositing mode, which is not a problem for us
because we always force compositing. I believe it is the case for CrOS too. As
most platforms are forcing compositing by default, I believe in general we favor
impl-side animation over main-thread.

[trchen, 2014-06-05 23:24:24]

cc/animation/scrollbar_animation_controller_thinning.cc looks like what you
described. What is the relationship between that implementation and the one you
uploaded here?

[weiliangc, 2014-06-06 02:12:37]

On 2014/06/05 23:24:24, trchen wrote:
> cc/animation/scrollbar_animation_controller_thinning.cc looks like what you
> described. What is the relationship between that implementation and the one
you
> uploaded here?

This CL is part of effor to prettify overlay scrollbar on Aura.

Currently even with force-compositing mode,on Aura platforms non main-frame
scrollbars (those belong to RenderLayerScrollableArea) are still rendered by
Blink and not promoted to cc. Blink has to understand how to paint, also how to
animate. Before being able to promote all scrollbar to cc, Blink has to know how
to animate.

Also effort is needed to let cc know how to paint prettified overlay scrollbar.
It is nice to have due to performance reason, but not going to be the first
step.

As for the relation to cc's scrollbar_animator_contoller_thinning, this CL does
the same thing just on Blink.

[trchen, 2014-06-06 02:38:08]

On 2014/06/06 02:12:37, weiliangc wrote:
> On 2014/06/05 23:24:24, trchen wrote:
> > cc/animation/scrollbar_animation_controller_thinning.cc looks like what you
> > described. What is the relationship between that implementation and the one
> you
> > uploaded here?
> 
> This CL is part of effor to prettify overlay scrollbar on Aura.
> 
> Currently even with force-compositing mode,on Aura platforms non main-frame
> scrollbars (those belong to RenderLayerScrollableArea) are still rendered by
> Blink and not promoted to cc. Blink has to understand how to paint, also how
to
> animate. Before being able to promote all scrollbar to cc, Blink has to know
how
> to animate.

Why is that? What happens if you enable non-main-frame impl-side scrollbars by
changing the hardcoded value in
ScrollingCoordinator::scrollableAreaScrollbarLayerDidChange() ?

The reason why we didn't enable it on other platform is because the z-order
issue with non-main-frame impl-side scrolling, but I believe vollick@ fixed that
long time ago. If impl-side scrollbars work for main frame on CrOS, why don't
they work for non-main frames?

> Also effort is needed to let cc know how to paint prettified overlay
scrollbar.
> It is nice to have due to performance reason, but not going to be the first
> step.
>
> As for the relation to cc's scrollbar_animator_contoller_thinning, this CL
does
> the same thing just on Blink.

So it looks like we already have some components in cc, then why implementing
the whole thing will be easier in Blink? Is there any difficulty working on cc
that I could help?

[weiliangc, 2014-06-06 04:30:08]

On 2014/06/06 02:38:08, trchen wrote:
> On 2014/06/06 02:12:37, weiliangc wrote:
> > On 2014/06/05 23:24:24, trchen wrote:
> > > cc/animation/scrollbar_animation_controller_thinning.cc looks like what
you
> > > described. What is the relationship between that implementation and the
one
> > you
> > > uploaded here?
> > 
> > This CL is part of effor to prettify overlay scrollbar on Aura.
> > 
> > Currently even with force-compositing mode,on Aura platforms non main-frame
> > scrollbars (those belong to RenderLayerScrollableArea) are still rendered by
> > Blink and not promoted to cc. Blink has to understand how to paint, also how
> to
> > animate. Before being able to promote all scrollbar to cc, Blink has to know
> how
> > to animate.
> 
> Why is that? What happens if you enable non-main-frame impl-side scrollbars by
> changing the hardcoded value in
> ScrollingCoordinator::scrollableAreaScrollbarLayerDidChange() ?
> 
I'll need to check and get back to you on this.

> The reason why we didn't enable it on other platform is because the z-order
> issue with non-main-frame impl-side scrolling, but I believe vollick@ fixed
that
> long time ago. If impl-side scrollbars work for main frame on CrOS, why don't
> they work for non-main frames?

I believe universal overflow scrolling still has maybe one or two blocker? Very
soon, yes.

> 
> > Also effort is needed to let cc know how to paint prettified overlay
> scrollbar.
> > It is nice to have due to performance reason, but not going to be the first
> > step.
> >
> > As for the relation to cc's scrollbar_animator_contoller_thinning, this CL
> does
> > the same thing just on Blink.
> 
> So it looks like we already have some components in cc, then why implementing
> the whole thing will be easier in Blink? Is there any difficulty working on cc
> that I could help?

Even with this animation handled in Blink, repaint on main thread only happens
when pinch-zoom or animation, while general scrolling is still on impl side.
There is no data show this will be a performance pain on CrOS, so optimizing for
performance is not the first problem being tackled.

To port prettified overlay scrollbar into cc (9-patch scrollbar), there needs to
be new scrollbar layer class that gets resource from Blink. Blink rendered
scrollbar has more straight forward painting path(CL 183973008), as for state
transition animation already has scrollAnimator in place with similar logic. I
created new class because scrollAnimators shares a lot of function that is not
used by state transition animation and I do not want to pollute scrollAnimator
with state transition animation code. Prettify overlay scrollbar is experimental
on design, the fastest way to push out consistent look is doing Blink side code
first.

[danakj, 2014-06-06 14:27:23]

On Fri, Jun 6, 2014 at 6:30 AM, <weiliangc@chromium.org> wrote:

> On 2014/06/06 02:38:08, trchen wrote:
>
>> On 2014/06/06 02:12:37, weiliangc wrote:
>> > On 2014/06/05 23:24:24, trchen wrote:
>> > > cc/animation/scrollbar_animation_controller_thinning.cc looks like
>> what
>>
> you
>
>> > > described. What is the relationship between that implementation and
>> the
>>
> one
>
>> > you
>> > > uploaded here?
>> >
>> > This CL is part of effor to prettify overlay scrollbar on Aura.
>> >
>> > Currently even with force-compositing mode,on Aura platforms non
>> main-frame
>> > scrollbars (those belong to RenderLayerScrollableArea) are still
>> rendered by
>> > Blink and not promoted to cc. Blink has to understand how to paint,
>> also how
>> to
>> > animate. Before being able to promote all scrollbar to cc, Blink has to
>> know
>> how
>> > to animate.
>>
>
>  Why is that? What happens if you enable non-main-frame impl-side
>> scrollbars by
>> changing the hardcoded value in
>> ScrollingCoordinator::scrollableAreaScrollbarLayerDidChange() ?
>>
>
> I'll need to check and get back to you on this.
>
>
>  The reason why we didn't enable it on other platform is because the
>> z-order
>> issue with non-main-frame impl-side scrolling, but I believe vollick@
>> fixed
>>
> that
>
>> long time ago. If impl-side scrollbars work for main frame on CrOS, why
>> don't
>> they work for non-main frames?
>>
>
> I believe universal overflow scrolling still has maybe one or two blocker?
> Very
> soon, yes.


We don't turn it on for low-dpi devices cuz promoting causes us to lose LCD
AA, so we need to handle non-promoted scrolling still.


>
>
>
>  > Also effort is needed to let cc know how to paint prettified overlay
>> scrollbar.
>> > It is nice to have due to performance reason, but not going to be the
>> first
>> > step.
>> >
>> > As for the relation to cc's scrollbar_animator_contoller_thinning,
>> this CL
>> does
>> > the same thing just on Blink.
>>
>
>  So it looks like we already have some components in cc, then why
>> implementing
>> the whole thing will be easier in Blink? Is there any difficulty working
>> on cc
>> that I could help?
>>
>
> Even with this animation handled in Blink, repaint on main thread only
> happens
> when pinch-zoom or animation, while general scrolling is still on impl
> side.
> There is no data show this will be a performance pain on CrOS, so
> optimizing for
> performance is not the first problem being tackled.
>
> To port prettified overlay scrollbar into cc (9-patch scrollbar), there
> needs to
> be new scrollbar layer class that gets resource from Blink. Blink rendered
> scrollbar has more straight forward painting path(CL 183973008), as for
> state
> transition animation already has scrollAnimator in place with similar
> logic. I
> created new class because scrollAnimators shares a lot of function that is
> not
> used by state transition animation and I do not want to pollute
> scrollAnimator
> with state transition animation code. Prettify overlay scrollbar is
> experimental
> on design, the fastest way to push out consistent look is doing Blink side
> code
> first.
>
>
> https://codereview.chromium.org/246293006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[trchen, 2014-06-06 21:28:42]

On 2014/06/06 14:27:23, danakj wrote:
> On Fri, Jun 6, 2014 at 6:30 AM, <mailto:weiliangc@chromium.org> wrote:
> 
> > On 2014/06/06 02:38:08, trchen wrote:
> >
> >> On 2014/06/06 02:12:37, weiliangc wrote:
> >> > On 2014/06/05 23:24:24, trchen wrote:
> >> > > cc/animation/scrollbar_animation_controller_thinning.cc looks like
> >> what
> >>
> > you
> >
> >> > > described. What is the relationship between that implementation and
> >> the
> >>
> > one
> >
> >> > you
> >> > > uploaded here?
> >> >
> >> > This CL is part of effor to prettify overlay scrollbar on Aura.
> >> >
> >> > Currently even with force-compositing mode,on Aura platforms non
> >> main-frame
> >> > scrollbars (those belong to RenderLayerScrollableArea) are still
> >> rendered by
> >> > Blink and not promoted to cc. Blink has to understand how to paint,
> >> also how
> >> to
> >> > animate. Before being able to promote all scrollbar to cc, Blink has to
> >> know
> >> how
> >> > to animate.
> >>
> >
> >  Why is that? What happens if you enable non-main-frame impl-side
> >> scrollbars by
> >> changing the hardcoded value in
> >> ScrollingCoordinator::scrollableAreaScrollbarLayerDidChange() ?
> >>
> >
> > I'll need to check and get back to you on this.
> >
> >
> >  The reason why we didn't enable it on other platform is because the
> >> z-order
> >> issue with non-main-frame impl-side scrolling, but I believe vollick@
> >> fixed
> >>
> > that
> >
> >> long time ago. If impl-side scrollbars work for main frame on CrOS, why
> >> don't
> >> they work for non-main frames?
> >>
> >
> > I believe universal overflow scrolling still has maybe one or two blocker?
> > Very
> > soon, yes.
> 
> 
> We don't turn it on for low-dpi devices cuz promoting causes us to lose LCD
> AA, so we need to handle non-promoted scrolling still.

I forgot why impl-side scrolling would cause us to lose LCD AA. If that's the
case, why doesn't it affect the main frame as well? Was it due to CSS
background-attachment? Though I remember that one reason why we couldn't enable
universal overflow scrolling on low-end devices was due to bigger memory
footprint.

That said, actually composited scrollbars can be used for non-impl-side
scrolling layers too, with minimal modification. All we need to do is to let cc
knows the latest scroll offset, without letting cc handles scroll gestures.

> >  > Also effort is needed to let cc know how to paint prettified overlay
> >> scrollbar.
> >> > It is nice to have due to performance reason, but not going to be the
> >> first
> >> > step.
> >> >
> >> > As for the relation to cc's scrollbar_animator_contoller_thinning,
> >> this CL
> >> does
> >> > the same thing just on Blink.
> >>
> >
> >  So it looks like we already have some components in cc, then why
> >> implementing
> >> the whole thing will be easier in Blink? Is there any difficulty working
> >> on cc
> >> that I could help?
> >>
> >
> > Even with this animation handled in Blink, repaint on main thread only
> > happens
> > when pinch-zoom or animation, while general scrolling is still on impl
> > side.
> > There is no data show this will be a performance pain on CrOS, so
> > optimizing for
> > performance is not the first problem being tackled.
> >
> > To port prettified overlay scrollbar into cc (9-patch scrollbar), there
> > needs to
> > be new scrollbar layer class that gets resource from Blink. Blink rendered
> > scrollbar has more straight forward painting path(CL 183973008), as for
> > state
> > transition animation already has scrollAnimator in place with similar
> > logic. I
> > created new class because scrollAnimators shares a lot of function that is
> > not
> > used by state transition animation and I do not want to pollute
> > scrollAnimator
> > with state transition animation code. Prettify overlay scrollbar is
> > experimental
> > on design, the fastest way to push out consistent look is doing Blink side
> > code
> > first.

I'd be really surprised if you can do reliable 60 fps animation on main thread,
especially on those ARM-based devices. We've spent tremendous amount of effort
to move much of the CSS animation to the impl thread. Introducing more animation
to the main thread sounds like a big no-no to me.

Anyway, you still need to enable impl-side scrolling for the main frame don't
you? If you can't animate the 9-patch scrollbars in the impl thread, how are you
going to synchronize the scrollbars with main frame scrolling?

[jamesr, 2014-06-06 22:38:35]

On non-android platforms some scrollbars will have to be handled completely on
the main thread (they won't even have cc::ScrollbarLayers), which means Blink
will have to animate/etc them.  This is in addition to scrollbars that cc
handles.  This means we need support for animations in both systems.  There is
no way around this until we decide we don't care about LCD text on all
platforms, which isn't happening in the forseeable future.

[jamesr, 2014-06-06 22:43:37]

On 2014/06/06 21:28:42, trchen wrote:
> > We don't turn it on for low-dpi devices cuz promoting causes us to lose LCD
> > AA, so we need to handle non-promoted scrolling still.
> 
> I forgot why impl-side scrolling would cause us to lose LCD AA.

Promoting to compositing loses LCD, not (just) impl-side scrolling.

[trchen, 2014-06-07 00:25:47]

On 2014/06/06 22:38:35, jamesr wrote:
> On non-android platforms some scrollbars will have to be handled completely on
> the main thread (they won't even have cc::ScrollbarLayers), which means Blink
> will have to animate/etc them.  This is in addition to scrollbars that cc
> handles.  This means we need support for animations in both systems.  There is
> no way around this until we decide we don't care about LCD text on all
> platforms, which isn't happening in the forseeable future.

That's a bummer. If that's the case we might need 3 animation systems... Since
we actually have 3 scrollbar implementation.
1. cc:ScrollbarLayer --> The scrollbar animates itself. Suitable for impl-side
scrolling too.
2. regular composited scrollbar --> Can use the animation system provided by
this CL, but will be more efficient if we can use WebAnimation mechanism. Or
with minimum modification, it is possible to promote these to
cc::ScrollbarLayer.
3. non-composited scrollbar --> Need the animation system provided by this CL.

I'm not against this CL since it looks like we need to support number 3 anyway.
What to do with number 2 is out of scope of this code review. We can continue
the discussion, perhaps at crbug.com/307578 .

[weiliangc, 2014-06-12 19:46:48]

On 2014/06/07 00:25:47, trchen wrote:
> On 2014/06/06 22:38:35, jamesr wrote:
> > On non-android platforms some scrollbars will have to be handled completely
on
> > the main thread (they won't even have cc::ScrollbarLayers), which means
Blink
> > will have to animate/etc them.  This is in addition to scrollbars that cc
> > handles.  This means we need support for animations in both systems.  There
is
> > no way around this until we decide we don't care about LCD text on all
> > platforms, which isn't happening in the forseeable future.
> 
> That's a bummer. If that's the case we might need 3 animation systems... Since
> we actually have 3 scrollbar implementation.
> 1. cc:ScrollbarLayer --> The scrollbar animates itself. Suitable for impl-side
> scrolling too.
> 2. regular composited scrollbar --> Can use the animation system provided by
> this CL, but will be more efficient if we can use WebAnimation mechanism. Or
> with minimum modification, it is possible to promote these to
> cc::ScrollbarLayer.

Composited scrollbar will always have cc::ScrollbarLayer, it's just solid color
scrollbars are rendered in impl thread, while others needs to call into main
thread then into Blink to render itself.

Ian and I are trying to do some digging to see whether we can use CSS animation
system for these state transition animations.

> 3. non-composited scrollbar --> Need the animation system provided by this CL.
> 
> I'm not against this CL since it looks like we need to support number 3
anyway.
> What to do with number 2 is out of scope of this code review. We can continue
> the discussion, perhaps at crbug.com/307578 .

So in the future, work can be done to composited 9 patch scrollbars go from No.2
to No.1, but No.3 will always be in low dpi non-composited scrollbars.

[weiliangc, 2014-06-12 19:47:38]

Now with unit test. PTAL.

[weiliangc, 2014-06-16 21:38:08]

On 2014/06/12 19:47:38, weiliangc wrote:
> Now with unit test. PTAL.

Changed reviewer to people who are more familiar with this area of code.

Julien: scroll related
abarth@: overall

Please take a look? Thanks.

[abarth-chromium, 2014-06-16 22:47:04]

https://codereview.chromium.org/246293006/diff/160001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/246293006/diff/160001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.cpp:53: void*
stateTranstionAnimatorPointer;
The point of having this struct here is to keep the size of this struct small. 
Is this feature worth the boat to this datastructure?

[weiliangc, 2014-06-17 16:27:58]

On 2014/06/16 22:47:04, abarth wrote:
>
https://codereview.chromium.org/246293006/diff/160001/Source/platform/scroll/...
> File Source/platform/scroll/ScrollableArea.cpp (right):
> 
>
https://codereview.chromium.org/246293006/diff/160001/Source/platform/scroll/...
> Source/platform/scroll/ScrollableArea.cpp:53: void*
> stateTranstionAnimatorPointer;
> The point of having this struct here is to keep the size of this struct small.

> Is this feature worth the boat to this datastructure?

Animation to change size/opacity is part of design for overlay scrollbars.

One alternative is to collapse this state transition animator back into
scrollAnimator (what patch set 1 would look like), it will reduce size of
ScrollableArea struct, but would make ScrollAnimator contain if checks of "is
the animation a scrollAnimator or a stateTransitionAnimation?"

Or better yet, is there a better place for state transition animator to be? I
have looked at CSS animation but has not been able to come up with a concrete
plan.

Also is this just a general rule of keep everything small, or is there a
specific reason that scrollableArea has to be smaller than certain size?

[abarth-chromium, 2014-06-17 21:47:20]

On 2014/06/17 at 16:27:58, weiliangc wrote:
> Also is this just a general rule of keep everything small, or is there a
specific reason that scrollableArea has to be smaller than certain size?

We have these checks only on certain objects that we've decided in the past we
didn't want to grow accidently.

The associated bug is marked OS-Chrome.  My understanding is that OS-Android has
overlay scrollbar.  How did we manage to implement them for Android without
bloating memory in this way?

[jamesr, 2014-06-17 22:01:48]

Android doesn't have main thread driven scrollbars, so they only need animation
machinery in the compositor.  On ChromeOS and other desktop platforms we need to
have main thread driven scrollbars since promoting the scrollbars to the
compositor thread loses LCD text.  Android has no LCD text.

[abarth-chromium, 2014-06-17 23:53:27]

It seems a shame to waste memory on Android to service desktop.

[jamesr, 2014-06-18 00:02:53]

Pretty much all of the fields on WebCore::Scrollbar are waste on android.  I
don't disagree with you in principle, but you're drawing the line in a very
arbitrary place.

[abarth-chromium, 2014-06-18 00:09:18]

On 2014/06/18 at 00:02:53, jamesr wrote:
> Pretty much all of the fields on WebCore::Scrollbar are waste on android.

ScrollableArea

> I don't disagree with you in principle, but you're drawing the line in a very
arbitrary place.

Yeah, it's certainly history-dependent in the sense that the fields currently
there are getting grandfathered in...  Maybe it's not worth worrying about.

[weiliangc, 2014-06-23 21:22:54]

On 2014/06/18 00:09:18, abarth wrote:
> On 2014/06/18 at 00:02:53, jamesr wrote:
> > Pretty much all of the fields on WebCore::Scrollbar are waste on android.
> 
> ScrollableArea
> 
> > I don't disagree with you in principle, but you're drawing the line in a
very
> arbitrary place.
> 
> Yeah, it's certainly history-dependent in the sense that the fields currently
> there are getting grandfathered in...  Maybe it's not worth worrying about.

Ping?

[abarth-chromium, 2014-06-23 21:52:43]

I have no objection to this CL, but I'm not interested in reviewing it further.

https://codereview.chromium.org/246293006/diff/160001/Source/web/tests/Scroll...
File Source/web/tests/ScrollbarStateTransitionAnimatorTest.cpp (right):

https://codereview.chromium.org/246293006/diff/160001/Source/web/tests/Scroll...
Source/web/tests/ScrollbarStateTransitionAnimatorTest.cpp:29: */
Please use new-style copyright blocks for new files:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/246293006/diff/160001/Source/web/tests/Scroll...
Source/web/tests/ScrollbarStateTransitionAnimatorTest.cpp:29: */
Please use new-style copyright blocks for new files:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/246293006/diff/160001/Source/web/tests/Scroll...
Source/web/tests/ScrollbarStateTransitionAnimatorTest.cpp:39: #include
"platform/scroll/ScrollbarThemeOverlayMock.h"
If all the code you're testing is in blink_platform, this test should be part of
blink_platform_unittests.

[weiliangc, 2014-09-12 18:30:08]

Patchset #7 (id:180001) has been deleted

[weiliangc, 2014-09-16 21:54:02]

Rebased, and addressed review comments.

[weiliangc, 2014-10-01 19:07:32]

Will split into smaller patches. Starting CL 619193003

[Julien - ping for review, 2016-03-10 22:00:44]

jchaffraix@chromium.org changed reviewers:
- jchaffraix@chromium.org