CC Animations: Port Impl-only-scrolling to use compositor animation timelines.

CC Animations: Port Impl-only-scrolling to use compositor animation timelines.

Use AnimationPlayer to add Scrolloffset animations if new system enabled.

Previous episode: https://codereview.chromium.org/1010663002/
Next episode:
https://codereview.chromium.org/1012583002/

A step towards AnimationPlayer/AnimationTimeline decoupling.

BUG=394777
R=ajuma@chromium.org
R=vollick@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[ajuma, 2015-03-24 13:37:23]

https://codereview.chromium.org/1009233002/diff/40001/cc/animation/animation_...
File cc/animation/animation_timeline.cc (right):

https://codereview.chromium.org/1009233002/diff/40001/cc/animation/animation_...
cc/animation/animation_timeline.cc:350: animation->set_is_impl_only(true);
Not all scroll animations are impl-only, so this method should be renamed to
something like ImplOnlyScrollAnimationCreate.

[loyso (OOO), 2015-04-13 07:38:24]

https://codereview.chromium.org/1009233002/diff/40001/cc/animation/animation_...
File cc/animation/animation_timeline.cc (right):

https://codereview.chromium.org/1009233002/diff/40001/cc/animation/animation_...
cc/animation/animation_timeline.cc:350: animation->set_is_impl_only(true);
On 2015/03/24 13:37:23, ajuma wrote:
> Not all scroll animations are impl-only, so this method should be renamed to
> something like ImplOnlyScrollAnimationCreate.

Done.

[loyso (OOO), 2015-07-06 01:36:22]

PTAL! I would like to land this CL.
This is a port for --enable-smooth-scrolling linux feature.
I could implement ScrollOffsetAnimations in a separate file, though.

[ajuma, 2015-07-06 17:40:28]

ajuma@chromium.org changed reviewers:
+ skobes@chromium.org

[ajuma, 2015-07-06 17:41:00]

+skobes as fyi

https://codereview.chromium.org/1009233002/diff/160002/cc/animation/animation...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1009233002/diff/160002/cc/animation/animation...
cc/animation/animation_host.cc:114: scoped_refptr<AnimationPlayer>
scroll_offset_animation_player_;
If I'm understanding, having a single such player for impl-only scroll offset
animations means only one layer can have an impl-only scroll offset animation at
any given time (which matches current behavior). Please add a comment about this
somewhere.

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (right):

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:757: 
Thanks for porting over these tests! These don't use impl-only scroll animations
though. For that, see the ScrollAnimated test in
layer_tree_host_impl_unittest.cc. Would it be possible to make a version of that
test using animation timelines? One approach would be to make a copy of the
existing test (putting the new version in layer_tree_host_impl_unittest.cc too),
create a LayerTreeSettings object, set use_compositor_animation_timelines to
true, and then create a LayerTreeHostImpl using this settings object. (There are
several examples of using non-default LayerTreeSettings this way in
layer_tree_host_impl_unittest.cc).

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/mutator_host_...
File cc/trees/mutator_host_client.h (right):

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/mutator_host_...
cc/trees/mutator_host_client.h:41: virtual void ScrollOffsetAnimationFinished()
{}
This should be pure virtual like the other methods here.

[loyso (OOO), 2015-07-07 04:14:38]

https://codereview.chromium.org/1009233002/diff/160002/cc/animation/animation...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1009233002/diff/160002/cc/animation/animation...
cc/animation/animation_host.cc:114: scoped_refptr<AnimationPlayer>
scroll_offset_animation_player_;
On 2015/07/06 17:41:00, ajuma wrote:
> If I'm understanding, having a single such player for impl-only scroll offset
> animations means only one layer can have an impl-only scroll offset animation
at
> any given time (which matches current behavior). Please add a comment about
this
> somewhere.

Done.

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (right):

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:757: 
On 2015/07/06 17:41:00, ajuma wrote:
> Thanks for porting over these tests! These don't use impl-only scroll
animations
> though. For that, see the ScrollAnimated test in
> layer_tree_host_impl_unittest.cc. Would it be possible to make a version of
that
> test using animation timelines? One approach would be to make a copy of the
> existing test (putting the new version in layer_tree_host_impl_unittest.cc
too),
> create a LayerTreeSettings object, set use_compositor_animation_timelines to
> true, and then create a LayerTreeHostImpl using this settings object. (There
are
> several examples of using non-default LayerTreeSettings this way in
> layer_tree_host_impl_unittest.cc).

Done.

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/mutator_host_...
File cc/trees/mutator_host_client.h (right):

https://codereview.chromium.org/1009233002/diff/160002/cc/trees/mutator_host_...
cc/trees/mutator_host_client.h:41: virtual void ScrollOffsetAnimationFinished()
{}
On 2015/07/06 17:41:00, ajuma wrote:
> This should be pure virtual like the other methods here.

Done.

https://codereview.chromium.org/1009233002/diff/190001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1009233002/diff/190001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:7661:
SetupScrollAndContentsLayers(gfx::Size(100, 200));
This is a copy-paste of LayerTreeHostImplTest.ScrollAnimated except different
test fixture (LayerTreeHostImplTimelinesTest). :(

[loyso (OOO), 2015-07-07 04:59:59]

AnimationHost::thread_instance_ was deleted as unused here:
https://codereview.chromium.org/1222243005

[ajuma, 2015-07-07 13:07:01]

Thanks, lgtm.

[Ian Vollick, 2015-07-08 00:42:26]

lgtm, too, with some nits.

https://codereview.chromium.org/1009233002/diff/210001/cc/animation/animation...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/1009233002/diff/210001/cc/animation/animation...
cc/animation/animation_player.cc:122: DCHECK(animation->target_property() !=
Animation::SCROLL_OFFSET ||
nit: DCHECK_IMPLIES is, I think, what you want here.

https://codereview.chromium.org/1009233002/diff/210001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1009233002/diff/210001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3443: if (animation_host_)
An early return in a void function is weird. Was this intended?

[Ian Vollick, 2015-07-08 00:46:14]

https://codereview.chromium.org/1009233002/diff/210001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1009233002/diff/210001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3443: if (animation_host_)
On 2015/07/08 00:42:25, vollick wrote:
> An early return in a void function is weird. Was this intended?

(Err, returning the result of ImplOnlyScrollAnimationCreate, I mean. :) )

[loyso (OOO), 2015-07-08 01:13:45]

https://codereview.chromium.org/1009233002/diff/210001/cc/animation/animation...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/1009233002/diff/210001/cc/animation/animation...
cc/animation/animation_player.cc:122: DCHECK(animation->target_property() !=
Animation::SCROLL_OFFSET ||
On 2015/07/08 00:42:25, vollick wrote:
> nit: DCHECK_IMPLIES is, I think, what you want here.

Done.

https://codereview.chromium.org/1009233002/diff/210001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1009233002/diff/210001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3443: if (animation_host_)
On 2015/07/08 00:46:14, vollick wrote:
> On 2015/07/08 00:42:25, vollick wrote:
> > An early return in a void function is weird. Was this intended?
> 
> (Err, returning the result of ImplOnlyScrollAnimationCreate, I mean. :) )
Yes, this is intended. Combined it into just one statement.

[loyso (OOO), 2015-07-08 01:15:38]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-07-08 01:15:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1009233002/#ps230001
(title: "Fix the nit.")

[commit-bot: I haz the power, 2015-07-08 01:19:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1009233002/230001

[commit-bot: I haz the power, 2015-07-08 02:40:53]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-07-08 02:41:54]

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