Impl thread smooth scrolling.

Impl thread smooth scrolling.

This is a first pass at smooth scrolling on the compositor thread.  It adds LayerTreeHostImpl::ScrollAnimated, which kicks off a scroll offset animation through the layer animation controller.

When the --enable-smooth-scrolling flag is present, InputHandlerProxy will call ScrollAnimated for composited wheel scrolls.

Once this is in the next steps are:

(1) Update the animation curve to aim at a new target when a wheel event comes in while a scroll animation is in progress.  (For now we just drop the event.)

(2) Unify the behavior with WebCore::ScrollAnimatorNone.  We should be able to share a lot of logic since cc::ScrollOffsetAnimationCurve is already exposed to Blink through compositor bindings (for http://crbug.com/243871).

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

[skobes, 2014-07-07 22:10:06]

@jamesr, the idea we discussed of setting the cc::LayerImpl as the animation
delegate did not work because NotifyAnimationFinished is only called on the main
thread's animation controller.  So I am just checking in
LayerTreeHostImpl::DidAnimateScrollOffset (called every tick) whether we have
reached the scroll offset we were animating toward, which might be brittle.

Let me know if you can think of a better way to know on the impl thread when the
animation is finished.

[skobes, 2014-07-08 00:48:06]

On 2014/07/07 22:10:06, skobes wrote:
> @jamesr, the idea we discussed of setting the cc::LayerImpl as the animation
> delegate did not work because NotifyAnimationFinished is only called on the
main
> thread's animation controller.  So I am just checking in
> LayerTreeHostImpl::DidAnimateScrollOffset (called every tick) whether we have
> reached the scroll offset we were animating toward, which might be brittle.

I've updated the patch with a slightly better way of detecting the end of the
animation, by hooking into LayerTreeHostImpl::UpdateAnimationState.

[ajuma, 2014-07-08 14:44:49]

A few questions and comments. Please also add some unit tests for this.

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2289: std::abs(actual_delta.y()) > kEpsilon);
If actual_delta is less than kEpsilon but non-zero, shouldn't the layer still be
scrolled by actual_delta?

ScrollBy has logic for deciding whether scrolls should bubble. Do we need that
logic here too? Can it be shared?

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2945: ScrollEnd();
I think a better approach would be to modify LayerAnimationController so that
for impl-only animations, we directly call NotifyAnimationStarted (in
::PromoteStartedAnimations) and NotifyAnimationFinished (in
::MarkAnimationsForDeletion) rather then enqueuing events which get sent to the
main thread. (These events currently get sent to the main thread because a
previous consumer of impl-only animations expected to get notified on the main
thread, but this is no longer used.) Then you can set LayerImpl as the animation
delegate as you initially planned.

[skobes, 2014-07-08 17:49:13]

Thanks for the review.

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2289: std::abs(actual_delta.y()) > kEpsilon);
On 2014/07/08 14:44:49, ajuma wrote:
> If actual_delta is less than kEpsilon but non-zero, shouldn't the layer still
be
> scrolled by actual_delta?

I based this on the logic in ScrollBy, I think the use of kEpsilon is to deal
with rounding error from floating point math.  I assume scroll offsets are
rounded to pixel boundary somewhere?

> ScrollBy has logic for deciding whether scrolls should bubble. Do we need that
> logic here too? Can it be shared?

I think it is not needed here since should_bubble_scrolls_ is always true for
wheel scrolls.  When the ScrollInputType is NonBubblingGesture we do not go
through ScrollAnimated.

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2945: ScrollEnd();
On 2014/07/08 14:44:49, ajuma wrote:
> I think a better approach would be to modify LayerAnimationController so that
> for impl-only animations, we directly call NotifyAnimationStarted (in
> ::PromoteStartedAnimations) and NotifyAnimationFinished (in
> ::MarkAnimationsForDeletion) rather then enqueuing events which get sent to
the
> main thread. (These events currently get sent to the main thread because a
> previous consumer of impl-only animations expected to get notified on the main
> thread, but this is no longer used.) Then you can set LayerImpl as the
animation
> delegate as you initially planned.

Done.

[ajuma, 2014-07-08 18:45:32]

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2289: std::abs(actual_delta.y()) > kEpsilon);
On 2014/07/08 17:49:12, skobes wrote:
> On 2014/07/08 14:44:49, ajuma wrote:
> > If actual_delta is less than kEpsilon but non-zero, shouldn't the layer
still
> be
> > scrolled by actual_delta?
> 
> I based this on the logic in ScrollBy, I think the use of kEpsilon is to deal
> with rounding error from floating point math.  I assume scroll offsets are
> rounded to pixel boundary somewhere?

It looks like ScrollBy actually performs the scroll (calling
ScrollLayerWithLocalDelta) before considering whether the amount scrolled was
greater than kEpsilon. You're right that it might not change what gets
displayed, but it's still going to be a difference in the value of the layer's
scroll offset.

> > ScrollBy has logic for deciding whether scrolls should bubble. Do we need
that
> > logic here too? Can it be shared?
> 
> I think it is not needed here since should_bubble_scrolls_ is always true for
> wheel scrolls.  When the ScrollInputType is NonBubblingGesture we do not go
> through ScrollAnimated.

There's also the logic for bubbling scrolls to show the top controls. Presumably
that's not so important here because wheel scrolling is going to be pretty
uncommon on Android, but it probably still warrants a comment and/or a TODO.

https://codereview.chromium.org/361143002/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (left):

https://codereview.chromium.org/361143002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:890: // being sent to the
main thread controller's animation delegate.
Instead of deleting this altogether, this should be modified to test that the
impl thread controller's animation delegate is getting start and finish
notifications.

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:80:
layer_animation_controller_->set_layer_animation_delegate(this);
Like the previous line, this needs to happen only for active layers (or else
active and pending layers will compete for becoming the animation delegate,
since active and pending layers share a single animation controller).

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:88:
layer_animation_controller_->remove_value_provider(this);
The animation delegate needs to be cleared here (if the layer IsActive()).

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:110: Animation::TargetProperty target_property)
OVERRIDE{};
Missing space after OVERRIDE? (git cl format should take care of this)

https://codereview.chromium.org/361143002/diff/120001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/361143002/diff/120001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:92: void ScrollOffsetAnimationFinished();
Since this is only going to get called for input-initiated scroll animations
rather than all scroll animations, something like 'InputScrollAnimationFinished'
might be a better name.

[skobes, 2014-07-08 21:21:58]

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2289: std::abs(actual_delta.y()) > kEpsilon);
On 2014/07/08 18:45:32, ajuma wrote:
> It looks like ScrollBy actually performs the scroll (calling
> ScrollLayerWithLocalDelta) before considering whether the amount scrolled was
> greater than kEpsilon. You're right that it might not change what gets
> displayed, but it's still going to be a difference in the value of the layer's
> scroll offset.

Fair point, done.

> There's also the logic for bubbling scrolls to show the top controls.
Presumably
> that's not so important here because wheel scrolling is going to be pretty
> uncommon on Android, but it probably still warrants a comment and/or a TODO.

Added a comment.  This codepath shouldn't apply to Android.

https://codereview.chromium.org/361143002/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (left):

https://codereview.chromium.org/361143002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:890: // being sent to the
main thread controller's animation delegate.
On 2014/07/08 18:45:32, ajuma wrote:
> Instead of deleting this altogether, this should be modified to test that the
> impl thread controller's animation delegate is getting start and finish
> notifications.

Done.

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:80:
layer_animation_controller_->set_layer_animation_delegate(this);
On 2014/07/08 18:45:32, ajuma wrote:
> Like the previous line, this needs to happen only for active layers (or else
> active and pending layers will compete for becoming the animation delegate,
> since active and pending layers share a single animation controller).

Done.

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:88:
layer_animation_controller_->remove_value_provider(this);
On 2014/07/08 18:45:32, ajuma wrote:
> The animation delegate needs to be cleared here (if the layer IsActive()).

Done.

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:110: Animation::TargetProperty target_property)
OVERRIDE{};
On 2014/07/08 18:45:32, ajuma wrote:
> Missing space after OVERRIDE? (git cl format should take care of this)

This is how git cl format wanted it.  I thought it was ugly too. :)

https://codereview.chromium.org/361143002/diff/120001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/361143002/diff/120001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:92: void ScrollOffsetAnimationFinished();
On 2014/07/08 18:45:32, ajuma wrote:
> Since this is only going to get called for input-initiated scroll animations
> rather than all scroll animations, something like
'InputScrollAnimationFinished'
> might be a better name.

Done.

[jdduke (slow), 2014-07-08 21:26:20]

https://codereview.chromium.org/361143002/diff/140001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/361143002/diff/140001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:162: smooth_scroll_enabled_ =
CommandLine::ForCurrentProcess()->HasSwitch(

Apologies for the drive-by, but why do we need a flag here? Don't we want this
behavior on all platforms that support impl thread event handling (Android
included)?

[jdduke (slow), 2014-07-08 21:28:34]

https://codereview.chromium.org/361143002/diff/140001/content/renderer/input/...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/361143002/diff/140001/content/renderer/input/...
content/renderer/input/input_handler_proxy.cc:162: smooth_scroll_enabled_ =
CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/07/08 21:26:20, jdduke wrote:
> 
> Apologies for the drive-by, but why do we need a flag here? Don't we want this
> behavior on all platforms that support impl thread event handling (Android
> included)?

Hmm, disregard that, it looks like you're re-using an existing flag (although
I'm curious why smooth scrolling isn't enabled by default?).

[skobes, 2014-07-08 21:33:34]

On 2014/07/08 21:26:20, jdduke wrote:
> Apologies for the drive-by, but why do we need a flag here? Don't we want this
> behavior on all platforms that support impl thread event handling (Android
> included)?

Fling scroll animations on Android and CrOS are handled differently, the
animation for that is driven by InputHandlerProxy.  The
--enable-smooth-scrolling flag affects wheel scrolls and scrolls generated by
keyboard arrows or clicking the scrollbar steppers.  Of those, only the wheel
scrolls can currently be handled on the impl thread, the others are handled in
Blink.

[skobes, 2014-07-08 21:35:14]

On 2014/07/08 21:28:34, jdduke wrote:
> Hmm, disregard that, it looks like you're re-using an existing flag (although
> I'm curious why smooth scrolling isn't enabled by default?).

It's not enabled by default because it doesn't work with impl-thread scrolls. 
That's what I'm trying to fix. :)

[ajuma, 2014-07-08 21:51:35]

On 2014/07/08 21:21:58, skobes wrote:
>
https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/361143002/diff/80001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:2289: std::abs(actual_delta.y()) > kEpsilon);
> On 2014/07/08 18:45:32, ajuma wrote:
> > It looks like ScrollBy actually performs the scroll (calling
> > ScrollLayerWithLocalDelta) before considering whether the amount scrolled
was
> > greater than kEpsilon. You're right that it might not change what gets
> > displayed, but it's still going to be a difference in the value of the
layer's
> > scroll offset.
> 
> Fair point, done.
> 
> > There's also the logic for bubbling scrolls to show the top controls.
> Presumably
> > that's not so important here because wheel scrolling is going to be pretty
> > uncommon on Android, but it probably still warrants a comment and/or a TODO.
> 
> Added a comment.  This codepath shouldn't apply to Android.
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/animation/layer_anim...
> File cc/animation/layer_animation_controller_unittest.cc (left):
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/animation/layer_anim...
> cc/animation/layer_animation_controller_unittest.cc:890: // being sent to the
> main thread controller's animation delegate.
> On 2014/07/08 18:45:32, ajuma wrote:
> > Instead of deleting this altogether, this should be modified to test that
the
> > impl thread controller's animation delegate is getting start and finish
> > notifications.
> 
> Done.
> 
> https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc...
> cc/layers/layer_impl.cc:80:
> layer_animation_controller_->set_layer_animation_delegate(this);
> On 2014/07/08 18:45:32, ajuma wrote:
> > Like the previous line, this needs to happen only for active layers (or else
> > active and pending layers will compete for becoming the animation delegate,
> > since active and pending layers share a single animation controller).
> 
> Done.
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.cc...
> cc/layers/layer_impl.cc:88:
> layer_animation_controller_->remove_value_provider(this);
> On 2014/07/08 18:45:32, ajuma wrote:
> > The animation delegate needs to be cleared here (if the layer IsActive()).
> 
> Done.
> 
> https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.h
> File cc/layers/layer_impl.h (right):
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/layers/layer_impl.h#...
> cc/layers/layer_impl.h:110: Animation::TargetProperty target_property)
> OVERRIDE{};
> On 2014/07/08 18:45:32, ajuma wrote:
> > Missing space after OVERRIDE? (git cl format should take care of this)
> 
> This is how git cl format wanted it.  I thought it was ugly too. :)
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/trees/layer_tree_impl.h
> File cc/trees/layer_tree_impl.h (right):
> 
>
https://codereview.chromium.org/361143002/diff/120001/cc/trees/layer_tree_imp...
> cc/trees/layer_tree_impl.h:92: void ScrollOffsetAnimationFinished();
> On 2014/07/08 18:45:32, ajuma wrote:
> > Since this is only going to get called for input-initiated scroll animations
> > rather than all scroll animations, something like
> 'InputScrollAnimationFinished'
> > might be a better name.
> 
> Done.

Thanks, cc/ lgtm.

[skobes, 2014-07-08 21:52:19]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-07-08 21:55:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/361143002/140001

[ajuma, 2014-07-08 21:59:22]

On 2014/07/08 21:52:19, skobes wrote:
> The CQ bit was checked by mailto:skobes@chromium.org

This still needs OWNERS review for the files not in cc/.

[ajuma, 2014-07-08 21:59:31]

The CQ bit was unchecked by ajuma@chromium.org

[skobes, 2014-07-08 22:09:05]

+jochen for content/OWNERS

[jdduke (slow), 2014-07-08 22:12:36]

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2274: const gfx::Point& viewport_point,
Was there a particular reason for adding a new animated API rather than, say,
hosting the easing curve in InputHandlerProxy and having it use |ScrollBy| to
achieve the same affect?

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2277: // TODO(skobes): Update the target of the
existing animation.
Do you have specific plans on how to achieve this? It's a bit of a shame that
our fling animation code and our smooth scroll animation code will be so
divergent.

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2296: 
Is it intentional that the (potential) excess delta from the animation is
dropped (if the layer hits its scrollable bounds in the course of the animated
scroll)?

[skobes, 2014-07-08 22:22:50]

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2274: const gfx::Point& viewport_point,
On 2014/07/08 22:12:35, jdduke wrote:
> Was there a particular reason for adding a new animated API rather than, say,
> hosting the easing curve in InputHandlerProxy and having it use |ScrollBy| to
> achieve the same affect?

I wanted to reuse cc::ScrollOffsetAnimationCurve, which is also exposed to
Blink.  That way the same curve can be used in main-thread scrolls.  Also jamesr
told me that the fling scrolls were only done in InputHandlerProxy because
LayerAnimationController didn't exist at the time.

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2277: // TODO(skobes): Update the target of the
existing animation.
On 2014/07/08 22:12:35, jdduke wrote:
> Do you have specific plans on how to achieve this?

My plan was to grab the existing animation from the animation controller and
tell it to aim somewhere else.  The curve would then update its parameters to be
a new Bezier curve starting at the current scroll offset, with the first control
point chosen to match the current velocity.

https://codereview.chromium.org/361143002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2296: 
On 2014/07/08 22:12:35, jdduke wrote:
> Is it intentional that the (potential) excess delta from the animation is
> dropped (if the layer hits its scrollable bounds in the course of the animated
> scroll)?

Yes, this code does not attempt to support bubbling mid-animation.

[jochen (gone - plz use gerrit), 2014-07-09 09:18:54]

content lgtm

the bug number in the cl description looks odd?

[skobes, 2014-07-09 15:42:36]

Thanks

On 2014/07/09 09:18:54, jochen wrote:
> the bug number in the cl description looks odd?

It's correct, this is a very old bug.

[skobes, 2014-07-09 15:42:43]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-07-09 15:43:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/361143002/180001

[commit-bot: I haz the power, 2014-07-09 19:25:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-09 21:05:41]

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

[commit-bot: I haz the power, 2014-07-09 21:05:42]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[skobes, 2014-07-09 21:12:36]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:18:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skobes@chromium.org/361143002/190001

[commit-bot: I haz the power, 2014-07-10 00:02:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-10 01:06:41]

Change committed as 282205