Animations: Introduce compositor AnimationPlayer and AnimationTimeline.

Animations: Introduce compositor AnimationPlayer and AnimationTimeline.

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 compositor representations for AnimaitonPlayer and AnimationTimeline.
- We add/remove animations to compositor AnimaitonPlayer from now.

A blink part. Chromium part: https://codereview.chromium.org/947033002/

BUG=394777
R=dstockwell@chromium.org
R=shane@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192909

[dstockwell, 2015-02-26 00:07:12]

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.cpp:97: createCompositorPlayer();
Why do we always create a compositor player? Seems like a waste of
time/resources.

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:160: void notifyAnimationFinished(double
monotonicTime, int group) override;
Is this just for consistency? Do we need it?

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:162: void createCompositorPlayer();
These should probably not be public.

[loyso (OOO), 2015-02-26 03:24:57]

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.cpp:97: createCompositorPlayer();
On 2015/02/26 00:07:12, dstockwell wrote:
> Why do we always create a compositor player? Seems like a waste of
> time/resources.
Because draft. In reality we must know beforehand, whether we need it. (If it
has compositor-compatible animations?)

I will hide it behind runtime flag (maybe, temporary).

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:160: void notifyAnimationFinished(double
monotonicTime, int group) override;
On 2015/02/26 00:07:12, dstockwell wrote:
> Is this just for consistency? Do we need it?
That's already existing interface with pure abstract function. We must override
it :(

https://codereview.chromium.org/946323002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationPlayer.h:162: void createCompositorPlayer();
On 2015/02/26 00:07:12, dstockwell wrote:
> These should probably not be public.
Sure! This is a side-effect of the draft version.

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

loyso@chromium.org changed reviewers:
+ aelias@chromium.org, jochen@chromium.org

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

aelias@chromium.org: Please review changes in 
jochen@chromium.org: Please review changes in 

This is DRAFT, not to be landed. Just a conceptual overview needed. Thanks!

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

loyso@chromium.org changed reviewers:
- ajuma@chromium.org, shane@chromium.org, vollick@chromium.org

[loyso (OOO), 2015-03-09 02:57:47]

On 2015/03/06 01:53:26, loyso wrote:

PTAL! This is not a draft anymore.

[loyso (OOO), 2015-03-09 03:27:09]

loyso@chromium.org changed reviewers:
+ sigbjornf@opera.com

[loyso (OOO), 2015-03-09 03:28:20]

Sigbjørn, Oilpan review please? :)

[sof, 2015-03-11 11:04:24]

Sorry about the delay; Oilpan lgtm.

[aelias_OOO_until_Jul13, 2015-03-13 00:38:32]

https://codereview.chromium.org/946323002/diff/80001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/946323002/diff/80001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:36: CompositorAnimationTimelines
I don't think we should have a permanent setting for this.  This is intended to
be deleted after the refactoring is complete, right?

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:702:
m_webView->setCompositorAnimationTimeline(compositorTimeline);
This looks out of place here, a better place might be in
WebViewImpl::initializeLayerTreeView()?

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

https://codereview.chromium.org/946323002/diff/80001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/946323002/diff/80001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:36: CompositorAnimationTimelines
On 2015/03/13 00:38:32, aelias wrote:
> I don't think we should have a permanent setting for this.  This is intended
to
> be deleted after the refactoring is complete, right?
Right, it will be deleted after the migration (long term refactoring).

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:702:
m_webView->setCompositorAnimationTimeline(compositorTimeline);
On 2015/03/13 00:38:32, aelias wrote:
> This looks out of place here, a better place might be in
> WebViewImpl::initializeLayerTreeView()?

Not sure how to do that.
1) CompositorTimeline object is owned by blink::Timeline (DocumentTimeline in
the future)
2) We want to swap them as we do for WebLayer's
3) There will be many Timeline objects.

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

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:702:
m_webView->setCompositorAnimationTimeline(compositorTimeline);
On 2015/03/13 at 01:07:01, loyso wrote:
> On 2015/03/13 00:38:32, aelias wrote:
> > This looks out of place here, a better place might be in
> > WebViewImpl::initializeLayerTreeView()?
> 
> Not sure how to do that.
> 1) CompositorTimeline object is owned by blink::Timeline (DocumentTimeline in
the future)
> 2) We want to swap them as we do for WebLayer's
> 3) There will be many Timeline objects.

Sorry, I don't follow right now how these points relate to whether this
initialization-time code should happen here or in initializeLayerTreeView.  I
think moving this call would still work regardless, are you talking about later
stages of the refactoring?  It doesn't belong in attachRootGraphicsLayer in any
case since it has nothing to do with the root graphics layer.

[loyso (OOO), 2015-03-13 03:37:42]

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:702:
m_webView->setCompositorAnimationTimeline(compositorTimeline);
On 2015/03/13 01:31:18, aelias wrote:
> On 2015/03/13 at 01:07:01, loyso wrote:
> > On 2015/03/13 00:38:32, aelias wrote:
> > > This looks out of place here, a better place might be in
> > > WebViewImpl::initializeLayerTreeView()?
> > 
> > Not sure how to do that.
> > 1) CompositorTimeline object is owned by blink::Timeline (DocumentTimeline
in
> the future)
> > 2) We want to swap them as we do for WebLayer's
> > 3) There will be many Timeline objects.
> 
> Sorry, I don't follow right now how these points relate to whether this
> initialization-time code should happen here or in initializeLayerTreeView.  I
> think moving this call would still work regardless, are you talking about
later
> stages of the refactoring?  It doesn't belong in attachRootGraphicsLayer in
any
> case since it has nothing to do with the root graphics layer.

I want compositor timeline to be attached/detached within
Document::attach/Document::detach scope. That's not purely initialization-time
code.

The question: where can I take a pointer to WebCompositorAnimationTimeline
object for a given Document (i.e. LocalFrame* localRoot)?

page()->mainFrame() isn't yet defined in WebViewImpl::initializeLayerTreeView().

Apart from that, you are right that it doesn't belong to attachRootGraphicsLayer
in any
case. I could plumb
ChromeClientImpl::attachCompositorAnimationTimeline(WebCompositorAnimationTimeline*,
LocalFrame* localRoot) to call it from Document::attach/Document::detach scope.
Any help how to route this would be much appreciated!

[dstockwell, 2015-03-16 00:11:27]

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:843: if
(Platform::current()->compositorSupport() &&
RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled()) {
For consistency I think it's best to check the runtime flag first.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:845: if (m_compositorPlayer)
How can this be null?

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:859:
timeline()->document()->compositorPendingAnimations().notifyCompositorAnimationStarted(monotonicTime,
group);
ASSERT compositorAnimationTimelinesEnabled()

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:197: void notifyAnimationFinished(double
monotonicTime, int group) override { }
We should just not add this.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.h (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.h:58: virtual void
pauseAnimationForTestingOnCompositor(const Element&, const AnimationPlayer*, int
id, double pauseTime);
can't this be const AnimationPlayer& ?

[aelias_OOO_until_Jul13, 2015-03-17 18:12:27]

Source/web lgtm

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:702:
m_webView->setCompositorAnimationTimeline(compositorTimeline);
On 2015/03/13 at 03:37:42, loyso wrote:
> On 2015/03/13 01:31:18, aelias wrote:
> > On 2015/03/13 at 01:07:01, loyso wrote:
> > > On 2015/03/13 00:38:32, aelias wrote:
> > > > This looks out of place here, a better place might be in
> > > > WebViewImpl::initializeLayerTreeView()?
> > > 
> > > Not sure how to do that.
> > > 1) CompositorTimeline object is owned by blink::Timeline (DocumentTimeline
in
> > the future)
> > > 2) We want to swap them as we do for WebLayer's
> > > 3) There will be many Timeline objects.
> > 
> > Sorry, I don't follow right now how these points relate to whether this
> > initialization-time code should happen here or in initializeLayerTreeView. 
I
> > think moving this call would still work regardless, are you talking about
later
> > stages of the refactoring?  It doesn't belong in attachRootGraphicsLayer in
any
> > case since it has nothing to do with the root graphics layer.
> 
> I want compositor timeline to be attached/detached within
Document::attach/Document::detach scope. That's not purely initialization-time
code.
> 
> The question: where can I take a pointer to WebCompositorAnimationTimeline
object for a given Document (i.e. LocalFrame* localRoot)?
> 
> page()->mainFrame() isn't yet defined in
WebViewImpl::initializeLayerTreeView().
> 
> Apart from that, you are right that it doesn't belong to
attachRootGraphicsLayer in any
> case. I could plumb
ChromeClientImpl::attachCompositorAnimationTimeline(WebCompositorAnimationTimeline*,
LocalFrame* localRoot) to call it from Document::attach/Document::detach scope.
Any help how to route this would be much appreciated!

OK, looking at that, the plumbing would be pretty redundant with this, so I
guess it's fine to leave here.

[loyso (OOO), 2015-03-18 06:59:43]

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:843: if
(Platform::current()->compositorSupport() &&
RuntimeEnabledFeatures::compositorAnimationTimelinesEnabled()) {
On 2015/03/16 00:11:27, dstockwell wrote:
> For consistency I think it's best to check the runtime flag first.

Done.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:845: if (m_compositorPlayer)
On 2015/03/16 00:11:27, dstockwell wrote:
> How can this be null?
If chromium side isn't landed and we run with
--enable-compositor-animation-timelines.
I could put ASSERT here.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:859:
timeline()->document()->compositorPendingAnimations().notifyCompositorAnimationStarted(monotonicTime,
group);
On 2015/03/16 00:11:27, dstockwell wrote:
> ASSERT compositorAnimationTimelinesEnabled()

Done.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:197: void notifyAnimationFinished(double
monotonicTime, int group) override { }
On 2015/03/16 00:11:27, dstockwell wrote:
> We should just not add this.
I didn't want to change WebCompositorAnimationDelegate. This function is
declared as pure virtual there.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.h (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.h:58: virtual void
pauseAnimationForTestingOnCompositor(const Element&, const AnimationPlayer*, int
id, double pauseTime);
On 2015/03/16 00:11:27, dstockwell wrote:
> can't this be const AnimationPlayer& ?

Done.

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/web/ChromeClientI...
Source/web/ChromeClientImpl.cpp:702:
m_webView->setCompositorAnimationTimeline(compositorTimeline);
On 2015/03/17 18:12:27, aelias wrote:
> On 2015/03/13 at 03:37:42, loyso wrote:
> > On 2015/03/13 01:31:18, aelias wrote:
> > > On 2015/03/13 at 01:07:01, loyso wrote:
> > > > On 2015/03/13 00:38:32, aelias wrote:
> > > > > This looks out of place here, a better place might be in
> > > > > WebViewImpl::initializeLayerTreeView()?
> > > > 
> > > > Not sure how to do that.
> > > > 1) CompositorTimeline object is owned by blink::Timeline
(DocumentTimeline
> in
> > > the future)
> > > > 2) We want to swap them as we do for WebLayer's
> > > > 3) There will be many Timeline objects.
> > > 
> > > Sorry, I don't follow right now how these points relate to whether this
> > > initialization-time code should happen here or in initializeLayerTreeView.

> I
> > > think moving this call would still work regardless, are you talking about
> later
> > > stages of the refactoring?  It doesn't belong in attachRootGraphicsLayer
in
> any
> > > case since it has nothing to do with the root graphics layer.
> > 
> > I want compositor timeline to be attached/detached within
> Document::attach/Document::detach scope. That's not purely initialization-time
> code.
> > 
> > The question: where can I take a pointer to WebCompositorAnimationTimeline
> object for a given Document (i.e. LocalFrame* localRoot)?
> > 
> > page()->mainFrame() isn't yet defined in
> WebViewImpl::initializeLayerTreeView().
> > 
> > Apart from that, you are right that it doesn't belong to
> attachRootGraphicsLayer in any
> > case. I could plumb
>
ChromeClientImpl::attachCompositorAnimationTimeline(WebCompositorAnimationTimeline*,
> LocalFrame* localRoot) to call it from Document::attach/Document::detach
scope.
> Any help how to route this would be much appreciated!
> 
> OK, looking at that, the plumbing would be pretty redundant with this, so I
> guess it's fine to leave here.

Ok. But I could put a note or TODO comment here.

[dstockwell, 2015-03-18 07:17:18]

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:845: if (m_compositorPlayer)
On 2015/03/18 at 06:59:43, loyso wrote:
> On 2015/03/16 00:11:27, dstockwell wrote:
> > How can this be null?
> If chromium side isn't landed and we run with
--enable-compositor-animation-timelines.
> I could put ASSERT here.

Yes it sounds like an invalid configuration, just assert.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:197: void notifyAnimationFinished(double
monotonicTime, int group) override { }
On 2015/03/18 at 06:59:43, loyso wrote:
> On 2015/03/16 00:11:27, dstockwell wrote:
> > We should just not add this.
> I didn't want to change WebCompositorAnimationDelegate. This function is
declared as pure virtual there.

Ahh, I see. Are you planning to keep WebCompositorAnimationDelegate? These
functions could move to WebCompositorAnimationPlayerClient.

[loyso (OOO), 2015-03-19 02:32:24]

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:845: if (m_compositorPlayer)
On 2015/03/18 07:17:18, dstockwell wrote:
> On 2015/03/18 at 06:59:43, loyso wrote:
> > On 2015/03/16 00:11:27, dstockwell wrote:
> > > How can this be null?
> > If chromium side isn't landed and we run with
> --enable-compositor-animation-timelines.
> > I could put ASSERT here.
> 
> Yes it sounds like an invalid configuration, just assert.

Done.

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.h:197: void notifyAnimationFinished(double
monotonicTime, int group) override { }
On 2015/03/18 07:17:18, dstockwell wrote:
> On 2015/03/18 at 06:59:43, loyso wrote:
> > On 2015/03/16 00:11:27, dstockwell wrote:
> > > We should just not add this.
> > I didn't want to change WebCompositorAnimationDelegate. This function is
> declared as pure virtual there.
> 
> Ahh, I see. Are you planning to keep WebCompositorAnimationDelegate? These
> functions could move to WebCompositorAnimationPlayerClient.

It's a part of old system. After the migration we merge it into
WebCompositorAnimationPlayerClient, yes.

[loyso (OOO), 2015-03-19 03:57:25]

dstockwell@, a major change uploaded (Patch Set 6). Notice, I can move:
1) m_content->attach(this) into attachElementToPlayer()
2) m_content->detach(this) into detachElementToPlayer()
... and rename them. What do you think?

[loyso (OOO), 2015-03-20 03:30:34]

PTAL? This is a blocker.

[dstockwell, 2015-03-20 04:26:38]

Hmm, I'm not really sure about attachElementToPlayer, player's shouldn't know
about elements. Won't there be a mapping for Animation/AnimationNode where we
can do this wiring?

[loyso (OOO), 2015-03-20 05:07:29]

On 2015/03/20 04:26:38, dstockwell wrote:
> Hmm, I'm not really sure about attachElementToPlayer, player's shouldn't know
> about elements. Won't there be a mapping for Animation/AnimationNode where we
> can do this wiring?

Yeah, AnimationPlayer can be attached to many Elements indirectly (in the
future).
I could rename it to AnimationPlayer::attachAnimationsToElements and
Animation::attachToCompositorPlayer.

[dstockwell, 2015-03-22 23:04:28]

On 2015/03/20 at 05:07:29, loyso wrote:
> On 2015/03/20 04:26:38, dstockwell wrote:
> > Hmm, I'm not really sure about attachElementToPlayer, player's shouldn't
know
> > about elements. Won't there be a mapping for Animation/AnimationNode where
we
> > can do this wiring?
> 
> Yeah, AnimationPlayer can be attached to many Elements indirectly (in the
future).
> I could rename it to AnimationPlayer::attachAnimationsToElements and
Animation::attachToCompositorPlayer.

Seems this is more about attaching to layers rather than elements, so why not
player::attachCompositedLayers ?

[dstockwell, 2015-03-22 23:07:28]

https://codereview.chromium.org/946323002/diff/100001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.h (right):

https://codereview.chromium.org/946323002/diff/100001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.h:279: bool
m_attachCompositorPlayerToElement;
Couldn't we tie this attachment to the lifetime of the compositor player? Don't
create the compositor player until we can attach?

I'm still not sure why we create compositor players for all players, even those
which will never run on the compositor.

[loyso (OOO), 2015-03-24 02:50:56]

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:845: if (m_compositorPlayer)
On 2015/03/18 07:17:18, dstockwell wrote:
> On 2015/03/18 at 06:59:43, loyso wrote:
> > On 2015/03/16 00:11:27, dstockwell wrote:
> > > How can this be null?
> > If chromium side isn't landed and we run with
> --enable-compositor-animation-timelines.
> > I could put ASSERT here.
> 
> Yes it sounds like an invalid configuration, just assert.

I recall now! In webkit_unit_tests it crashes. We need 'if' here.

[loyso (OOO), 2015-03-24 02:55:17]

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/80001/Source/core/animation/An...
Source/core/animation/AnimationPlayer.cpp:845: if (m_compositorPlayer)
On 2015/03/24 02:50:56, loyso wrote:
> On 2015/03/18 07:17:18, dstockwell wrote:
> > On 2015/03/18 at 06:59:43, loyso wrote:
> > > On 2015/03/16 00:11:27, dstockwell wrote:
> > > > How can this be null?
> > > If chromium side isn't landed and we run with
> > --enable-compositor-animation-timelines.
> > > I could put ASSERT here.
> > 
> > Yes it sounds like an invalid configuration, just assert.
> 
> I recall now! In webkit_unit_tests it crashes. We need 'if' here.

UPDATE: It's ok since we have a flag now :)

[loyso (OOO), 2015-03-24 06:30:15]

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:283: if
(isCandidateForAnimationOnCompositor()) {
Do we need to delete it on cancellation? I'm afraid of frequent reallocations.

[dstockwell, 2015-03-26 03:04:39]

Sorry, didn't realize you had finished responding to my comments. Please reply
with "Done." so I know.

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:283: if
(isCandidateForAnimationOnCompositor()) {
On 2015/03/24 at 06:30:15, loyso wrote:
> Do we need to delete it on cancellation? I'm afraid of frequent reallocations.

I think we should. We can revisit if necessary. I don't think this is a common
case, starting new players should happen far more often than replaying existing
players.

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:463: attachCompositedLayers();
Is this really the correct time to do this? Compositing information is not up to
date until precommit.

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:898: m_attachCompositedLayers = true;
m_compositedLayersAttached

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationTimeline.cpp:171: for (AnimationPlayer* player :
m_players)
m_playersNeedingUpdate?

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.cpp:349: if (!layer ||
!layer->isAllowedToQueryCompositingState() || !layer->compositedLayerMapping()
|| !layer->compositedLayerMapping()->mainGraphicsLayer())
I think isAllowedToQueryCompositingState being false means we're calling this at
the wrong time.

[loyso (OOO), 2015-03-26 23:36:14]

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:283: if
(isCandidateForAnimationOnCompositor()) {
On 2015/03/26 03:04:39, dstockwell wrote:
> On 2015/03/24 at 06:30:15, loyso wrote:
> > Do we need to delete it on cancellation? I'm afraid of frequent
reallocations.
> 
> I think we should. We can revisit if necessary. I don't think this is a common
> case, starting new players should happen far more often than replaying
existing
> players.

Acknowledged.

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:463: attachCompositedLayers();
On 2015/03/26 03:04:39, dstockwell wrote:
> Is this really the correct time to do this? Compositing information is not up
to
> date until precommit.
If compositing information isn't ready, the m_attachCompositedLayers flag stays
switched on. Which is basically a deferred command "attach composited layers".
It needs a better name, really. How about m_needsCompositedLayersAttachment?

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:898: m_attachCompositedLayers = true;
On 2015/03/26 03:04:39, dstockwell wrote:
> m_compositedLayersAttached

It's a command, or 'dirty' flag. Not a sign that event happened. See those extra
2 comments around.

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/C...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/C...
Source/core/animation/CompositorAnimations.cpp:349: if (!layer ||
!layer->isAllowedToQueryCompositingState() || !layer->compositedLayerMapping()
|| !layer->compositedLayerMapping()->mainGraphicsLayer())
On 2015/03/26 03:04:39, dstockwell wrote:
> I think isAllowedToQueryCompositingState being false means we're calling this
at
> the wrong time.
It allows us to defer the actual attachment.

[dstockwell, 2015-03-27 00:27:22]

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:898: m_attachCompositedLayers = true;
On 2015/03/26 23:36:14, loyso wrote:
> On 2015/03/26 03:04:39, dstockwell wrote:
> > m_compositedLayersAttached
> 
> It's a command, or 'dirty' flag. Not a sign that event happened. See those
extra
> 2 comments around.

It seems wrong that this method is called attachCompositedLayers, but it does
may not actually attach the layers. Perhaps, setNeedsCompositeLayerAttachment

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:899:
updateCompositedLayersAttachment();
So, why do we need to attempt do the attachment at this point? Why not wait
until after the compositing update in preCommit?

[dstockwell, 2015-03-27 00:30:58]

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
Source/web/ChromeClientImpl.cpp:697: WebCompositorAnimationTimeline*
compositorTimeline = rootLayer && localRoot->document() ?
localRoot->document()->timeline().compositorTimeline() : nullptr;
Hmm, what's going on here exactly? Why is this timeline special?

[loyso (OOO), 2015-03-27 00:53:39]

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
Source/web/ChromeClientImpl.cpp:697: WebCompositorAnimationTimeline*
compositorTimeline = rootLayer && localRoot->document() ?
localRoot->document()->timeline().compositorTimeline() : nullptr;
On 2015/03/27 00:30:58, dstockwell wrote:
> Hmm, what's going on here exactly? Why is this timeline special?
We had a conversation with aelias@ there:
https://codereview.chromium.org/946323002/patch/80001/90009
Basically, we have just one document timeline for blink document. In cc that
timeline counterpart is attached to LayerTreeHost, the same way as root layer is
attached.

There is a plan to plumb separate
attachCompositorTimeline/detachCompositorTimeline once we have a collection of
timelines in blink.

[dstockwell, 2015-03-27 01:01:20]

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
Source/web/ChromeClientImpl.cpp:697: WebCompositorAnimationTimeline*
compositorTimeline = rootLayer && localRoot->document() ?
localRoot->document()->timeline().compositorTimeline() : nullptr;
On 2015/03/27 at 00:53:39, loyso wrote:
> On 2015/03/27 00:30:58, dstockwell wrote:
> > Hmm, what's going on here exactly? Why is this timeline special?
> We had a conversation with aelias@ there:
https://codereview.chromium.org/946323002/patch/80001/90009
> Basically, we have just one document timeline for blink document. In cc that
timeline counterpart is attached to LayerTreeHost, the same way as root layer is
attached.
> 
> There is a plan to plumb separate
attachCompositorTimeline/detachCompositorTimeline once we have a collection of
timelines in blink.

Is attachRootGraphicsLayer called for every frame?

[loyso (OOO), 2015-03-27 01:02:42]

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:899:
updateCompositedLayersAttachment();
On 2015/03/27 00:27:22, dstockwell wrote:
> So, why do we need to attempt do the attachment at this point? Why not wait
> until after the compositing update in preCommit?

Preface: we had to attach the layer too early in a previous patch-set.

We may want to attach layer in two scenarios:
1) when compositing information isn't available yet, so we want it ot be
attached ASAP (on next preCommit)
2) when compositing information is in place and we are allowed to query it.

If we make it always deferred (case 1) it may complicate some sub-systems and
make attachments/additions of animations scattered across the time and code. I
need to migrate a couple of blink subsystems (LinkHighlight and
ProgrammaticScrollAnimator).

I could make these scenarios explicit with two functions.
Or we could go with always async way (attach on next preCommit).

[loyso (OOO), 2015-03-27 01:11:10]

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
Source/web/ChromeClientImpl.cpp:697: WebCompositorAnimationTimeline*
compositorTimeline = rootLayer && localRoot->document() ?
localRoot->document()->timeline().compositorTimeline() : nullptr;
On 2015/03/27 01:01:19, dstockwell wrote:
> On 2015/03/27 at 00:53:39, loyso wrote:
> > On 2015/03/27 00:30:58, dstockwell wrote:
> > > Hmm, what's going on here exactly? Why is this timeline special?
> > We had a conversation with aelias@ there:
> https://codereview.chromium.org/946323002/patch/80001/90009
> > Basically, we have just one document timeline for blink document. In cc that
> timeline counterpart is attached to LayerTreeHost, the same way as root layer
is
> attached.
> > 
> > There is a plan to plumb separate
> attachCompositorTimeline/detachCompositorTimeline once we have a collection of
> timelines in blink.
> 
> Is attachRootGraphicsLayer called for every frame?
No, it's not. It strictly matches Document attach/detach scope
(attachRootGraphicsLayer non-null/attachRootGraphicsLayer nullptr).

[dstockwell, 2015-03-27 01:29:52]

On 2015/03/27 at 01:11:10, loyso wrote:
>
https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
> File Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
> Source/web/ChromeClientImpl.cpp:697: WebCompositorAnimationTimeline*
compositorTimeline = rootLayer && localRoot->document() ?
localRoot->document()->timeline().compositorTimeline() : nullptr;
> On 2015/03/27 01:01:19, dstockwell wrote:
> > On 2015/03/27 at 00:53:39, loyso wrote:
> > > On 2015/03/27 00:30:58, dstockwell wrote:
> > > > Hmm, what's going on here exactly? Why is this timeline special?
> > > We had a conversation with aelias@ there:
> > https://codereview.chromium.org/946323002/patch/80001/90009
> > > Basically, we have just one document timeline for blink document. In cc
that
> > timeline counterpart is attached to LayerTreeHost, the same way as root
layer is
> > attached.
> > > 
> > > There is a plan to plumb separate
> > attachCompositorTimeline/detachCompositorTimeline once we have a collection
of
> > timelines in blink.
> > 
> > Is attachRootGraphicsLayer called for every frame?
> No, it's not. It strictly matches Document attach/detach scope
(attachRootGraphicsLayer non-null/attachRootGraphicsLayer nullptr).

Sorry, I don't know anything about this. If we have a tree of frames (thus
multiple documents) are we attaching all of the timelines?

[loyso (OOO), 2015-03-27 02:23:00]

On 2015/03/27 01:29:52, dstockwell wrote:
> On 2015/03/27 at 01:11:10, loyso wrote:
> >
>
https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
> > File Source/web/ChromeClientImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/946323002/diff/120001/Source/web/ChromeClient...
> > Source/web/ChromeClientImpl.cpp:697: WebCompositorAnimationTimeline*
> compositorTimeline = rootLayer && localRoot->document() ?
> localRoot->document()->timeline().compositorTimeline() : nullptr;
> > On 2015/03/27 01:01:19, dstockwell wrote:
> > > On 2015/03/27 at 00:53:39, loyso wrote:
> > > > On 2015/03/27 00:30:58, dstockwell wrote:
> > > > > Hmm, what's going on here exactly? Why is this timeline special?
> > > > We had a conversation with aelias@ there:
> > > https://codereview.chromium.org/946323002/patch/80001/90009
> > > > Basically, we have just one document timeline for blink document. In cc
> that
> > > timeline counterpart is attached to LayerTreeHost, the same way as root
> layer is
> > > attached.
> > > > 
> > > > There is a plan to plumb separate
> > > attachCompositorTimeline/detachCompositorTimeline once we have a
collection
> of
> > > timelines in blink.
> > > 
> > > Is attachRootGraphicsLayer called for every frame?
> > No, it's not. It strictly matches Document attach/detach scope
> (attachRootGraphicsLayer non-null/attachRootGraphicsLayer nullptr).
> 
> Sorry, I don't know anything about this. If we have a tree of frames (thus
> multiple documents) are we attaching all of the timelines?

I know even less than you, i.e. almost nothing :) I'll examine that in detail.
Meanwhile, can we add a reviewer to shed a light?

[dstockwell, 2015-03-27 02:46:08]

On 2015/03/27 at 01:02:42, loyso wrote:
>
https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
> File Source/core/animation/AnimationPlayer.cpp (right):
> 
>
https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
> Source/core/animation/AnimationPlayer.cpp:899:
updateCompositedLayersAttachment();
> On 2015/03/27 00:27:22, dstockwell wrote:
> > So, why do we need to attempt do the attachment at this point? Why not wait
> > until after the compositing update in preCommit?
> 
> Preface: we had to attach the layer too early in a previous patch-set.
> 
> We may want to attach layer in two scenarios:
> 1) when compositing information isn't available yet, so we want it ot be
attached ASAP (on next preCommit)
> 2) when compositing information is in place and we are allowed to query it.
> 
> If we make it always deferred (case 1) it may complicate some sub-systems and
make attachments/additions of animations scattered across the time and code. I
need to migrate a couple of blink subsystems (LinkHighlight and
ProgrammaticScrollAnimator).
> 
> I could make these scenarios explicit with two functions.
> Or we could go with always async way (attach on next preCommit).

I don't understand the relationship, LinkHighlight and
ProgrammaticScrollAnimator do not use AnimationPlayer.

[loyso (OOO), 2015-03-27 02:53:35]

On 2015/03/27 02:46:08, dstockwell wrote:
> On 2015/03/27 at 01:02:42, loyso wrote:
> >
>
https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
> > File Source/core/animation/AnimationPlayer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
> > Source/core/animation/AnimationPlayer.cpp:899:
> updateCompositedLayersAttachment();
> > On 2015/03/27 00:27:22, dstockwell wrote:
> > > So, why do we need to attempt do the attachment at this point? Why not
wait
> > > until after the compositing update in preCommit?
> > 
> > Preface: we had to attach the layer too early in a previous patch-set.
> > 
> > We may want to attach layer in two scenarios:
> > 1) when compositing information isn't available yet, so we want it ot be
> attached ASAP (on next preCommit)
> > 2) when compositing information is in place and we are allowed to query it.
> > 
> > If we make it always deferred (case 1) it may complicate some sub-systems
and
> make attachments/additions of animations scattered across the time and code. I
> need to migrate a couple of blink subsystems (LinkHighlight and
> ProgrammaticScrollAnimator).
> > 
> > I could make these scenarios explicit with two functions.
> > Or we could go with always async way (attach on next preCommit).
> 
> I don't understand the relationship, LinkHighlight and
> ProgrammaticScrollAnimator do not use AnimationPlayer.

LinkHighlight and ProgrammaticScrollAnimator add animations to WebLayer. In a
new world, they should add animations to AnimationPlayer. That's why they need
to create one and attach/detach it properly. Yes, this is a drawback to have our
"mutators" as completely external.

[dstockwell, 2015-03-29 22:31:40]

On 2015/03/27 at 02:53:35, loyso wrote:
> LinkHighlight and ProgrammaticScrollAnimator add animations to WebLayer. In a
new world, they should add animations to AnimationPlayer. That's why they need
to create one and attach/detach it properly. Yes, this is a drawback to have our
"mutators" as completely external.

OK, then I think we can wait until that refactoring. For now attaching the
layers as part of the regular lifecycle (in AnimationPlayer::preCommit) is
easier to comprehend. Making it part of preCommit should mean we can get rid of
AnimationTimeline::updateCompositedLayersAttachments too.

I think we'll want more tests before we can land this. At the very least:
1. a compositor animation running inside an iframe
2. a compositor animation running on an element that is detached and then
reattched with new layers

Could we land support for automated testing (setCompositorTime) before this
patch?

[loyso (OOO), 2015-03-30 02:47:07]

On 2015/03/29 22:31:40, dstockwell wrote:
> On 2015/03/27 at 02:53:35, loyso wrote:
> > LinkHighlight and ProgrammaticScrollAnimator add animations to WebLayer. In
a
> new world, they should add animations to AnimationPlayer. That's why they need
> to create one and attach/detach it properly. Yes, this is a drawback to have
our
> "mutators" as completely external.
> 
> OK, then I think we can wait until that refactoring. For now attaching the
> layers as part of the regular lifecycle (in AnimationPlayer::preCommit) is
> easier to comprehend. Making it part of preCommit should mean we can get rid
of
> AnimationTimeline::updateCompositedLayersAttachments too.
Done!

> 
> I think we'll want more tests before we can land this. At the very least:
> 1. a compositor animation running inside an iframe
> 2. a compositor animation running on an element that is detached and then
> reattched with new layers
> Could we land support for automated testing (setCompositorTime) before this
> patch?
Is 'land' == enable? This patch is disabled anyway. I'm going to investigate
multiple iframes/Documents and add a manual html test (temp).
Is it ok?

Compositor-animation-layout-tests is not a simple task and not started yet. Of
course, we want tests landed before we migrate (switch) to a new code.

[loyso (OOO), 2015-03-30 02:47:20]

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:898: m_attachCompositedLayers = true;
On 2015/03/27 00:27:22, dstockwell wrote:
> On 2015/03/26 23:36:14, loyso wrote:
> > On 2015/03/26 03:04:39, dstockwell wrote:
> > > m_compositedLayersAttached
> > 
> > It's a command, or 'dirty' flag. Not a sign that event happened. See those
> extra
> > 2 comments around.
> 
> It seems wrong that this method is called attachCompositedLayers, but it does
> may not actually attach the layers. Perhaps, setNeedsCompositeLayerAttachment

UPDATE: Simplified it so not applicable.

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/946323002/diff/120001/Source/core/animation/A...
Source/core/animation/AnimationTimeline.cpp:171: for (AnimationPlayer* player :
m_players)
On 2015/03/26 03:04:39, dstockwell wrote:
> m_playersNeedingUpdate?

UPDATE: Not applicable anymore.

[loyso (OOO), 2015-03-31 01:38:59]

PTAL!

[dstockwell, 2015-03-31 02:13:02]

lgtm

I'd still like to see some tests for detaching and reattaching elements. eg.

1. element.animate() (transform), then set style.display to 'none', then back to
'block'
2. element.animate() (opacity, since transform should not be valid on inline),
then set style.display to 'inline', then back to 'block'

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:278: createCompositorPlayer();
It now seems like createCompositorPlayer/attachCompositedLayers and
destroy/detach are always paired. Should we merge them?

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:456: attachCompositedLayers();
Shouldn't this be removed in favor of the call in preCommit?

[loyso (OOO), 2015-03-31 02:20:36]

loyso@chromium.org changed reviewers:
+ jchaffraix@chromium.org, vollick@chromium.org

[loyso (OOO), 2015-03-31 02:30:15]

On 2015/03/31 02:13:02, dstockwell wrote:
> I'd still like to see some tests for detaching and reattaching elements. eg.
Ack.

[loyso (OOO), 2015-03-31 02:30:27]

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:278: createCompositorPlayer();
On 2015/03/31 02:13:01, dstockwell wrote:
> It now seems like createCompositorPlayer/attachCompositedLayers and
> destroy/detach are always paired. Should we merge them?

Not always. AnimationPlayer::setSource uses attach/detach. Shouldn't we be able
to switch the source while compositor animation is running?

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:456: attachCompositedLayers();
On 2015/03/31 02:13:01, dstockwell wrote:
> Shouldn't this be removed in favor of the call in preCommit?
It's related to the comment above. Don't we call setSource from javascript
'on-the-fly'?

[Ian Vollick, 2015-03-31 02:52:34]

https://codereview.chromium.org/946323002/diff/180001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/946323002/diff/180001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:36: CompositorAnimationTimelines
Source/platform lgtm

[dstockwell, 2015-03-31 03:27:32]

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:456: attachCompositedLayers();
On 2015/03/31 02:30:27, loyso wrote:
> On 2015/03/31 02:13:01, dstockwell wrote:
> > Shouldn't this be removed in favor of the call in preCommit?
> It's related to the comment above. Don't we call setSource from javascript
> 'on-the-fly'?

Yes, but it can be called at any time, including before creation of layers. We
need to attachCompositedLayers in preCommit any time sourceChanged is true,
which will also include other cases like the target of an Animation changing.

[loyso (OOO), 2015-03-31 04:43:31]

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:456: attachCompositedLayers();
On 2015/03/31 03:27:32, dstockwell wrote:
> On 2015/03/31 02:30:27, loyso wrote:
> > On 2015/03/31 02:13:01, dstockwell wrote:
> > > Shouldn't this be removed in favor of the call in preCommit?
> > It's related to the comment above. Don't we call setSource from javascript
> > 'on-the-fly'?
> 
> Yes, but it can be called at any time, including before creation of layers. We
> need to attachCompositedLayers in preCommit any time sourceChanged is true,
> which will also include other cases like the target of an Animation changing.

Done.

[loyso (OOO), 2015-04-01 00:14:26]

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
File Source/core/animation/AnimationPlayer.cpp (right):

https://codereview.chromium.org/946323002/diff/180001/Source/core/animation/A...
Source/core/animation/AnimationPlayer.cpp:278: createCompositorPlayer();
On 2015/03/31 02:13:01, dstockwell wrote:
> It now seems like createCompositorPlayer/attachCompositedLayers and
> destroy/detach are always paired. Should we merge them?

Done.

[loyso (OOO), 2015-04-01 00:15:24]

On 2015/03/31 02:13:02, dstockwell wrote:
> I'd still like to see some tests for detaching and reattaching elements. eg.
> 
> 1. element.animate() (transform), then set style.display to 'none', then back
to
> 'block'
> 2. element.animate() (opacity, since transform should not be valid on inline),
> then set style.display to 'inline', then back to 'block'
Done.

[loyso (OOO), 2015-04-01 00:50:39]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-04-01 00:50:40]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
sigbjornf@opera.com, vollick@chromium.org, dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/946323002/#ps240001
(title: "Format.")

[commit-bot: I haz the power, 2015-04-01 00:50:46]

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

[commit-bot: I haz the power, 2015-04-01 06:30:02]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192909