CC Animations: Redirect all compositor animation requests to AnimationHost.

CC Animations: Redirect all compositor animation requests to AnimationHost.

1) Use AnimationHost's AnimationRegistrar instance if new system enabled.
2) Use AnimationPlayer's LayerAnimationController instance if new system enabled.

Previous episode: https://codereview.chromium.org/947033002/
Next episode:
https://codereview.chromium.org/1009233002/

In general: The compositor should not need to worry about servicing animations.
A step towards AnimationHost/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/d71ece8b808e33c65ff26e898bf4868f448b6bfc
Cr-Commit-Position: refs/heads/master@{#337309}

[ajuma, 2015-03-24 13:27:51]

I'd like to strongly reiterate a suggestion I made on a previous CL about
porting/adding tests as you write these CLs. This will help you sanity-check
your work and fix bugs, and will make these CLs easier to review.

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

loyso@chromium.org changed reviewers:
+ piman@chromium.org

[ajuma, 2015-04-21 17:57:57]

Thanks for porting unit tests! Here are a few comments from a first pass through
this CL.

https://codereview.chromium.org/1010663002/diff/140001/cc/animation/animation...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/animation/animation...
cc/animation/animation_host.cc:163: bool
AnimationHost::GetSupportsScrollAnimations() const {
Nit: s/GetSupportsScrollAnimations/SupportsScrollAnimations

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1026:
layer_animation_controller_->scroll_offset_animation_was_interrupted())
This will need an equivalent in the new system.

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1274: if (layer_animation_controller_)
This will always be true if we're constructing a controller in the constructor,
even when user_compositor_animation_timelines is true. (More generally, all the
code that's conditioned on layer_animation_controller_ is going to be taking the
branch corresponding to the old system.)

[loyso (OOO), 2015-04-22 01:08:11]

https://codereview.chromium.org/1010663002/diff/140001/cc/animation/animation...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/animation/animation...
cc/animation/animation_host.cc:163: bool
AnimationHost::GetSupportsScrollAnimations() const {
On 2015/04/21 17:57:57, ajuma wrote:
> Nit: s/GetSupportsScrollAnimations/SupportsScrollAnimations

Acknowledged.

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1026:
layer_animation_controller_->scroll_offset_animation_was_interrupted())
On 2015/04/21 17:57:57, ajuma wrote:
> This will need an equivalent in the new system.

Acknowledged.

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1274: if (layer_animation_controller_)
On 2015/04/21 17:57:57, ajuma wrote:
> This will always be true if we're constructing a controller in the
constructor,
> even when user_compositor_animation_timelines is true. (More generally, all
the
> code that's conditioned on layer_animation_controller_ is going to be taking
the
> branch corresponding to the old system.)

Yes, that's correct statement. The problem is that we don't have access to LTH
or LayerTreSettings in Layer's ctor.
So that's why I have the TODO note in constructor.

The idea is that I want to delay LayerAnimationController creation until the
RegisterForAnimations call. So I need to port all the subsystems first (see an
example here https://codereview.chromium.org/1012583002/patch/80001/90003).

I could remove the creation of LAC from RegisterForAnimations for this CL. And
make a special small CL.

Alternatively, I could setup some sort of global variable or extend a singleton
to determine use_compositor_animation_timelines state in Layer ctors. But I
don't like that. Having an initialization with a 'window' into the whole 'outer
world' sounds like correct idea for extensible code.
Any advices/preferences, please?

[loyso (OOO), 2015-04-22 01:27:30]

https://codereview.chromium.org/1010663002/diff/140001/cc/animation/animation...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/animation/animation...
cc/animation/animation_host.cc:195: AnimationPlayer* player =
GetPlayerForLayerId(layer_id);
We have a hash look-up for such a request. The number of players is very low (in
average). However, the number of requests can be quite high. As a future
solution we could manage a LayerTreeMutator weak pointer(s) from a Layer to it's
attached player(s). That pointer would be non-null within Attach/Detach scope.

[ajuma, 2015-04-22 14:36:37]

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1274: if (layer_animation_controller_)
On 2015/04/22 01:08:11, loyso wrote:
> On 2015/04/21 17:57:57, ajuma wrote:
> > This will always be true if we're constructing a controller in the
> constructor,
> > even when user_compositor_animation_timelines is true. (More generally, all
> the
> > code that's conditioned on layer_animation_controller_ is going to be taking
> the
> > branch corresponding to the old system.)
> 
> Yes, that's correct statement. The problem is that we don't have access to LTH
> or LayerTreSettings in Layer's ctor.
> So that's why I have the TODO note in constructor.
> 
> The idea is that I want to delay LayerAnimationController creation until the
> RegisterForAnimations call. So I need to port all the subsystems first (see an
> example here https://codereview.chromium.org/1012583002/patch/80001/90003).
> 
> I could remove the creation of LAC from RegisterForAnimations for this CL. And
> make a special small CL.
> 
> Alternatively, I could setup some sort of global variable or extend a
singleton
> to determine use_compositor_animation_timelines state in Layer ctors. But I
> don't like that. Having an initialization with a 'window' into the whole
'outer
> world' sounds like correct idea for extensible code.
> Any advices/preferences, please?

I think I'd prefer that the creation of LACs be moved out from the constructor
and into RegisterForAnimations (with calls to LAC::SetAnimationRegistrar
replaced with calls to Layer::RegisterForAnimations) in a separate CL that lands
before this one, so that tests added by this CL will actually test the new
system. I don't think ui::Layers will require anything special; when
SetLayerTreeHost gets called for the first time on the corresponding cc::Layers,
RegisterForAnimations will get called and will construct LACs. ui::Layers
already avoid adding animations to detached layers.

[loyso (OOO), 2015-04-23 07:58:20]

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1274: if (layer_animation_controller_)
On 2015/04/22 14:36:37, ajuma wrote:
> On 2015/04/22 01:08:11, loyso wrote:
> > On 2015/04/21 17:57:57, ajuma wrote:
> > > This will always be true if we're constructing a controller in the
> > constructor,
> > > even when user_compositor_animation_timelines is true. (More generally,
all
> > the
> > > code that's conditioned on layer_animation_controller_ is going to be
taking
> > the
> > > branch corresponding to the old system.)
> > 
> > Yes, that's correct statement. The problem is that we don't have access to
LTH
> > or LayerTreSettings in Layer's ctor.
> > So that's why I have the TODO note in constructor.
> > 
> > The idea is that I want to delay LayerAnimationController creation until the
> > RegisterForAnimations call. So I need to port all the subsystems first (see
an
> > example here https://codereview.chromium.org/1012583002/patch/80001/90003).
> > 
> > I could remove the creation of LAC from RegisterForAnimations for this CL.
And
> > make a special small CL.
> > 
> > Alternatively, I could setup some sort of global variable or extend a
> singleton
> > to determine use_compositor_animation_timelines state in Layer ctors. But I
> > don't like that. Having an initialization with a 'window' into the whole
> 'outer
> > world' sounds like correct idea for extensible code.
> > Any advices/preferences, please?
> 
> I think I'd prefer that the creation of LACs be moved out from the constructor
> and into RegisterForAnimations (with calls to LAC::SetAnimationRegistrar
> replaced with calls to Layer::RegisterForAnimations) in a separate CL that
lands
> before this one, so that tests added by this CL will actually test the new
> system. I don't think ui::Layers will require anything special; when
> SetLayerTreeHost gets called for the first time on the corresponding
cc::Layers,
> RegisterForAnimations will get called and will construct LACs. ui::Layers
> already avoid adding animations to detached layers.

That's not that simple with ui::Layers. See the high-priority CL here:
https://codereview.chromium.org/1101823002/

[loyso (OOO), 2015-04-30 07:06:01]

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1010663002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1026:
layer_animation_controller_->scroll_offset_animation_was_interrupted())
On 2015/04/21 17:57:57, ajuma wrote:
> This will need an equivalent in the new system.

Done.

[loyso (OOO), 2015-04-30 07:11:50]

loyso@chromium.org changed reviewers:
- piman@chromium.org

[loyso (OOO), 2015-06-23 04:01:28]

PTAL, everyone? This CL review may affect the previous episode (CL).

[ajuma, 2015-06-23 14:46:17]

On 2015/06/23 04:01:28, loyso wrote:
> PTAL, everyone? This CL review may affect the previous episode (CL).

This looks great, lgtm.

[loyso (OOO), 2015-07-03 04:53:20]

--disable_threaded_animation mode isn't supported yet with
--enable_compositor_animation_timelines.
We need that runtime flag on the blink site to make implementation simple.
Otherwise we have ugly code like this:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Ian Vollick, 2015-07-03 13:44:10]

On 2015/07/03 04:53:20, loyso wrote:
> --disable_threaded_animation mode isn't supported yet with
> --enable_compositor_animation_timelines.
> We need that runtime flag on the blink site to make implementation simple.
> Otherwise we have ugly code like this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

lgtm, too. Looking forward to future patches that remove all of these ternary
operators.

[Ian Vollick, 2015-07-03 13:44:16]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-07-03 13:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1010663002/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-07-03 13:44:36]

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

[commit-bot: I haz the power, 2015-07-03 13:49:01]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-03 13:50:06]

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