cc: Support animating scroll offset

cc: Support animating scroll offset

This makes cc::LayerAnimationController support animating scroll offset.
This is needed for implementing the CSSOM View smooth scroll API.

BUG=243871
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238271

[ajuma, 2013-11-28 22:33:35]

https://codereview.chromium.org/95763002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/layers/layer.cc#newcode1077
cc/layers/layer.cc:1077: // compositor-driven scrolling.
SingleThreadProxy doesn't send back scroll deltas. We have a few options here:
1) Just make SingleThreadProxy support this.
2) Rely on the fact that the flags that trigger
ScrollableArea::usesCompositedScrolling won't be true in places where we still
ship SingleThreadProxy (and Blink will check usesCompositedScrolling before
sending scroll animations over to cc).
3) Add a flag for accelerated smooth scroll animations (which would only affect
whether these animations get sent to cc, not whether the smooth scroll API is
enabled), and make this default to false when threaded compositing is disabled.

The main downside to (1) is that it requires doing an additional tree walk (for
LayerTreeHostImpl::ProcessScrollDeltas) on each commit, to gather scroll deltas
that will virtually always be 0 in practice for SingleThreadProxy.

[ajuma, 2013-11-29 15:14:06]

One thing I forgot to mention:

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
cc/animation/scroll_offset_animation_curve.cc:39: duration_ =
std::sqrt(max_delta)/kDurationDivisor;
This calculation will almost certainly be tweaked before we ship the smooth
scroll API. For now, we just need *some* reasonable duration calculation here.

[Ian Vollick, 2013-11-29 15:29:28]

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller.h (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller.h:103: void
remove_value_provider(LayerAnimationValueProvider* provider) {
Why not just set_value_provider(NULL)?

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:594:
controller->UpdateState(true, NULL);
Should we check that we get no events here?

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:607: controller->Animate(1.0
+ duration/2.0);
This (1.0 + duration/2.0) and (1.0 + duration) stuff confuses me a bit. Why this
number? (Why the extra 1.0 +?)

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_value_provider.h (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_value_provider.h:12: 
A comment explaining the need for this would be helpful. IIRC, it's because
scroll animations (unlike the other types of animation) need to start from the
'current' value. This value needs to be provided somehow, and the
LayerAnimationValueProvider is our means of doing so. That's right, right?

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
cc/animation/scroll_offset_animation_curve.cc:39: duration_ =
std::sqrt(max_delta)/kDurationDivisor;
On 2013/11/29 15:14:06, ajuma wrote:
> This calculation will almost certainly be tweaked before we ship the smooth
> scroll API. For now, we just need *some* reasonable duration calculation here.

Please add a comment here explaining that this math is just a tweak to get the
right feel (and might need to be updated). At first I was wondering if there was
a geometrical reason for the sqrt..

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
File cc/animation/scroll_offset_animation_curve_unittest.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
cc/animation/scroll_offset_animation_curve_unittest.cc:19:
EaseInOutTimingFunction::Create().Pass()));
Won't this need to be updated each time we tweak the formula for computing
duration?

https://codereview.chromium.org/95763002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/layers/layer.cc#newcode1077
cc/layers/layer.cc:1077: // compositor-driven scrolling.
On 2013/11/28 22:33:36, ajuma wrote:
> SingleThreadProxy doesn't send back scroll deltas. We have a few options here:
> 1) Just make SingleThreadProxy support this.
> 2) Rely on the fact that the flags that trigger
> ScrollableArea::usesCompositedScrolling won't be true in places where we still
> ship SingleThreadProxy (and Blink will check usesCompositedScrolling before
> sending scroll animations over to cc).
> 3) Add a flag for accelerated smooth scroll animations (which would only
affect
> whether these animations get sent to cc, not whether the smooth scroll API is
> enabled), and make this default to false when threaded compositing is
disabled.
> 
> The main downside to (1) is that it requires doing an additional tree walk
(for
> LayerTreeHostImpl::ProcessScrollDeltas) on each commit, to gather scroll
deltas
> that will virtually always be 0 in practice for SingleThreadProxy.

Re 2), we could use scroll to on the main frame, too, and that will never have
usesCompositedScrolling, even if its on the fast path.

As we chatted about offline, I think my favorite option is to detect when we're
in single threaded mode, and never invoke the cc scroll animation machinery in
that case (and instead fall back to the main thread machinery that we'll need to
create anyhow). And if we go with this approach, this method can stay as-is,
doing nothing.

[ajuma, 2013-11-29 21:18:41]

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller.h (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller.h:103: void
remove_value_provider(LayerAnimationValueProvider* provider) {
On 2013/11/29 15:29:29, Ian Vollick wrote:
> Why not just set_value_provider(NULL)?

The issue is that a LayerImpl might not know whether it's the value provider for
its LayerAnimationController. Only active layers act as value providers.
LayerImpl checks IsActive before setting itself up as a value provider. Doing
the same thing for de-registering seems more fragile, since IsActive works by
checking if the LayerImpl's tree is the same as LayerTreeHostImpl's active_tree.
If, in the future, we allowed a LayerTreeHostImpl to throw away its active tree
and create a  new one, and if the old active tree's destruction sequence
happened after LayerTreeHostImpl's active tree pointer had been reset, relying
on the IsActive check when destroying would lead to incorrect behavior here.

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:594:
controller->UpdateState(true, NULL);
On 2013/11/29 15:29:29, Ian Vollick wrote:
> Should we check that we get no events here?

Note that we're passing NULL as the events argument (as we should for
|controller|, which is representing the main thread controller), so there isn't
any events vector to check here.

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:607: controller->Animate(1.0
+ duration/2.0);
On 2013/11/29 15:29:29, Ian Vollick wrote:
> This (1.0 + duration/2.0) and (1.0 + duration) stuff confuses me a bit. Why
this
> number? (Why the extra 1.0 +?)

The issue is that LayerAnimationController::PromoteStartedAnimations treats 0 as
meaning that the animation's start time hasn't been set (see
Animation::has_set_start_time). This is problematic for tests that use
NotifyAnimationStarted with an event whose monotonic_time is 0. Looking through
the other tests in this file, in some places we're doing the right think (using
times like 1.0), but there are at least a few places where we're using 0.0
despite also using NotifyAnimationStarted.

I think the right solution here is to DCHECK(monotonic_time != 0.0) in
LayerAnimationController::Animate, and (of course) to fix all the unit tests
that animate at 0.0. Since this is going to be a large-ish diff (since it will
affect all tests that tick at time 0, not just those that call
NotifyAnimationStarted), I'm not sure that it belongs here, but I'm happy to do
that here if you like or as a separate CL.

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_value_provider.h (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_value_provider.h:12: 
On 2013/11/29 15:29:29, Ian Vollick wrote:
> A comment explaining the need for this would be helpful. IIRC, it's because
> scroll animations (unlike the other types of animation) need to start from the
> 'current' value. This value needs to be provided somehow, and the
> LayerAnimationValueProvider is our means of doing so. That's right, right?

Comment added. Note that I've avoided mentioning "scroll" specifically in the
comment, since it's reasonable to do the same thing for other properties (e.g.
this approach gives a way to interrupt CSS transitions without jitter).

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
cc/animation/scroll_offset_animation_curve.cc:39: duration_ =
std::sqrt(max_delta)/kDurationDivisor;
On 2013/11/29 15:29:29, Ian Vollick wrote:
> On 2013/11/29 15:14:06, ajuma wrote:
> > This calculation will almost certainly be tweaked before we ship the smooth
> > scroll API. For now, we just need *some* reasonable duration calculation
here.
> 
> Please add a comment here explaining that this math is just a tweak to get the
> right feel (and might need to be updated). At first I was wondering if there
was
> a geometrical reason for the sqrt..

Done. (The reason for the sqrt is just so that the duration grows more slowly
than the size.)

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
File cc/animation/scroll_offset_animation_curve_unittest.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/scroll_offset_an...
cc/animation/scroll_offset_animation_curve_unittest.cc:19:
EaseInOutTimingFunction::Create().Pass()));
On 2013/11/29 15:29:29, Ian Vollick wrote:
> Won't this need to be updated each time we tweak the formula for computing
> duration?

Yes, but I think it's still worth having, since there are a couple useful things
this test does: 1) It verifies that the duration does depend on both the
horizontal and vertical deltas. 2) It gives a rough idea of how the size of a
scroll translates into a duration (e.g. that tiny scrolls are getting tiny
durations, and larger scrolls are getting larger durations but not ridiculously
larger durations).

This shouldn't be too much of a burden to maintain; once the smooth scroll API
ships, it's unlikely that we'll change this very much (if at all).

https://codereview.chromium.org/95763002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/layers/layer.cc#newcode1077
cc/layers/layer.cc:1077: // compositor-driven scrolling.
On 2013/11/29 15:29:29, Ian Vollick wrote:
> On 2013/11/28 22:33:36, ajuma wrote:
> > SingleThreadProxy doesn't send back scroll deltas. We have a few options
here:
> > 1) Just make SingleThreadProxy support this.
> > 2) Rely on the fact that the flags that trigger
> > ScrollableArea::usesCompositedScrolling won't be true in places where we
still
> > ship SingleThreadProxy (and Blink will check usesCompositedScrolling before
> > sending scroll animations over to cc).
> > 3) Add a flag for accelerated smooth scroll animations (which would only
> affect
> > whether these animations get sent to cc, not whether the smooth scroll API
is
> > enabled), and make this default to false when threaded compositing is
> disabled.
> > 
> > The main downside to (1) is that it requires doing an additional tree walk
> (for
> > LayerTreeHostImpl::ProcessScrollDeltas) on each commit, to gather scroll
> deltas
> > that will virtually always be 0 in practice for SingleThreadProxy.
> 
> Re 2), we could use scroll to on the main frame, too, and that will never have
> usesCompositedScrolling, even if its on the fast path.

Ah, I missed that usesCompositedScrolling doesn't get set in this case.
 
> As we chatted about offline, I think my favorite option is to detect when
we're
> in single threaded mode, and never invoke the cc scroll animation machinery in
> that case (and instead fall back to the main thread machinery that we'll need
to
> create anyhow). And if we go with this approach, this method can stay as-is,
> doing nothing.

Agreed. Note that this is effectively option (3), since (as far as I can tell,
looking through Source/core/page/Settings.in) Blink doesn't "know" when we're in
single-threaded mode (which is something I didn't realize when we discussed this
offline).

[ajuma, 2013-12-02 21:16:11]

PTAL.

[Ian Vollick, 2013-12-02 21:23:31]

lgtm

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller.h (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller.h:103: void
remove_value_provider(LayerAnimationValueProvider* provider) {
On 2013/11/29 21:18:41, ajuma wrote:
> On 2013/11/29 15:29:29, Ian Vollick wrote:
> > Why not just set_value_provider(NULL)?
> 
> The issue is that a LayerImpl might not know whether it's the value provider
for
> its LayerAnimationController. Only active layers act as value providers.
> LayerImpl checks IsActive before setting itself up as a value provider. Doing
> the same thing for de-registering seems more fragile, since IsActive works by
> checking if the LayerImpl's tree is the same as LayerTreeHostImpl's
active_tree.
> If, in the future, we allowed a LayerTreeHostImpl to throw away its active
tree
> and create a  new one, and if the old active tree's destruction sequence
> happened after LayerTreeHostImpl's active tree pointer had been reset, relying
> on the IsActive check when destroying would lead to incorrect behavior here.
Thanks for the explanation.

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:594:
controller->UpdateState(true, NULL);
On 2013/11/29 21:18:41, ajuma wrote:
> On 2013/11/29 15:29:29, Ian Vollick wrote:
> > Should we check that we get no events here?
> 
> Note that we're passing NULL as the events argument (as we should for
> |controller|, which is representing the main thread controller), so there
isn't
> any events vector to check here.

Ah, right -- main thread! Sorry.

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:607: controller->Animate(1.0
+ duration/2.0);
On 2013/11/29 21:18:41, ajuma wrote:
> On 2013/11/29 15:29:29, Ian Vollick wrote:
> > This (1.0 + duration/2.0) and (1.0 + duration) stuff confuses me a bit. Why
> this
> > number? (Why the extra 1.0 +?)
> 
> The issue is that LayerAnimationController::PromoteStartedAnimations treats 0
as
> meaning that the animation's start time hasn't been set (see
> Animation::has_set_start_time). This is problematic for tests that use
> NotifyAnimationStarted with an event whose monotonic_time is 0. Looking
through
> the other tests in this file, in some places we're doing the right think
(using
> times like 1.0), but there are at least a few places where we're using 0.0
> despite also using NotifyAnimationStarted.
> 
> I think the right solution here is to DCHECK(monotonic_time != 0.0) in
> LayerAnimationController::Animate, and (of course) to fix all the unit tests
> that animate at 0.0. Since this is going to be a large-ish diff (since it will
> affect all tests that tick at time 0, not just those that call
> NotifyAnimationStarted), I'm not sure that it belongs here, but I'm happy to
do
> that here if you like or as a separate CL. 

I think this is fine for this CL, but yes, it would be nice to either clean this
up, or move the creation of times for tests into a common helper to hide the
1.0+ ugliness away with a nice comment.

[ajuma, 2013-12-02 21:48:56]

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/95763002/diff/1/cc/animation/layer_animation_...
cc/animation/layer_animation_controller_unittest.cc:607: controller->Animate(1.0
+ duration/2.0);
On 2013/12/02 21:23:32, Ian Vollick wrote:
> On 2013/11/29 21:18:41, ajuma wrote:
> > On 2013/11/29 15:29:29, Ian Vollick wrote:
> > > This (1.0 + duration/2.0) and (1.0 + duration) stuff confuses me a bit.
Why
> > this
> > > number? (Why the extra 1.0 +?)
> > 
> > The issue is that LayerAnimationController::PromoteStartedAnimations treats
0
> as
> > meaning that the animation's start time hasn't been set (see
> > Animation::has_set_start_time). This is problematic for tests that use
> > NotifyAnimationStarted with an event whose monotonic_time is 0. Looking
> through
> > the other tests in this file, in some places we're doing the right think
> (using
> > times like 1.0), but there are at least a few places where we're using 0.0
> > despite also using NotifyAnimationStarted.
> > 
> > I think the right solution here is to DCHECK(monotonic_time != 0.0) in
> > LayerAnimationController::Animate, and (of course) to fix all the unit tests
> > that animate at 0.0. Since this is going to be a large-ish diff (since it
will
> > affect all tests that tick at time 0, not just those that call
> > NotifyAnimationStarted), I'm not sure that it belongs here, but I'm happy to
> do
> > that here if you like or as a separate CL. 
> 
> I think this is fine for this CL, but yes, it would be nice to either clean
this
> up, or move the creation of times for tests into a common helper to hide the
> 1.0+ ugliness away with a nice comment.

Sgtm. For this CL, I've replaced the "1.0+" with "start_time+".

[commit-bot: I haz the power, 2013-12-02 21:59:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/95763002/50001

[commit-bot: I haz the power, 2013-12-03 02:20:51]

Change committed as 238271