CC Animations: Establish AnimationHost, AnimationTimeline and AnimationPlayer

CC Animations: Establish AnimationHost, AnimationTimeline and AnimationPlayer.

The compositor should not need to worry about servicing animations.
Instead, it should be able to request property updates as needed from external
mutators.

- We setup CC representations for blink::AnimationPlayer and blink::AnimationTimeline.
- We want to move all the animation-related code from LayerTreeHost to AnimationHost.
- We move LayerAnimatedController ownership to cc::AnimationPlayer.
- We move AnimationRegistrar ownership to cc::AnimationHost.
- We add/remove animations to cc::AnimationPlayer from now.
- LayerAnimatedController to be merged into cc::AnimaitonPlayer.
- AnimationRegistrar to be merged into cc::AnimationHost/cc::AnimationTimeline.

A chromium part. Blink part: https://codereview.chromium.org/946323002

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

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

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

[loyso (OOO), 2015-02-23 05:12:03]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org
- ajuma@google.com, vollick@google.com

[ajuma, 2015-02-23 16:36:44]

This looks good at a high level. Porting over the existing unit tests (and
adding new tests for newly-added functionality) as a next step would be a great
way to iron out the remaining details.

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:26: id_ = g_next_animation_player_id.GetNext()
+ 1;
Having a combination of provided and automatically-generated ids seems
error-prone (because of the potential for accidental collisions). We should
always require an id to be provided, and make callers responsible for generating
ids when needed (e.g. WebCompositorAnimationPlayerImpl should use something like
AnimationIdProvider to generate ids).

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:76: pending_value_observer_.get());
The asymmetry between active and pending observers (where the AnimationPlayer
itself is the active observer but an external object is the pending observer)
seems a bit awkward to me. Would it make sense to have a separate
ActiveValueObserver?

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:138: if (animation_timeline() && layer_id_) {
Should the check for layer_id_ really be a DCHECK? If the layer is detached,
shouldn't the animation controller be destroyed?

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_player.h
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.h:87: class AnimationPlayerPendingValueObserver :
public LayerAnimationValueObserver {
Nit: This should go in a separate file.

https://codereview.chromium.org/947033002/diff/1/cc/blink/web_compositor_anim...
File cc/blink/web_compositor_animation_timeline_impl.h (right):

https://codereview.chromium.org/947033002/diff/1/cc/blink/web_compositor_anim...
cc/blink/web_compositor_animation_timeline_impl.h:31: //
blink::WebCompositorAnimationTimiline implementation
nit: s/Timiline/Timeline

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1427: if (layer)
In what situation would |layer| be null here? Should this be a DCHECK instead?

[loyso (OOO), 2015-02-25 04:03:30]

loyso@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@google.com

[loyso (OOO), 2015-02-25 04:37:03]

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:26: id_ = g_next_animation_player_id.GetNext()
+ 1;
On 2015/02/23 16:36:44, ajuma wrote:
> Having a combination of provided and automatically-generated ids seems
> error-prone (because of the potential for accidental collisions). We should
> always require an id to be provided, and make callers responsible for
generating
> ids when needed (e.g. WebCompositorAnimationPlayerImpl should use something
like
> AnimationIdProvider to generate ids).

I agree. That's trivial.

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:76: pending_value_observer_.get());
On 2015/02/23 16:36:44, ajuma wrote:
> The asymmetry between active and pending observers (where the AnimationPlayer
> itself is the active observer but an external object is the pending observer)
> seems a bit awkward to me. Would it make sense to have a separate
> ActiveValueObserver?

This is messy intermediate solution. We get rid of 'value observers' at the
subsequent stages of the refactoring.

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:138: if (animation_timeline() && layer_id_) {
On 2015/02/23 16:36:44, ajuma wrote:
> Should the check for layer_id_ really be a DCHECK? If the layer is detached,
> shouldn't the animation controller be destroyed?
Yes, we can go with DCHECK here.

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_player.h
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.h:87: class AnimationPlayerPendingValueObserver :
public LayerAnimationValueObserver {
On 2015/02/23 16:36:44, ajuma wrote:
> Nit: This should go in a separate file.

We will eliminate LayerAnimationValueObserver and merge LayerAnimationController
into AnimationPlayer completely. So I consider
AnimationPlayerPendingValueObserver to be a very temporary part of
AnimationPlayer.

Do you still insist that we need that intermediate migration class in a separate
file?

https://codereview.chromium.org/947033002/diff/1/cc/blink/web_compositor_anim...
File cc/blink/web_compositor_animation_timeline_impl.h (right):

https://codereview.chromium.org/947033002/diff/1/cc/blink/web_compositor_anim...
cc/blink/web_compositor_animation_timeline_impl.h:31: //
blink::WebCompositorAnimationTimiline implementation
On 2015/02/23 16:36:44, ajuma wrote:
> nit: s/Timiline/Timeline
fixed.

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1427: if (layer)
On 2015/02/23 16:36:44, ajuma wrote:
> In what situation would |layer| be null here? Should this be a DCHECK instead?
Active tree is a 'passive' view of pending tree. Object creation/deletion
updates propagate from main tree to impl pending tree to impl active tree in
non-deterministic way. We don't notify external mutators on all those multiple
copies of the entity. That's the decoupling point.

We could refine it at later stages, though. Via introduction of some sort of
proxy-layer object so we can keep weak references externally.

[ajuma, 2015-02-25 14:57:25]

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:76: pending_value_observer_.get());
On 2015/02/25 04:37:03, loyso wrote:
> On 2015/02/23 16:36:44, ajuma wrote:
> > The asymmetry between active and pending observers (where the
AnimationPlayer
> > itself is the active observer but an external object is the pending
observer)
> > seems a bit awkward to me. Would it make sense to have a separate
> > ActiveValueObserver?
> 
> This is messy intermediate solution. We get rid of 'value observers' at the
> subsequent stages of the refactoring.

We should aim to keep things as clean and readable as possible at each stage of
the refactor. Would separating out ActiveValueObserver here make subsequent
stages significantly more complicated?

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_player.h
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.h:87: class AnimationPlayerPendingValueObserver :
public LayerAnimationValueObserver {
On 2015/02/25 04:37:03, loyso wrote:
> On 2015/02/23 16:36:44, ajuma wrote:
> > Nit: This should go in a separate file.
> 
> We will eliminate LayerAnimationValueObserver and merge
LayerAnimationController
> into AnimationPlayer completely. So I consider
> AnimationPlayerPendingValueObserver to be a very temporary part of
> AnimationPlayer.
> 
> Do you still insist that we need that intermediate migration class in a
separate
> file?

If this is purely an implementation detail of AnimationPlayer, the whole thing
can be moved to animation_player.cc or be made an internal class (see below).
Otherwise, it should get its own {.h, .cc} files.

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.h:98: friend class AnimationPlayer;
I missed this the first time through, but rather than making AnimationPlayer and
AnimationPlayerPendingValueObserver mutual friends, how about making
AnimationPlayerPendingValueObserver an internal class?

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1427: if (layer)
On 2015/02/25 04:37:03, loyso wrote:
> On 2015/02/23 16:36:44, ajuma wrote:
> > In what situation would |layer| be null here? Should this be a DCHECK
instead?
> Active tree is a 'passive' view of pending tree. Object creation/deletion
> updates propagate from main tree to impl pending tree to impl active tree in
> non-deterministic way. We don't notify external mutators on all those multiple
> copies of the entity. That's the decoupling point.

I'm not sure I follow. LayerTreeHost is the main-thread side, so there is only a
single tree, not an active/pending pair. An external mutator trying to animate a
layer that no longer exists on the main thread seems like a bug at first glance.
Could you write a unit test (probably a LayerTreeHostAnimationTest) that fails
when this 'if' is changed to a DCHECK?

More generally, mutators cannot be ignorant of the progress of updates from the
main thread to the pending tree and then to the active tree, since we need to
preserve the atomicity of updates. For example, if some javascript code triggers
an animation and at the same time makes other changes to the page, the animation
must start at the same time as the other changes appear on screen.

[loyso (OOO), 2015-02-26 03:04:45]

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.cc:76: pending_value_observer_.get());
On 2015/02/25 14:57:25, ajuma wrote:
> On 2015/02/25 04:37:03, loyso wrote:
> > On 2015/02/23 16:36:44, ajuma wrote:
> > > The asymmetry between active and pending observers (where the
> AnimationPlayer
> > > itself is the active observer but an external object is the pending
> observer)
> > > seems a bit awkward to me. Would it make sense to have a separate
> > > ActiveValueObserver?
> > 
> > This is messy intermediate solution. We get rid of 'value observers' at the
> > subsequent stages of the refactoring.
> 
> We should aim to keep things as clean and readable as possible at each stage
of
> the refactor. Would separating out ActiveValueObserver here make subsequent
> stages significantly more complicated?

No, not at all. We can spawn many nested classes.

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_player.h
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/1/cc/animation/animation_playe...
cc/animation/animation_player.h:98: friend class AnimationPlayer;
On 2015/02/25 14:57:25, ajuma wrote:
> I missed this the first time through, but rather than making AnimationPlayer
and
> AnimationPlayerPendingValueObserver mutual friends, how about making
> AnimationPlayerPendingValueObserver an internal class? 

Do you mean nested class? Ok. I'm going to make it nested, use a pImpl idiom,
move to .cc.

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1427: if (layer)
On 2015/02/25 14:57:25, ajuma wrote:
> On 2015/02/25 04:37:03, loyso wrote:
> > On 2015/02/23 16:36:44, ajuma wrote:
> > > In what situation would |layer| be null here? Should this be a DCHECK
> instead?
> > Active tree is a 'passive' view of pending tree. Object creation/deletion
> > updates propagate from main tree to impl pending tree to impl active tree in
> > non-deterministic way. We don't notify external mutators on all those
multiple
> > copies of the entity. That's the decoupling point.
> 
> I'm not sure I follow. LayerTreeHost is the main-thread side, so there is only
a
> single tree, not an active/pending pair. An external mutator trying to animate
a
> layer that no longer exists on the main thread seems like a bug at first
glance.
> Could you write a unit test (probably a LayerTreeHostAnimationTest) that fails
> when this 'if' is changed to a DCHECK?
Oh, sorry. I was commenting with LayerTreeHostImpl in mind. Yes, we need DCHECK
here.
 
> More generally, mutators cannot be ignorant of the progress of updates from
the
> main thread to the pending tree and then to the active tree, since we need to
> preserve the atomicity of updates. For example, if some javascript code
triggers
> an animation and at the same time makes other changes to the page, the
animation
> must start at the same time as the other changes appear on screen.
We use needs_synchronized_start_time flag for that, don't we? Any other cases?

[ajuma, 2015-02-26 14:16:31]

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1427: if (layer)
> > More generally, mutators cannot be ignorant of the progress of updates from
> the
> > main thread to the pending tree and then to the active tree, since we need
to
> > preserve the atomicity of updates. For example, if some javascript code
> triggers
> > an animation and at the same time makes other changes to the page, the
> animation
> > must start at the same time as the other changes appear on screen.
> We use needs_synchronized_start_time flag for that, don't we? Any other cases?

needs_synchronized_start_time means "use the compositor's start time". It's
unrelated to ensuring that animations added during a commit don't start until
tree activation (rather than starting immediately after the commit). The logic
to ensure that animations propagate in-sync with tree activation is implemented
using the affects_active_observers and affects_pending_observers logic in
animation.cc and layer_animation_controller.cc. The comment at
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima...
has a brief description of this logic.

[loyso (OOO), 2015-02-27 02:45:17]

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1427: if (layer)
On 2015/02/26 14:16:31, ajuma wrote:
> > > More generally, mutators cannot be ignorant of the progress of updates
from
> > the
> > > main thread to the pending tree and then to the active tree, since we need
> to
> > > preserve the atomicity of updates. For example, if some javascript code
> > triggers
> > > an animation and at the same time makes other changes to the page, the
> > animation
> > > must start at the same time as the other changes appear on screen.
> > We use needs_synchronized_start_time flag for that, don't we? Any other
cases?
> 
> needs_synchronized_start_time means "use the compositor's start time". It's
> unrelated to ensuring that animations added during a commit don't start until
> tree activation (rather than starting immediately after the commit). The logic
> to ensure that animations propagate in-sync with tree activation is
implemented
> using the affects_active_observers and affects_pending_observers logic in
> animation.cc and layer_animation_controller.cc. The comment at
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima...
> has a brief description of this logic.

Thank you so much Ali for pointing out that important issue. See the PatchSet#3
- AnimationPlayer now knows about layer tree activation.

[Ian Vollick, 2015-02-27 15:49:55]

On 2015/02/27 02:45:17, loyso wrote:
> https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/947033002/diff/1/cc/trees/layer_tree_host.cc#...
> cc/trees/layer_tree_host.cc:1427: if (layer)
> On 2015/02/26 14:16:31, ajuma wrote:
> > > > More generally, mutators cannot be ignorant of the progress of updates
> from
> > > the
> > > > main thread to the pending tree and then to the active tree, since we
need
> > to
> > > > preserve the atomicity of updates. For example, if some javascript code
> > > triggers
> > > > an animation and at the same time makes other changes to the page, the
> > > animation
> > > > must start at the same time as the other changes appear on screen.
> > > We use needs_synchronized_start_time flag for that, don't we? Any other
> cases?
> > 
> > needs_synchronized_start_time means "use the compositor's start time". It's
> > unrelated to ensuring that animations added during a commit don't start
until
> > tree activation (rather than starting immediately after the commit). The
logic
> > to ensure that animations propagate in-sync with tree activation is
> implemented
> > using the affects_active_observers and affects_pending_observers logic in
> > animation.cc and layer_animation_controller.cc. The comment at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima...
> > has a brief description of this logic.
> 
> Thank you so much Ali for pointing out that important issue. See the
PatchSet#3
> - AnimationPlayer now knows about layer tree activation.

I realize this is a huge hassle, but before getting too much further in the
specifics of these WIP patches, I think we should meet ASAP to discuss the
design in light of the planned scrolling refactor. I'll set something up
offline.

[aelias_OOO_until_Jul13, 2015-03-13 01:03:18]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-03-13 01:03:19]

I took some time today to understand better what these patches are doing and I
have a high-level question first.  It looks like until "Stage 6" of your plan
we'd end up maintaining three animation data structure forests: the impl-thread
side one, the CC main-thread one, and the Blink one.  The two main-thread ones
are redundant with each other and you're implementing bindings between them that
seem like they're going to amount to quite a bit of complexity.

I wonder if we could move forward "Stage 6" code sharing to much earlier in the
implementation plan?  We could copy over the contents of Source/core/animation/
to cc/ and add interfaces to Blink call into CC whenever it does anything
animation-related (even fine-grained stuff like looking at curves).  What do you
think?

[loyso (OOO), 2015-03-13 04:59:27]

On 2015/03/13 01:03:19, aelias wrote:
> I took some time today to understand better what these patches are doing and I
> have a high-level question first.  It looks like until "Stage 6" of your plan
> we'd end up maintaining three animation data structure forests: the
impl-thread
> side one, the CC main-thread one, and the Blink one.  The two main-thread ones
> are redundant with each other and you're implementing bindings between them
that
> seem like they're going to amount to quite a bit of complexity.
> 
> I wonder if we could move forward "Stage 6" code sharing to much earlier in
the
> implementation plan?  We could copy over the contents of
Source/core/animation/
> to cc/ and add interfaces to Blink call into CC whenever it does anything
> animation-related (even fine-grained stuff like looking at curves).  What do
you
> think?

As for three animation data structure forests, I see it the following way:
The impl-thread side one and CC main-thread one already share
LayerAnimationController, Animation, curves and timing functions. So we have
just two forests, basically. Those forests are quite stable, we don't spend any
significant efforts to maintain them. We want some features to be implemented
(like same-property animation priorities, same-property animation stacking with
fillmode=forwards support etc), but they are low priority enough to be postponed
until we have the animation engine unified and shared.

As for copying Curves and Timing Functions code from Source/core/animation/ to
cc/... What's the benefit? If we don't introduce significant structural changes
in cc/animations, then we can substitute only small low-level subset of the
engine: curves and timing functions. This substitution is a low-risk routine job
which gives us nothing. The essence of advanced changes we want (same-property
animation priorities and stacking, for example) is hidden in
LayerAnimationController/AnimationRegistrar logic and their interaction with
cc::Layer/LTH/LTHI/LTI objects. That's why we want to 'massage'
LayerAnimationController/AnimationRegistrar into
AnimationPlayer/AnimationTimeline respectively. In general, we want to move the
cc/animation model towards blink model (less stateful, mutator-based where time
can be a random-access input parameter to get proper output).

[loyso (OOO), 2015-03-13 05:09:07]

On 2015/03/13 01:03:19, aelias wrote:
> add interfaces to Blink call into CC whenever it does anything
> animation-related (even fine-grained stuff like looking at curves).

p.s. Apart from that: those interfaces would be quite messy. Because we can't
express all the low level stuff with just pure abstract classes.
We struggle with enums, OwnPtr's and pervasive Dependency Inhection/Inversion.

[aelias_OOO_until_Jul13, 2015-03-17 04:31:12]

On 2015/03/13 at 04:59:27, loyso wrote:
> On 2015/03/13 01:03:19, aelias wrote:
> That's why we want to 'massage' LayerAnimationController/AnimationRegistrar
into AnimationPlayer/AnimationTimeline respectively. In general, we want to move
the cc/animation model towards blink model (less stateful, mutator-based where
time can be a random-access input parameter to get proper output).

Right, this sounds great to me, and my point is that I wonder if we can also
delete the Blink-side AnimationPlayer/AnimationTimeline if the CC main-thread
side encompasses the same capabilities?

[dstockwell, 2015-03-17 04:47:43]

On 2015/03/17 at 04:31:12, aelias wrote:
> On 2015/03/13 at 04:59:27, loyso wrote:
> > On 2015/03/13 01:03:19, aelias wrote:
> > That's why we want to 'massage' LayerAnimationController/AnimationRegistrar
into AnimationPlayer/AnimationTimeline respectively. In general, we want to move
the cc/animation model towards blink model (less stateful, mutator-based where
time can be a random-access input parameter to get proper output).
> 
> Right, this sounds great to me, and my point is that I wonder if we can also
delete the Blink-side AnimationPlayer/AnimationTimeline if the CC main-thread
side encompasses the same capabilities?

I don't think that's really going to be feasible given that the blink-side needs
to expose JavaScript bindings to these types.

[aelias_OOO_until_Jul13, 2015-03-17 18:20:07]

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

[aelias_OOO_until_Jul13, 2015-03-17 18:21:41]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-03-17 18:21:42]

OK.  Well, I think I'm still lacking the full knowledge and the cycles to
micro-review this, so I'll take myself off the reviewer list and let
vollick@/ajuma@ do the review.

[loyso (OOO), 2015-04-22 01:40:45]

Tests added. PTAL!

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

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:22: }  // namespace cc
This file is a stub. Tests to be added in the next episode
(https://codereview.chromium.org/1010663002/).
I could remove it from this CL.

[ajuma, 2015-04-22 15:47:18]

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_host.cc:70: if (player)
Can this be a DCHECK? (And if not, what if RegisterPlayerForLayer gets called
later?)

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_host.h:25: class CC_EXPORT AnimationHost {
It'd be helpful to have high-level comments somewhere describing how all the new
classes interact with each other (e.g. who owns what, is the relationship 1:1 or
many:1, etc.), either all in a single place, or in each of the new classes,
whichever seems clearer.

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:278: // removing player from timeline
detaches layer.
Style nit: Start comments with a capital letter (here and elsewhere).

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:287: // always and only in active tree
for main thread.
Please re-phrase this comment to make it clearer.

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

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3466: if (affects_active_tree) {
This logic doesn't seem to handle the case where an animation affects both the
pending tree and the active tree. Once an animation starts affecting the active
tree, it should continue affecting the pending tree too, until it finishes or is
removed during a commit.

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:22: }  // namespace cc
On 2015/04/22 01:46:01, loyso wrote:
> This file is a stub. Tests to be added in the next episode
> (https://codereview.chromium.org/1010663002/).
> I could remove it from this CL.

Yes, please remove it and add it to the CL where it becomes non-empty.

[loyso (OOO), 2015-04-30 06:40:22]

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_host.cc:70: if (player)
On 2015/04/22 15:47:18, ajuma wrote:
> Can this be a DCHECK? (And if not, what if RegisterPlayerForLayer gets called
> later?)

No, it can't. AnimationPlayer::AddControllerToTimeline (which calls
RegisterPlayerForLayer) checks, whether layer was registered before via
animation_host_->layer_tree_mutators_client()->IsLayerInActiveTree() or
animation_host_->layer_tree_mutators_client()->IsLayerInPendingTree().

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_host.h:25: class CC_EXPORT AnimationHost {
On 2015/04/22 15:47:18, ajuma wrote:
> It'd be helpful to have high-level comments somewhere describing how all the
new
> classes interact with each other (e.g. who owns what, is the relationship 1:1
or
> many:1, etc.), either all in a single place, or in each of the new classes,
> whichever seems clearer.

Done.

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:278: // removing player from timeline
detaches layer.
On 2015/04/22 15:47:18, ajuma wrote:
> Style nit: Start comments with a capital letter (here and elsewhere).

Acknowledged.

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:287: // always and only in active tree
for main thread.
On 2015/04/22 15:47:18, ajuma wrote:
> Please re-phrase this comment to make it clearer.

Done.

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

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3466: if (affects_active_tree) {
On 2015/04/22 15:47:18, ajuma wrote:
> This logic doesn't seem to handle the case where an animation affects both the
> pending tree and the active tree. Once an animation starts affecting the
active
> tree, it should continue affecting the pending tree too, until it finishes or
is
> removed during a commit.

Each cc::AnimationPlayer has up to two AnimationPlayer::ValueObservers.
SetLayerOpacityMutated is called twice with affects_active_tree as true/false.

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (right):

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:22: }  // namespace cc
On 2015/04/22 15:47:18, ajuma wrote:
> On 2015/04/22 01:46:01, loyso wrote:
> > This file is a stub. Tests to be added in the next episode
> > (https://codereview.chromium.org/1010663002/).
> > I could remove it from this CL.
> 
> Yes, please remove it and add it to the CL where it becomes non-empty.

Done.

[loyso (OOO), 2015-04-30 07:12:52]

loyso@chromium.org changed reviewers:
- aelias@chromium.org, dstockwell@chromium.org, shane@chromium.org

[ajuma, 2015-04-30 13:17:54]

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

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3466: if (affects_active_tree) {
On 2015/04/30 06:40:21, loyso wrote:
> On 2015/04/22 15:47:18, ajuma wrote:
> > This logic doesn't seem to handle the case where an animation affects both
the
> > pending tree and the active tree. Once an animation starts affecting the
> active
> > tree, it should continue affecting the pending tree too, until it finishes
or
> is
> > removed during a commit.
> 
> Each cc::AnimationPlayer has up to two AnimationPlayer::ValueObservers.
> SetLayerOpacityMutated is called twice with affects_active_tree as true/false.

Ah, ok, that makes sense. But I'm not seeing how these ValueObservers are
created/destroyed at tree activation time, which part should I look at for this?
Thanks for bearing with me here :)

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:33: // AnimationHost has it's own instance of
AnimationRegistrar (to be merged).
Nit: s/it's/its (also above and elsewhere)

[loyso (OOO), 2015-05-01 00:46:55]

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

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3466: if (affects_active_tree) {
On 2015/04/30 13:17:53, ajuma wrote:
> On 2015/04/30 06:40:21, loyso wrote:
> > On 2015/04/22 15:47:18, ajuma wrote:
> > > This logic doesn't seem to handle the case where an animation affects both
> the
> > > pending tree and the active tree. Once an animation starts affecting the
> > active
> > > tree, it should continue affecting the pending tree too, until it finishes
> or
> > is
> > > removed during a commit.
> > 
> > Each cc::AnimationPlayer has up to two AnimationPlayer::ValueObservers.
> > SetLayerOpacityMutated is called twice with affects_active_tree as
true/false.
> 
> Ah, ok, that makes sense. But I'm not seeing how these ValueObservers are
> created/destroyed at tree activation time, which part should I look at for
this?
> Thanks for bearing with me here :)

No worries. That's a key detail.
AnimationPlayer receives notification on active OR pending LayerImpl creation.
(see AnimationPlayer::LayerRegistered, AnimationPlayer::LayerUnregistered). This
completely mimics the old behavior with the LAC and observers creation in the
LayerImpl constructor.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:33: // AnimationHost has it's own instance of
AnimationRegistrar (to be merged).
On 2015/04/30 13:17:54, ajuma wrote:
> Nit: s/it's/its (also above and elsewhere)

Acknowledged.

[loyso (OOO), 2015-05-01 04:02:54]

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/200001/cc/animation/animation_...
cc/animation/animation_host.h:25: class CC_EXPORT AnimationHost {
On 2015/04/22 15:47:18, ajuma wrote:
> It'd be helpful to have high-level comments somewhere describing how all the
new
> classes interact with each other (e.g. who owns what, is the relationship 1:1
or
> many:1, etc.), either all in a single place, or in each of the new classes,
> whichever seems clearer.

Moreover, I'm going to update the design document and share it for all chromium
devs.

[ajuma, 2015-05-01 14:45:40]

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

https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3466: if (affects_active_tree) {
On 2015/05/01 00:46:55, loyso wrote:
> On 2015/04/30 13:17:53, ajuma wrote:
> > On 2015/04/30 06:40:21, loyso wrote:
> > > On 2015/04/22 15:47:18, ajuma wrote:
> > > > This logic doesn't seem to handle the case where an animation affects
both
> > the
> > > > pending tree and the active tree. Once an animation starts affecting the
> > > active
> > > > tree, it should continue affecting the pending tree too, until it
finishes
> > or
> > > is
> > > > removed during a commit.
> > > 
> > > Each cc::AnimationPlayer has up to two AnimationPlayer::ValueObservers.
> > > SetLayerOpacityMutated is called twice with affects_active_tree as
> true/false.
> > 
> > Ah, ok, that makes sense. But I'm not seeing how these ValueObservers are
> > created/destroyed at tree activation time, which part should I look at for
> this?
> > Thanks for bearing with me here :)
> 
> No worries. That's a key detail.
> AnimationPlayer receives notification on active OR pending LayerImpl creation.
> (see AnimationPlayer::LayerRegistered, AnimationPlayer::LayerUnregistered).
This
> completely mimics the old behavior with the LAC and observers creation in the
> LayerImpl constructor.

Thanks for the explanation. Just to make sure I understand,
LayerAnimationController will still be responsible for using the
affects_active_observers_ and affects_pending_observers_ bools on each Animation
to decide which AnimationPlayer::ValueObservers get notified (in
LAC::NotifyObservers*Animated)?

[loyso (OOO), 2015-05-04 01:29:25]

On 2015/05/01 14:45:40, ajuma wrote:
>
https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/947033002/diff/200001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:3466: if (affects_active_tree) {
> On 2015/05/01 00:46:55, loyso wrote:
> > On 2015/04/30 13:17:53, ajuma wrote:
> > > On 2015/04/30 06:40:21, loyso wrote:
> > > > On 2015/04/22 15:47:18, ajuma wrote:
> > > > > This logic doesn't seem to handle the case where an animation affects
> both
> > > the
> > > > > pending tree and the active tree. Once an animation starts affecting
the
> > > > active
> > > > > tree, it should continue affecting the pending tree too, until it
> finishes
> > > or
> > > > is
> > > > > removed during a commit.
> > > > 
> > > > Each cc::AnimationPlayer has up to two AnimationPlayer::ValueObservers.
> > > > SetLayerOpacityMutated is called twice with affects_active_tree as
> > true/false.
> > > 
> > > Ah, ok, that makes sense. But I'm not seeing how these ValueObservers are
> > > created/destroyed at tree activation time, which part should I look at for
> > this?
> > > Thanks for bearing with me here :)
> > 
> > No worries. That's a key detail.
> > AnimationPlayer receives notification on active OR pending LayerImpl
creation.
> > (see AnimationPlayer::LayerRegistered, AnimationPlayer::LayerUnregistered).
> This
> > completely mimics the old behavior with the LAC and observers creation in
the
> > LayerImpl constructor.
> 
> Thanks for the explanation. Just to make sure I understand,
> LayerAnimationController will still be responsible for using the
> affects_active_observers_ and affects_pending_observers_ bools on each
Animation
> to decide which AnimationPlayer::ValueObservers get notified (in
> LAC::NotifyObservers*Animated)?

Yes, that's correct. We don't touch that logic for now. Once we eliminate the
old system, we can dissolve LAC in AnimationPlayer and rewrite LTH+AnimationHost
communication to make tree activation/draw phases explicit.

[ajuma, 2015-05-04 13:42:54]

Thanks, cc/ lgtm, but I'd like vollick@ to have a look too.

[Ian Vollick, 2015-05-05 03:52:07]

I think I'm getting a picture of how this step of your plan hangs together, but
I have to admit, it's a pretty overwhelming patch. Is it possible to break it
up? A diagram explaining the relationships between all these classes (in
particular, the cc-to-blink glue) would be very helpful.

Here are some comments and (likely dumb) questions.

https://codereview.chromium.org/947033002/diff/100001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/100001/cc/animation/animation_...
cc/animation/animation_player.h:37: void
set_layer_animation_delegate(AnimationDelegate* delegate);
Will we need to plumb back animation start times once we've converted to web
animations fully?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.cc:92: if (iter != layer_to_player_map_.end() &&
iter->second == player)
when would iter->second != player?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:33: // AnimationHost has it's own instance of
AnimationRegistrar (to be merged).
To be clear, you mean that the AnimationRegistrar will eventually be merged into
AnimationHost?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:39: void AddAnimationTimeline(AnimationTimeline*
timeline);
If it's going to hold a reference to the animation timeline objects, should it
not pass a scoped_refptr to make transfer of ownership clear?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:45: void RegisterLayer(int layer_id, bool
is_active_tree);
Please use an enum rather than a boolean. It makes the call sites easier to
read. Here and other places where you use a bool to mean "this is the impl
thread"

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:65: AnimationRegistrar* animation_registrar()
const {
This looks like it could be a reference given that the registrar is created in
the ctor and its lifetime is tied to the host's.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host_unittest.cc:16: TEST(AnimationHostTest,
SyncTimelinesAddRemove) {
Is layer/player registration and unregistration tested elsewhere?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:18: : player_(player), is_active_(is_active) {
Just to make sure I'm understanding this correctly, the
AnimationPlayer::ValueObserver will let the AnimationPlayer just forward along
updates from the LAC? I.e., the LAC will update values, and they'll make their
way into the relevant OnXXXAnimated fn which just passes through to the player's
SetXXXMutated?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:40: // TODO(loyso): implement it.
Why's it ok to put this off?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:85: RemoveControllerFromTimeline();
Can you add a comment explaining why you need to remove and add the controller?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:236:
animation_host()->layer_tree_mutators_client()->SetLayerTransformMutated(
If I understand correctly, this will cause us to find layer-by-id in either LTH
or LTHI and then apply the mutation. Is there any performance impact?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.h:35: class CC_EXPORT AnimationPlayer : public
base::RefCounted<AnimationPlayer>,
Why RefCounted?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:150: class AnimationDelegateTest :
public AnimationDelegate {
Name's a bit confusing; this isn't a test.

How about TestAnimationDelegate? Here and other places where Test is used as a
suffix for something that's not a test.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:187: void GetImplObjectsByIDs() {
nit: I find the name of this function uninformative. It looks like it's checking
that you've got a timeline and player (and updating the relevant members). How
about EnsureTimelineAndPlayer?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:438:
client_impl_.IsPropertyMutated(layer_id_, false, Animation::FILTER));
Should we check that they get mutated to the right values?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:463: EXPECT_TRUE(delegate.started_);
Do we need to test started _and_ finished?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_timeline.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_timeline.h:27: class CC_EXPORT AnimationTimeline : public
base::RefCounted<AnimationTimeline> {
Why refptr?

https://codereview.chromium.org/947033002/diff/220001/cc/trees/layer_tree_mut...
File cc/trees/layer_tree_mutators_client.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/trees/layer_tree_mut...
cc/trees/layer_tree_mutators_client.h:18: class LayerTreeMutatorsClient {
In future, we'll have a transform/opacity/filter/scroll mutator clients for each
of the property trees?

[loyso (OOO), 2015-05-05 04:49:48]

On 2015/05/04 13:42:54, ajuma wrote:
> Thanks, cc/ lgtm, but I'd like vollick@ to have a look too.
Thanks, Ali!

content/renderer/gpu/render_widget_compositor.cc - PTAL, anyone? That's just
obvious plumbing.

[loyso (OOO), 2015-05-05 05:00:00]

On 2015/05/05 03:52:07, vollick wrote:
> A diagram explaining the relationships between all these classes (in
> particular, the cc-to-blink glue) would be very helpful.
Just a fast note for the cc-to-blink glue:
blink::AnimationPlayer/Timeline to cc::AnimationPlayer/Timeline
interaction/ownership/lifetimes completely mimics the behavior for
blink::WebLayer -> blink::WebLayerImpl -> cc::Layer.

[loyso (OOO), 2015-05-05 06:51:19]

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.cc:92: if (iter != layer_to_player_map_.end() &&
iter->second == player)
On 2015/05/05 03:52:06, vollick wrote:
> when would iter->second != player?
Good point! We should go with DCHECK here and in RegisterPlayerForLayer.
In future, we must be able to run multiple AnimationPlayers per
blink::Element/cc::Layer.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:33: // AnimationHost has it's own instance of
AnimationRegistrar (to be merged).
On 2015/05/05 03:52:06, vollick wrote:
> To be clear, you mean that the AnimationRegistrar will eventually be merged
into
> AnimationHost?
Yes, correct.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:39: void AddAnimationTimeline(AnimationTimeline*
timeline);
On 2015/05/05 03:52:06, vollick wrote:
> If it's going to hold a reference to the animation timeline objects, should it
> not pass a scoped_refptr to make transfer of ownership clear?
It would require a complete definition for AnimationTimeline class in all
translation units where we pass that pointer. Since we have a lot of obvious
plumbing and it's an intrusive RefCounting I've decided to go with forward
declarations. Changing this to scoped_refptr will require several #includes and
it will increase coupling. Plus some runtime overhead on passing.
Isn't it better to add a comment? =)

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:45: void RegisterLayer(int layer_id, bool
is_active_tree);
On 2015/05/05 03:52:06, vollick wrote:
> Please use an enum rather than a boolean. It makes the call sites easier to
> read. Here and other places where you use a bool to mean "this is the impl
> thread"
I wanted to go the LAC way with bools and no extra #include declarations
necessary since cc::Layers are deprecated. But yeah, I can add enum class.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:65: AnimationRegistrar* animation_registrar()
const {
On 2015/05/05 03:52:06, vollick wrote:
> This looks like it could be a reference given that the registrar is created in
> the ctor and its lifetime is tied to the host's.
I preserved the accessor signature from LayerTreeHost/LayerTreeHostImpl where we
have identical situation (since AnimationRegistrar is marked for deletion).

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host_unittest.cc:16: TEST(AnimationHostTest,
SyncTimelinesAddRemove) {
On 2015/05/05 03:52:06, vollick wrote:
> Is layer/player registration and unregistration tested elsewhere?
Yeah, that's in animation_player_unittest.cc

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:18: : player_(player), is_active_(is_active) {
On 2015/05/05 03:52:06, vollick wrote:
> Just to make sure I'm understanding this correctly, the
> AnimationPlayer::ValueObserver will let the AnimationPlayer just forward along
> updates from the LAC? I.e., the LAC will update values, and they'll make their
> way into the relevant OnXXXAnimated fn which just passes through to the
player's
> SetXXXMutated?

Yes, that's absolutely correct.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:40: // TODO(loyso): implement it.
On 2015/05/05 03:52:06, vollick wrote:
> Why's it ok to put this off?
It's in separate CL for scroll offset.
https://codereview.chromium.org/1009233002/patch/140001/150003

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:85: RemoveControllerFromTimeline();
On 2015/05/05 03:52:06, vollick wrote:
> Can you add a comment explaining why you need to remove and add the
controller?

Acknowledged.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:236:
animation_host()->layer_tree_mutators_client()->SetLayerTransformMutated(
On 2015/05/05 03:52:06, vollick wrote:
> If I understand correctly, this will cause us to find layer-by-id in either
LTH
> or LTHI and then apply the mutation. Is there any performance impact?
Yes, that's a hash_map look up. We discussed that in person, as you might
remember. =) Yes, we should pay attention to this and monitor the performance
for any regressions. In general, layer_id represents a set of objects. It looks
like we can enhance it in the future with a list of subscribers.

The number of players is very low in general.

Moreover, we have a reverse mapping. That's more risky because the number of
requests can be huge. See my lonely comment here:
https://codereview.chromium.org/1010663002/patch/140001/150001

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.h:35: class CC_EXPORT AnimationPlayer : public
base::RefCounted<AnimationPlayer>,
On 2015/05/05 03:52:06, vollick wrote:
> Why RefCounted?

It's in shared ownership between blink::AnimationPlayer and
cc::AnimationTimeline/cc::AnimationHost(=Registrar). Same idea as for cc::Layer.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:150: class AnimationDelegateTest :
public AnimationDelegate {
On 2015/05/05 03:52:07, vollick wrote:
> Name's a bit confusing; this isn't a test.
> 
> How about TestAnimationDelegate? Here and other places where Test is used as a
> suffix for something that's not a test.

Acknowledged.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:187: void GetImplObjectsByIDs() {
On 2015/05/05 03:52:07, vollick wrote:
> nit: I find the name of this function uninformative. It looks like it's
checking
> that you've got a timeline and player (and updating the relevant members). How
> about EnsureTimelineAndPlayer?
It brings me impl-side object pointers if only IDs known.
How about ObtainImplPointersByIDs?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:438:
client_impl_.IsPropertyMutated(layer_id_, false, Animation::FILTER));
On 2015/05/05 03:52:07, vollick wrote:
> Should we check that they get mutated to the right values?
That's a matter of balance :) Yes, we can.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:463: EXPECT_TRUE(delegate.started_);
On 2015/05/05 03:52:06, vollick wrote:
> Do we need to test started _and_ finished?
This test is about the delegate accumulation/buffering.

What you propose is more about integration with LTH. I.e. a part of
layer_tree_host_unittest_animation_timelines.cc

But yes, we could introduce some redundancy.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_timeline.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_timeline.h:27: class CC_EXPORT AnimationTimeline : public
base::RefCounted<AnimationTimeline> {
On 2015/05/05 03:52:07, vollick wrote:
> Why refptr?
Shared between cc::AnimationHost and blink::AnimationTimeline. Same as for
cc::Layer and cc::AnimationPlayer.

https://codereview.chromium.org/947033002/diff/220001/cc/trees/layer_tree_mut...
File cc/trees/layer_tree_mutators_client.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/trees/layer_tree_mut...
cc/trees/layer_tree_mutators_client.h:18: class LayerTreeMutatorsClient {
On 2015/05/05 03:52:07, vollick wrote:
> In future, we'll have a transform/opacity/filter/scroll mutator clients for
each
> of the property trees?
Sure. We can divide this interface and conquer. These flows are lose-coupled.

[Ian Vollick, 2015-05-07 13:58:43]

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:39: void AddAnimationTimeline(AnimationTimeline*
timeline);
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > If it's going to hold a reference to the animation timeline objects, should
it
> > not pass a scoped_refptr to make transfer of ownership clear?
> It would require a complete definition for AnimationTimeline class in all
> translation units where we pass that pointer. Since we have a lot of obvious
> plumbing and it's an intrusive RefCounting I've decided to go with forward
> declarations. Changing this to scoped_refptr will require several #includes
and
> it will increase coupling.

I don't think this is true; this appears to compile:
https://codereview.chromium.org/1131593007

> Plus some runtime overhead on passing.

I think this cost is negligible, and I prefer the clarity/safety of passing the
refptr. I know that in this case, you coerce the ptr into a scoped_refptr when
you add it to the vector, but in general, function(Thingy* thingy) is risky if
|thingy| is ref counted. If the callee hangs onto the ptr, it can end up with a
pointer to nothing if the caller decrements the ref count.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:45: void RegisterLayer(int layer_id, bool
is_active_tree);
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > Please use an enum rather than a boolean. It makes the call sites easier to
> > read. Here and other places where you use a bool to mean "this is the impl
> > thread"
> I wanted to go the LAC way with bools and no extra #include declarations
> necessary since cc::Layers are deprecated. But yeah, I can add enum class.

Using bools was probably a mistake with LAC. I would prefer that you switch to
an enum here, but it's probably not worth updating the LAC to use it since it's
hopefully going away.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:65: AnimationRegistrar* animation_registrar()
const {
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > This looks like it could be a reference given that the registrar is created
in
> > the ctor and its lifetime is tied to the host's.
> I preserved the accessor signature from LayerTreeHost/LayerTreeHostImpl where
we
> have identical situation (since AnimationRegistrar is marked for deletion).

I would prefer the reference, but if this accessor is temporary, then I don't
have strong feelings.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host_unittest.cc:16: TEST(AnimationHostTest,
SyncTimelinesAddRemove) {
On 2015/05/05 at 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > Is layer/player registration and unregistration tested elsewhere?
> Yeah, that's in animation_player_unittest.cc

k, great. Please add a comment to this effect here.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:40: // TODO(loyso): implement it.
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > Why's it ok to put this off?
> It's in separate CL for scroll offset.
> https://codereview.chromium.org/1009233002/patch/140001/150003

My questions is more about staging. Will the code still be correct before the
other CL you cite lands?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:236:
animation_host()->layer_tree_mutators_client()->SetLayerTransformMutated(
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > If I understand correctly, this will cause us to find layer-by-id in either
> LTH
> > or LTHI and then apply the mutation. Is there any performance impact?
> Yes, that's a hash_map look up. We discussed that in person, as you might
> remember. =) Yes, we should pay attention to this and monitor the performance
> for any regressions. In general, layer_id represents a set of objects. It
looks
> like we can enhance it in the future with a list of subscribers.
> 
> The number of players is very low in general.
> 
> Moreover, we have a reverse mapping. That's more risky because the number of
> requests can be huge. See my lonely comment here:
> https://codereview.chromium.org/1010663002/patch/140001/150001

I think you're right that we won't have that many players in general, but having
hundreds on a highly dynamic page seems like something we should be prepared
for.

With this CL, we switch from pointer derefs to hash map lookups and it would be
nice to quantify the impact this has on pages with a lot of animated layers. It
will hopefully be too small to worry about, but if the difference is at all
significant, perhaps the compositor could vend opaque ptrs or handles to the
mutators so that they can quickly look the animated values.

Could you look into creating such a sample page and measuring this?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.h:35: class CC_EXPORT AnimationPlayer : public
base::RefCounted<AnimationPlayer>,
On 2015/05/05 at 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > Why RefCounted?
> 
> It's in shared ownership between blink::AnimationPlayer and
cc::AnimationTimeline/cc::AnimationHost(=Registrar). Same idea as for cc::Layer.

This is probably a dumb question, but can players outlive their timelines or be
created before them?

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:187: void GetImplObjectsByIDs() {
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:07, vollick wrote:
> > nit: I find the name of this function uninformative. It looks like it's
> checking
> > that you've got a timeline and player (and updating the relevant members).
How
> > about EnsureTimelineAndPlayer?
> It brings me impl-side object pointers if only IDs known.
> How about ObtainImplPointersByIDs?

I don't feel strongly about this one, but the bit that seemed vague was
"Objects" in the previous name and "Pointers" in your new name -- they don't
explain what objects or pointers you're getting or obtaining. Just switching the
old name to GetImplTimelineAndPlayerByID feels a bit nicer to me.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:463: EXPECT_TRUE(delegate.started_);
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > Do we need to test started _and_ finished?
> This test is about the delegate accumulation/buffering.
> 
> What you propose is more about integration with LTH. I.e. a part of
> layer_tree_host_unittest_animation_timelines.cc
> 
> But yes, we could introduce some redundancy.

I'm not sure how I understand how this is testing accumulation or buffering. It
looks like the only things that are explicitly checked here are that the
delegate doesn't report that it's started before the call to animate layers, and
does afterwards. This suggested to me that you'd want to test that the delegate
reported that it was finished as well.

If there's more going on in this test, please describe it with a comment.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_timeline.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_timeline.h:27: class CC_EXPORT AnimationTimeline : public
base::RefCounted<AnimationTimeline> {
On 2015/05/05 06:51:18, loyso wrote:
> On 2015/05/05 03:52:07, vollick wrote:
> > Why refptr?
> Shared between cc::AnimationHost and blink::AnimationTimeline. Same as for
> cc::Layer and cc::AnimationPlayer.

IIUC, this means that both the cc::AnimationHost and blink::AnimationTimeline
need to refer to the cc::AnimationTimeline, but we cannot guarantee that the
lifetime of one of these two owners will encompass the lifetime of the other. Is
that true? It seems like the AnimationHost would live longer than any timeline?

[loyso (OOO), 2015-05-08 01:04:35]

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:39: void AddAnimationTimeline(AnimationTimeline*
timeline);
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> I don't think this is true; this appears to compile:
> https://codereview.chromium.org/1131593007
That's true for AnimationTimeline::AttachPlayer(AnimationPlayer* player) case.
web_compositor_animation_timeline_impl.cc can't be compiled w/o 
#include "cc/animation/animation_player.h"
But that's the only case. I agree with you completely - I'll change it to
scoped_refptr.

> 
> > Plus some runtime overhead on passing.
> 
> I think this cost is negligible, and I prefer the clarity/safety of passing
the
> refptr. I know that in this case, you coerce the ptr into a scoped_refptr when
> you add it to the vector, but in general, function(Thingy* thingy) is risky if
> |thingy| is ref counted. If the callee hangs onto the ptr, it can end up with
a
> pointer to nothing if the caller decrements the ref count.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:45: void RegisterLayer(int layer_id, bool
is_active_tree);
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Please use an enum rather than a boolean. It makes the call sites easier
to
> > > read. Here and other places where you use a bool to mean "this is the impl
> > > thread"
> > I wanted to go the LAC way with bools and no extra #include declarations
> > necessary since cc::Layers are deprecated. But yeah, I can add enum class.
> 
> Using bools was probably a mistake with LAC. I would prefer that you switch to
> an enum here, but it's probably not worth updating the LAC to use it since
it's
> hopefully going away.

Acknowledged.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host_unittest.cc:16: TEST(AnimationHostTest,
SyncTimelinesAddRemove) {
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 at 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Is layer/player registration and unregistration tested elsewhere?
> > Yeah, that's in animation_player_unittest.cc
> 
> k, great. Please add a comment to this effect here.

Acknowledged.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:40: // TODO(loyso): implement it.
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Why's it ok to put this off?
> > It's in separate CL for scroll offset.
> > https://codereview.chromium.org/1009233002/patch/140001/150003
> 
> My questions is more about staging. Will the code still be correct before the
> other CL you cite lands?

We don't run this code path w/o --enable_compositor_animation_timelines.
The new mode will not be ready at this stage, of course.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:236:
animation_host()->layer_tree_mutators_client()->SetLayerTransformMutated(
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > If I understand correctly, this will cause us to find layer-by-id in
either
> > LTH
> > > or LTHI and then apply the mutation. Is there any performance impact?
> > Yes, that's a hash_map look up. We discussed that in person, as you might
> > remember. =) Yes, we should pay attention to this and monitor the
performance
> > for any regressions. In general, layer_id represents a set of objects. It
> looks
> > like we can enhance it in the future with a list of subscribers.
> > 
> > The number of players is very low in general.
> > 
> > Moreover, we have a reverse mapping. That's more risky because the number of
> > requests can be huge. See my lonely comment here:
> > https://codereview.chromium.org/1010663002/patch/140001/150001
> 
> I think you're right that we won't have that many players in general, but
having
> hundreds on a highly dynamic page seems like something we should be prepared
> for.
> 
> With this CL, we switch from pointer derefs to hash map lookups and it would
be
> nice to quantify the impact this has on pages with a lot of animated layers.
It
> will hopefully be too small to worry about, but if the difference is at all
> significant, perhaps the compositor could vend opaque ptrs or handles to the
> mutators so that they can quickly look the animated values.
> 
> Could you look into creating such a sample page and measuring this?
May I set this as a separate perf and optimization sub-task(s)? We are not going
to enable the new system for now anyway.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.h:35: class CC_EXPORT AnimationPlayer : public
base::RefCounted<AnimationPlayer>,
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 at 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Why RefCounted?
> > 
> > It's in shared ownership between blink::AnimationPlayer and
> cc::AnimationTimeline/cc::AnimationHost(=Registrar). Same idea as for
cc::Layer.
> 
> This is probably a dumb question, but can players outlive their timelines or
be
> created before them?

Yes, sure. Typically you create them in blink in advance. So you can attach it
to an element and add animations later.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:187: void GetImplObjectsByIDs() {
On 2015/05/07 13:58:43, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:07, vollick wrote:
> > > nit: I find the name of this function uninformative. It looks like it's
> > checking
> > > that you've got a timeline and player (and updating the relevant members).
> How
> > > about EnsureTimelineAndPlayer?
> > It brings me impl-side object pointers if only IDs known.
> > How about ObtainImplPointersByIDs?
> 
> I don't feel strongly about this one, but the bit that seemed vague was
> "Objects" in the previous name and "Pointers" in your new name -- they don't
> explain what objects or pointers you're getting or obtaining. Just switching
the
> old name to GetImplTimelineAndPlayerByID feels a bit nicer to me.

Acknowledged.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:463: EXPECT_TRUE(delegate.started_);
On 2015/05/07 13:58:43, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Do we need to test started _and_ finished?
> > This test is about the delegate accumulation/buffering.
> > 
> > What you propose is more about integration with LTH. I.e. a part of
> > layer_tree_host_unittest_animation_timelines.cc
> > 
> > But yes, we could introduce some redundancy.
> 
> I'm not sure how I understand how this is testing accumulation or buffering.
It
> looks like the only things that are explicitly checked here are that the
> delegate doesn't report that it's started before the call to animate layers,
and
> does afterwards. This suggested to me that you'd want to test that the
delegate
> reported that it was finished as well.
> 
> If there's more going on in this test, please describe it with a comment.

Acknowledged.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_timeline.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_timeline.h:27: class CC_EXPORT AnimationTimeline : public
base::RefCounted<AnimationTimeline> {
On 2015/05/07 13:58:43, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:07, vollick wrote:
> > > Why refptr?
> > Shared between cc::AnimationHost and blink::AnimationTimeline. Same as for
> > cc::Layer and cc::AnimationPlayer.
> 
> IIUC, this means that both the cc::AnimationHost and blink::AnimationTimeline
> need to refer to the cc::AnimationTimeline, but we cannot guarantee that the
> lifetime of one of these two owners will encompass the lifetime of the other.
Is
> that true? It seems like the AnimationHost would live longer than any
timeline?

Yes, that's true. We can create WebCompositorAnimationTimeline object anywhere
in blink, at any stage. Even if the compositor isn't yet initialized/shut down.

[Ian Vollick, 2015-05-08 14:58:23]

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:40: // TODO(loyso): implement it.
On 2015/05/08 01:04:34, loyso wrote:
> On 2015/05/07 13:58:42, vollick wrote:
> > On 2015/05/05 06:51:18, loyso wrote:
> > > On 2015/05/05 03:52:06, vollick wrote:
> > > > Why's it ok to put this off?
> > > It's in separate CL for scroll offset.
> > > https://codereview.chromium.org/1009233002/patch/140001/150003
> > 
> > My questions is more about staging. Will the code still be correct before
the
> > other CL you cite lands?
> 
> We don't run this code path w/o --enable_compositor_animation_timelines.
> The new mode will not be ready at this stage, of course.

K, great.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:236:
animation_host()->layer_tree_mutators_client()->SetLayerTransformMutated(
On 2015/05/08 01:04:34, loyso wrote:
> On 2015/05/07 13:58:42, vollick wrote:
> > On 2015/05/05 06:51:18, loyso wrote:
> > > On 2015/05/05 03:52:06, vollick wrote:
> > > > If I understand correctly, this will cause us to find layer-by-id in
> either
> > > LTH
> > > > or LTHI and then apply the mutation. Is there any performance impact?
> > > Yes, that's a hash_map look up. We discussed that in person, as you might
> > > remember. =) Yes, we should pay attention to this and monitor the
> performance
> > > for any regressions. In general, layer_id represents a set of objects. It
> > looks
> > > like we can enhance it in the future with a list of subscribers.
> > > 
> > > The number of players is very low in general.
> > > 
> > > Moreover, we have a reverse mapping. That's more risky because the number
of
> > > requests can be huge. See my lonely comment here:
> > > https://codereview.chromium.org/1010663002/patch/140001/150001
> > 
> > I think you're right that we won't have that many players in general, but
> having
> > hundreds on a highly dynamic page seems like something we should be prepared
> > for.
> > 
> > With this CL, we switch from pointer derefs to hash map lookups and it would
> be
> > nice to quantify the impact this has on pages with a lot of animated layers.
> It
> > will hopefully be too small to worry about, but if the difference is at all
> > significant, perhaps the compositor could vend opaque ptrs or handles to the
> > mutators so that they can quickly look the animated values.
> > 
> > Could you look into creating such a sample page and measuring this?
> May I set this as a separate perf and optimization sub-task(s)? We are not
going
> to enable the new system for now anyway.

Since these results could affect the design of this CL, they ought to be
gathered before landing this. IIUC, it'll amount to creating a synthetic page w/
a bunch of animations and using a profiler to show that the hash map lookups
either don't show up in the profiles or that the change in hash map cost is
negligible. Do you think the testing will be more involved?

[loyso (OOO), 2015-06-22 07:48:51]

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.cc:92: if (iter != layer_to_player_map_.end() &&
iter->second == player)
On 2015/05/05 03:52:06, vollick wrote:
> when would iter->second != player?
Well, in fact this is an intermediate state of things. There is a TODO(loyso)
comment in animation_host.h for this (near the layer_to_players_map_ member). In
blink we can have many blink::AnimationPlayers per blink::Element. In cc we
re-use the old system for now. It has a limitation - one LAC per 1 Layer. The
temporary solution in these CLs is that cc::AnimationPlayer share the same LAC
if they are attached to same Element's Layer. (See
AnimationPlayer::AddControllerToTimeline ... layer_animation_controller_ =
registrar->GetAnimationControllerForId(layer_id_))

As a result, there is no difference which player is mapped for a given layer_id,
the mapping leads to the same LAC anyway.

I've added PlayersList in the recent PatchSet to make layer-to-player
relationship explicit.

And the plan is to merge LAC into cc::AnimationPlayer and allow different
AnimationPlayers to play same-Layer and same-property animations (like Blink
does). Also, we will need cc::LayerAnimations (a counterpart for
blink::ElementAnimations). This is the unification part.

I'm thinking if I need to introduce cc::LayerAnimations (cc::ElementAnimations?)
in this CL.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:33: // AnimationHost has it's own instance of
AnimationRegistrar (to be merged).
On 2015/04/30 13:17:54, ajuma (OOO till June 16th) wrote:
> Nit: s/it's/its (also above and elsewhere)

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:33: // AnimationHost has it's own instance of
AnimationRegistrar (to be merged).
On 2015/05/05 03:52:06, vollick wrote:
> To be clear, you mean that the AnimationRegistrar will eventually be merged
into
> AnimationHost?

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:39: void AddAnimationTimeline(AnimationTimeline*
timeline);
On 2015/05/05 03:52:06, vollick wrote:
> If it's going to hold a reference to the animation timeline objects, should it
> not pass a scoped_refptr to make transfer of ownership clear?

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:45: void RegisterLayer(int layer_id, bool
is_active_tree);
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Please use an enum rather than a boolean. It makes the call sites easier
to
> > > read. Here and other places where you use a bool to mean "this is the impl
> > > thread"
> > I wanted to go the LAC way with bools and no extra #include declarations
> > necessary since cc::Layers are deprecated. But yeah, I can add enum class.
> 
> Using bools was probably a mistake with LAC. I would prefer that you switch to
> an enum here, but it's probably not worth updating the LAC to use it since
it's
> hopefully going away.

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.h:65: AnimationRegistrar* animation_registrar()
const {
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > This looks like it could be a reference given that the registrar is
created
> in
> > > the ctor and its lifetime is tied to the host's.
> > I preserved the accessor signature from LayerTreeHost/LayerTreeHostImpl
where
> we
> > have identical situation (since AnimationRegistrar is marked for deletion).
> 
> I would prefer the reference, but if this accessor is temporary, then I don't
> have strong feelings.

Let it be a reference (to be deleted)

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host_unittest.cc:16: TEST(AnimationHostTest,
SyncTimelinesAddRemove) {
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 at 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Is layer/player registration and unregistration tested elsewhere?
> > Yeah, that's in animation_player_unittest.cc
> 
> k, great. Please add a comment to this effect here.

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:85: RemoveControllerFromTimeline();
On 2015/05/05 03:52:06, vollick wrote:
> Can you add a comment explaining why you need to remove and add the
controller?

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.cc:236:
animation_host()->layer_tree_mutators_client()->SetLayerTransformMutated(
On 2015/05/08 14:58:22, vollick wrote:
> On 2015/05/08 01:04:34, loyso wrote:
> > On 2015/05/07 13:58:42, vollick wrote:
> > > On 2015/05/05 06:51:18, loyso wrote:
> > > > On 2015/05/05 03:52:06, vollick wrote:
> > > > > If I understand correctly, this will cause us to find layer-by-id in
> > either
> > > > LTH
> > > > > or LTHI and then apply the mutation. Is there any performance impact?
> > > > Yes, that's a hash_map look up. We discussed that in person, as you
might
> > > > remember. =) Yes, we should pay attention to this and monitor the
> > performance
> > > > for any regressions. In general, layer_id represents a set of objects.
It
> > > looks
> > > > like we can enhance it in the future with a list of subscribers.
> > > > 
> > > > The number of players is very low in general.
> > > > 
> > > > Moreover, we have a reverse mapping. That's more risky because the
number
> of
> > > > requests can be huge. See my lonely comment here:
> > > > https://codereview.chromium.org/1010663002/patch/140001/150001
> > > 
> > > I think you're right that we won't have that many players in general, but
> > having
> > > hundreds on a highly dynamic page seems like something we should be
prepared
> > > for.
> > > 
> > > With this CL, we switch from pointer derefs to hash map lookups and it
would
> > be
> > > nice to quantify the impact this has on pages with a lot of animated
layers.
> > It
> > > will hopefully be too small to worry about, but if the difference is at
all
> > > significant, perhaps the compositor could vend opaque ptrs or handles to
the
> > > mutators so that they can quickly look the animated values.
> > > 
> > > Could you look into creating such a sample page and measuring this?
> > May I set this as a separate perf and optimization sub-task(s)? We are not
> going
> > to enable the new system for now anyway.
> 
> Since these results could affect the design of this CL, they ought to be
> gathered before landing this. IIUC, it'll amount to creating a synthetic page
w/
> a bunch of animations and using a profiler to show that the hash map lookups
> either don't show up in the profiles or that the change in hash map cost is
> negligible. Do you think the testing will be more involved?

Done. https://codereview.chromium.org/1172583003/

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.h:35: class CC_EXPORT AnimationPlayer : public
base::RefCounted<AnimationPlayer>,
On 2015/05/07 13:58:42, vollick wrote:
> On 2015/05/05 at 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Why RefCounted?
> > 
> > It's in shared ownership between blink::AnimationPlayer and
> cc::AnimationTimeline/cc::AnimationHost(=Registrar). Same idea as for
cc::Layer.
> 
> This is probably a dumb question, but can players outlive their timelines or
be
> created before them?
Class diagrams added to the design doc.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player_unittest.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:150: class AnimationDelegateTest :
public AnimationDelegate {
On 2015/05/05 03:52:07, vollick wrote:
> Name's a bit confusing; this isn't a test.
> 
> How about TestAnimationDelegate? Here and other places where Test is used as a
> suffix for something that's not a test.

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:187: void GetImplObjectsByIDs() {
On 2015/05/07 13:58:43, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:07, vollick wrote:
> > > nit: I find the name of this function uninformative. It looks like it's
> > checking
> > > that you've got a timeline and player (and updating the relevant members).
> How
> > > about EnsureTimelineAndPlayer?
> > It brings me impl-side object pointers if only IDs known.
> > How about ObtainImplPointersByIDs?
> 
> I don't feel strongly about this one, but the bit that seemed vague was
> "Objects" in the previous name and "Pointers" in your new name -- they don't
> explain what objects or pointers you're getting or obtaining. Just switching
the
> old name to GetImplTimelineAndPlayerByID feels a bit nicer to me.

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:438:
client_impl_.IsPropertyMutated(layer_id_, false, Animation::FILTER));
On 2015/05/05 03:52:07, vollick wrote:
> Should we check that they get mutated to the right values?

Done.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player_unittest.cc:463: EXPECT_TRUE(delegate.started_);
On 2015/05/07 13:58:43, vollick wrote:
> On 2015/05/05 06:51:18, loyso wrote:
> > On 2015/05/05 03:52:06, vollick wrote:
> > > Do we need to test started _and_ finished?
> > This test is about the delegate accumulation/buffering.
> > 
> > What you propose is more about integration with LTH. I.e. a part of
> > layer_tree_host_unittest_animation_timelines.cc
> > 
> > But yes, we could introduce some redundancy.
> 
> I'm not sure how I understand how this is testing accumulation or buffering.
It
> looks like the only things that are explicitly checked here are that the
> delegate doesn't report that it's started before the call to animate layers,
and
> does afterwards. This suggested to me that you'd want to test that the
delegate
> reported that it was finished as well.
> 
> If there's more going on in this test, please describe it with a comment.

Done.

[Ian Vollick, 2015-06-22 13:58:39]

lgtm!

(Thanks especially for the work and care you put into the documentation and
performance testing.)

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_host.cc:92: if (iter != layer_to_player_map_.end() &&
iter->second == player)
On 2015/06/22 at 07:48:51, loyso wrote:
> On 2015/05/05 03:52:06, vollick wrote:
> > when would iter->second != player?
> Well, in fact this is an intermediate state of things. There is a TODO(loyso)
comment in animation_host.h for this (near the layer_to_players_map_ member). In
blink we can have many blink::AnimationPlayers per blink::Element. In cc we
re-use the old system for now. It has a limitation - one LAC per 1 Layer. The
temporary solution in these CLs is that cc::AnimationPlayer share the same LAC
if they are attached to same Element's Layer. (See
AnimationPlayer::AddControllerToTimeline ... layer_animation_controller_ =
registrar->GetAnimationControllerForId(layer_id_))
> 
> As a result, there is no difference which player is mapped for a given
layer_id, the mapping leads to the same LAC anyway.
> 
> I've added PlayersList in the recent PatchSet to make layer-to-player
relationship explicit.
> 
> And the plan is to merge LAC into cc::AnimationPlayer and allow different
AnimationPlayers to play same-Layer and same-property animations (like Blink
does). Also, we will need cc::LayerAnimations (a counterpart for
blink::ElementAnimations). This is the unification part.
> 
> I'm thinking if I need to introduce cc::LayerAnimations
(cc::ElementAnimations?) in this CL.

That sounds reasonable. Thanks for the details.

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/220001/cc/animation/animation_...
cc/animation/animation_player.h:35: class CC_EXPORT AnimationPlayer : public
base::RefCounted<AnimationPlayer>,
On 2015/06/22 07:48:51, loyso wrote:
> On 2015/05/07 13:58:42, vollick wrote:
> > On 2015/05/05 at 06:51:18, loyso wrote:
> > > On 2015/05/05 03:52:06, vollick wrote:
> > > > Why RefCounted?
> > > 
> > > It's in shared ownership between blink::AnimationPlayer and
> > cc::AnimationTimeline/cc::AnimationHost(=Registrar). Same idea as for
> cc::Layer.
> > 
> > This is probably a dumb question, but can players outlive their timelines or
> be
> > created before them?
> Class diagrams added to the design doc.

These diagrams are amazing. Thank you!

https://codereview.chromium.org/947033002/diff/260001/cc/animation/animation_...
File cc/animation/animation_host.h (right):

https://codereview.chromium.org/947033002/diff/260001/cc/animation/animation_...
cc/animation/animation_host.h:25: enum class ThreadInstance { MAIN, IMPL };
This looks much nicer. Thanks!

https://codereview.chromium.org/947033002/diff/260001/cc/animation/animation_...
cc/animation/animation_host.h:95: typedef base::hash_map<int, PlayersList*>
LayerToPlayersMap;
The PlayersList is a good idea -- it does make things much clearer.

[loyso (OOO), 2015-06-23 01:44:45]

RenderWidgetCompositor - two obvious methods added. Owners, please review!

[piman, 2015-06-23 02:01:05]

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

[piman, 2015-06-23 02:01:06]

lgtm

https://codereview.chromium.org/947033002/diff/260001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/947033002/diff/260001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:591:
blink::WebCompositorAnimationTimeline* compositor_timeline) {
nit: should compositor_timeline be a const
blink::WebCompositorAnimationTimeline*? (or const &).

https://codereview.chromium.org/947033002/diff/260001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:600:
blink::WebCompositorAnimationTimeline* compositor_timeline) {
nit: same here.

[loyso (OOO), 2015-06-23 05:22:07]

https://codereview.chromium.org/947033002/diff/260001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/947033002/diff/260001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:591:
blink::WebCompositorAnimationTimeline* compositor_timeline) {
On 2015/06/23 02:01:06, piman (Very slow to review) wrote:
> nit: should compositor_timeline be a const
> blink::WebCompositorAnimationTimeline*? (or const &).
Well, technically we can make it const (plus, it requires blink-side CL to
change the interface).
Semantically, it means that we register blink::WebCompositorAnimationTimeline in
some centralized list by non-const reference, so we can perform mutable
non-const operations in that system (CC compositor).

So I would leave it as-is.

[loyso (OOO), 2015-06-25 07:37:11]

Despite all received LGTMs I decided to introduce cc::ElementAnimations in this
CL (with PlayersList and shared LAC reference in it).
I'll update the diagrams in the design doc.
Also, we want to rename Mutators interfaces.
Stay tuned.

[loyso (OOO), 2015-06-29 07:46:11]

cc animation owners - please, take another pass. It's changed significantly.

[ajuma, 2015-06-29 14:56:20]

Please update the design doc with a description about ElementAnimations (it
might already be there and I'm just missing it; if so, please point me to right
section!).

I'd also find it helpful (given the size of this CL) to have a more detailed
description about the changes between the previous patch set and this one, and
the motivation behind doing this latest set of changes here rather than in a
separate follow-up CL.

https://codereview.chromium.org/947033002/diff/280001/cc/public/mutator_host_...
File cc/public/mutator_host_client.h (right):

https://codereview.chromium.org/947033002/diff/280001/cc/public/mutator_host_...
cc/public/mutator_host_client.h:5: #ifndef CC_PUBLIC_MUTATOR_HOST_CLIENT_H_
Creating a cc/public directory for this seems a bit confusing to me (since it
suggests that all of cc's public interface lives in this directory,  which isn't
actually the case).

[loyso (OOO), 2015-06-30 01:39:40]

On 2015/06/29 14:56:20, ajuma wrote:
> I'd also find it helpful (given the size of this CL) to have a more detailed
> description about the changes between the previous patch set and this one, and
> the motivation behind doing this latest set of changes here rather than in a
> separate follow-up CL.
1) I renamed LayerTreeMutatorsClient to MutatorHostClient and moved the file to
cc/public (discussed with vollick@)
2) I extracted common test code from animation_player_unittest.cc into
cc/test/animation_timelines_test_common.cc/.h
It's used in element_animations_unittest.cc
3) I added ElementAnimations entity to correctly regroup the data members.
Firstly, we must have just two LayerAnimationValueObservers for each
LayerAnimationsController. Having those two observers in each AnimationPlayer
was wrong
since we can have many AnimationPlayers for each Layer. That's why we had ugly
RefreshObservers method on AnimationPlayer detach.
Secondly, the major problem was that LayerAnimationController can have only one
AnimationDelegate and we wanted many AnimationPlayers with AnimationDelegate in
each of them to be attached to that LAC.
We needed some multiplexer to report
NotifyAnimationStarted/NotifyAnimationFinished for all attached
AnimationPlayers.

[loyso (OOO), 2015-06-30 01:39:57]

https://codereview.chromium.org/947033002/diff/280001/cc/public/mutator_host_...
File cc/public/mutator_host_client.h (right):

https://codereview.chromium.org/947033002/diff/280001/cc/public/mutator_host_...
cc/public/mutator_host_client.h:5: #ifndef CC_PUBLIC_MUTATOR_HOST_CLIENT_H_
On 2015/06/29 14:56:19, ajuma wrote:
> Creating a cc/public directory for this seems a bit confusing to me (since it
> suggests that all of cc's public interface lives in this directory,  which
isn't
> actually the case).
We have a plan to accumulate all the external cc interfaces here (see vollick@).
What's wrong?

[Ian Vollick, 2015-06-30 03:45:11]

On 2015/06/30 at 01:39:57, loyso wrote:
>
https://codereview.chromium.org/947033002/diff/280001/cc/public/mutator_host_...
> File cc/public/mutator_host_client.h (right):
> 
>
https://codereview.chromium.org/947033002/diff/280001/cc/public/mutator_host_...
> cc/public/mutator_host_client.h:5: #ifndef CC_PUBLIC_MUTATOR_HOST_CLIENT_H_
> On 2015/06/29 14:56:19, ajuma wrote:
> > Creating a cc/public directory for this seems a bit confusing to me (since
it
> > suggests that all of cc's public interface lives in this directory,  which
isn't
> > actually the case).
> We have a plan to accumulate all the external cc interfaces here (see
vollick@). What's wrong?

Hey Alexey. Sorry for steering you in the wrong direction. I would indeed love
to see us consolidate our external interfaces in a public/ directory, but the
idea hasn't been discussed with all the cc folks and hasn't been agreed upon. I
think these files should go in the usual spots for now. If a proposal gets sent
to graphics-dev and approved, we could move all the interfaces at once in a
future CL.

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

> Hey Alexey. Sorry for steering you in the wrong direction. I would indeed love
> to see us consolidate our external interfaces in a public/ directory, but the
> idea hasn't been discussed with all the cc folks and hasn't been agreed upon.
I
> think these files should go in the usual spots for now. If a proposal gets
sent
> to graphics-dev and approved, we could move all the interfaces at once in a
> future CL.

No problem, Ian. I'll just revert the change in my local branch. I made it
isolated.

[loyso (OOO), 2015-06-30 08:10:51]

On 2015/06/29 14:56:20, ajuma wrote:
> Please update the design doc with a description about ElementAnimations (it
> might already be there and I'm just missing it; if so, please point me to
right
> section!).
Done.

[ajuma, 2015-06-30 15:56:55]

Thanks for the description of changes, I found that very helpful.

https://codereview.chromium.org/947033002/diff/300001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/947033002/diff/300001/cc/BUILD.gn#newcode509
cc/BUILD.gn:509: "trees/layer_tree_mutators_client.h",
mutator_host_client.h?

https://codereview.chromium.org/947033002/diff/300001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/300001/cc/animation/animation_...
cc/animation/animation_host.cc:96: std::make_pair(layer_id,
element_animations));
Who owns |element_animations|? Please use an appropriate smart pointer, rather
than depending on a manual call to |delete|.

https://codereview.chromium.org/947033002/diff/300001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/947033002/diff/300001/cc/cc.gyp#newcode566
cc/cc.gyp:566: 'trees/layer_tree_mutators_client.h',
mutator_host_client.h?

https://codereview.chromium.org/947033002/diff/300001/cc/test/animation_timel...
File cc/test/animation_timelines_test_common.cc (right):

https://codereview.chromium.org/947033002/diff/300001/cc/test/animation_timel...
cc/test/animation_timelines_test_common.cc:44: delete kv.second;
Can we express ownership using smart pointers, rather than using raw pointers
and depending on manual deletion? For example, can we make these hash maps store
scoped_ptrs?

[loyso (OOO), 2015-07-01 02:25:47]

https://codereview.chromium.org/947033002/diff/300001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/947033002/diff/300001/cc/BUILD.gn#newcode509
cc/BUILD.gn:509: "trees/layer_tree_mutators_client.h",
On 2015/06/30 15:56:55, ajuma (OOO July 1) wrote:
> mutator_host_client.h?

Done.

https://codereview.chromium.org/947033002/diff/300001/cc/animation/animation_...
File cc/animation/animation_host.cc (right):

https://codereview.chromium.org/947033002/diff/300001/cc/animation/animation_...
cc/animation/animation_host.cc:96: std::make_pair(layer_id,
element_animations));
On 2015/06/30 15:56:55, ajuma (OOO July 1) wrote:
> Who owns |element_animations|? Please use an appropriate smart pointer, rather
> than depending on a manual call to |delete|.

Done.

https://codereview.chromium.org/947033002/diff/300001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/947033002/diff/300001/cc/cc.gyp#newcode566
cc/cc.gyp:566: 'trees/layer_tree_mutators_client.h',
On 2015/06/30 15:56:55, ajuma (OOO July 1) wrote:
> mutator_host_client.h?

Done.

https://codereview.chromium.org/947033002/diff/300001/cc/test/animation_timel...
File cc/test/animation_timelines_test_common.cc (right):

https://codereview.chromium.org/947033002/diff/300001/cc/test/animation_timel...
cc/test/animation_timelines_test_common.cc:44: delete kv.second;
On 2015/06/30 15:56:55, ajuma (OOO July 1) wrote:
> Can we express ownership using smart pointers, rather than using raw pointers
> and depending on manual deletion? For example, can we make these hash maps
store
> scoped_ptrs?

Done.

[Ian Vollick, 2015-07-01 03:03:29]

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
cc/animation/animation_player.cc:111: if (!animations_.empty())
When will this be the case?

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
cc/animation/animation_player.h:95: AnimationList animations_;
If I'm understanding correctly, animations_ here is a contains Animations that
need to be "bound" to an ElementAnimations? And in any case, can you please add
a comment here explaining this member and how it differs from
element_animations_ below?

https://codereview.chromium.org/947033002/diff/320001/cc/animation/element_an...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/947033002/diff/320001/cc/animation/element_an...
cc/animation/element_animations.cc:46: ElementAnimations* element_animation_;
nit: element_animations_;

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

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
File cc/animation/animation_player.cc (right):

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
cc/animation/animation_player.cc:111: if (!animations_.empty())
On 2015/07/01 03:03:29, vollick wrote:
> When will this be the case?
|take| doesn't erase elements. It makes the element value to be nullptr.
If we add one or more animations, we call SetNeedsCommit.
We don't call SetNeedsCommit if no animations added.

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/947033002/diff/320001/cc/animation/animation_...
cc/animation/animation_player.h:95: AnimationList animations_;
On 2015/07/01 03:03:29, vollick wrote:
> If I'm understanding correctly, animations_ here is a contains Animations that
> need to be "bound" to an ElementAnimations? 
We create ElementAnimations+LAC objects later on attachment to the particular
layer. We accumulate added animations in animations_ container if
element_animations_ is a nullptr. It allows us to add animations to non-attached
AnimationPlayers.

> And in any case, can you please add
> a comment here explaining this member and how it differs from
> element_animations_ below?
Done.

https://codereview.chromium.org/947033002/diff/320001/cc/animation/element_an...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/947033002/diff/320001/cc/animation/element_an...
cc/animation/element_animations.cc:46: ElementAnimations* element_animation_;
On 2015/07/01 03:03:29, vollick wrote:
> nit: element_animations_;

Done.

[ajuma, 2015-07-02 13:05:21]

Thanks, lgtm % vollick.

[Ian Vollick, 2015-07-02 13:10:20]

On 2015/07/02 13:05:21, ajuma (OOO July 1) wrote:
> Thanks, lgtm % vollick.

lgtm 2.

[loyso (OOO), 2015-07-03 00:15:45]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-07-03 00:15:46]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/947033002/#ps380001
(title: "Fix MSVC warning.")

[commit-bot: I haz the power, 2015-07-03 00:16:02]

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

[commit-bot: I haz the power, 2015-07-03 00:19:58]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-07-03 00:21:05]

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