Web Animations CSS: Start running animations on the compositor

Enables opacity, transform and filter animations to be run by the compositor.

CSSPendingAnimations is introduced to manage the synchronized start of
compositor and main-thread animations.

RenderLayerCompositor is updated to query the Web Animations model for cases
where it should composite a layer.

ActiveAnimations can be queried by property to test whether animations are
running, or running on the compositor.

Note that these changes are still behind the WebAnimationsCSS runtime flag.

There will be some subsequent changes to address missing functionality:
* pauseForTesting is not wired up for animations on the compositor
* CSS Transitions do not start from an up to date value when the target is updated
* layers are not always composited as they are in the current implementation

BUG=311423, 298211
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162269

[dstockwell, 2013-11-17 22:22:42]

Ready for a look:
* need to shuffle some of the isCandidate* methods between Player,
ActiveAnimations and CSSAnimations
* some fixmes need to be revalidated and removed
* some minor changes can be split into separate patches

[shans, 2013-11-17 22:55:22]

Against old patch, but still apply to head.

https://codereview.chromium.org/23874019/diff/313001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/313001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:42: return
renderer.style()->isRunningAcceleratedAnimation();
Should this be isRunningAnimation?

https://codereview.chromium.org/23874019/diff/313001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:44: }
You've refactored the old CSS path somewhat, or this clause wouldn't need to be
here. Can you add a comment about it somewhere, or perhaps split the refactor as
a separate patch?

[shans, 2013-11-18 01:02:05]

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.cpp:152: if
(!isCandidateForCompositorAnimation())
If this is part of the this method, then it should be maybeStart

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.h:61: bool startCompositorAnimations();
startOnCompositor?

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.h:62: bool isRunningCompositorAnimation(const
Element* = 0, CSSPropertyID = CSSPropertyInvalid) const;
isRunningOnCompositor?

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.h:63: void cancelCompositorAnimations();
Some comments about when these can be called would be good. For example, it
seems like it's fine to speculatively call cancel, but one is expected to call
start only once.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:328: // FIXME: Compositor only
non-zero, integer iteration counts.
I liked your old FIXME better than your new FIXME

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:485: // ASSERT(values[i].first ==
chained->m_segments[i].m_min);
Just remove then?

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

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.h:64: virtual bool
startCompositorAnimation(const Element&, const Timing&, const AnimationEffect&,
Vector<int>& startedAnimationIds);
Plus One. Should this FIXME be in Animation.h too?

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/In...
File Source/core/animation/InertAnimation.h (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/In...
Source/core/animation/InertAnimation.h:47: bool
isCandidateForCompositorAnimation() const;
Does this need an implementation?

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Pl...
File Source/core/animation/Player.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Pl...
Source/core/animation/Player.cpp:89: return
animation->isRunningCompositorAnimation();
This is confusing. Why doesn't this method map cleanly through?

https://codereview.chromium.org/23874019/diff/573001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1933:
renderer()->animation().notifyAnimationStarted(renderer(), time);
(Not required for this CL): I think we need to draw up a diagram of all the
places that the Web Animations engine is poked by / pokes the rest of Blink

[Steve Block, 2013-11-18 05:03:02]

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
File Source/core/animation/AnimationStack.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/AnimationStack.cpp:52: for (size_t i = 0; i <
m_activeAnimations.size(); ++i)
Needs braces

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Ke...
File Source/core/animation/KeyframeAnimationEffect.h (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Ke...
Source/core/animation/KeyframeAnimationEffect.h:84: bool affects(CSSPropertyID
property)
I know the spelling is different, but having an 'effect' class with an 'affects'
method seems to be inviting confusion. Maybe 'doesAnimate' or
'modifiesProperty'?

https://codereview.chromium.org/23874019/diff/573001/Source/core/frame/animat...
File Source/core/frame/animation/AnimationController.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/frame/animat...
Source/core/frame/animation/AnimationController.cpp:484:
ASSERT(!RuntimeEnabledFeatures::webAnimationsCSSEnabled());
Shouldn't we just assert in the constructor?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/ActiveAnimations.cpp:89: if (property ==
CSSPropertyInvalid)
Do you need this? I don't see a call site with CSSPropertyInvalid. If you do
need it, you should probably overload the method, as discussed.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/ActiveAnimations.h:49: bool
shouldCompositeForAnimation(const RenderObject&, bool inCompositingMode);
Ordering in .h and .cpp should match

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:127: return isNull(timeToNextIteration) ?
activeEndTime - localTime : timeToNextIteration;
Does this assume that timeToNextIteration is always <= the time to the end of
the animation? Can we assert this?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:156: if
(CompositorAnimations::instance()->startCompositorAnimation(*m_target.get(),
specified(), *effect(), m_acceleratedAnimationIds)) {
Given that startCompositorAnimation() returns a vector of animation IDs, the
name really should be plural.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:158: return true;
Can you invert this 'if' to make all the early-outs 'false'.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:175: return
toKeyframeAnimationEffect(m_effect.get())->affects(property);
Hmm, why isn't affects() on AnimationEffect, rather than just
KeyframeAnimationEffect? It seems like the concept makes sense for any effect.

I guess that for CSS, we only need to consider KAE, but most of the Web
Animations model logic supports the general case. I think we should at least
have a comment somewhere explaining what doesn't support the general case.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.h:64: void cancelCompositorAnimations();
The use of 'Animation' here seems awkward given the class name. I guess the
problem is that a single 'Animation' can map to several 'compositor animations'.
I'm not sure I can think of better names though.

Perhaps we should just make isCandidateForCompositorAnimations() and
isRunningCompositorAnimations() plural to make it more clear?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.h:86: Vector<int> m_acceleratedAnimationIds;
We should be consistent with naming: 'compositorAnimation' vs
'acceleratedAnimation'.

Also probably warrants a comment.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/AnimationEffect.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/AnimationEffect.h:63: virtual bool affects(CSSPropertyID)
{ return false; };
It looks like my comment in Animation::affects() is moot. I guess you don't need
toKeyframeAnimationEffect() there.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/D...
File Source/core/animation/DocumentTimeline.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/D...
Source/core/animation/DocumentTimeline.cpp:72: PassRefPtr<Player>
DocumentTimeline::createPlayer(TimedItem* child)
This should return a raw pointer, not a PRP. This object holds a ref to the
newly created Player, so we can't know that the caller wants to take a ref.
timeline.createPlayer()->pause() is valid and shouldn't churn the ref count.

Same is true of play().

Could you pull the addition of this method (and fixing the PRP) into a
prerequisite patch?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/D...
Source/core/animation/DocumentTimeline.cpp:172: ++started;
I think this is wrong, but it was before too. See
https://codereview.chromium.org/66953009

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/K...
File Source/core/animation/KeyframeAnimationEffect.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/K...
Source/core/animation/KeyframeAnimationEffect.h:84: bool affects(CSSPropertyID
property) OVERRIDE
virtual

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
File Source/core/animation/Player.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.cpp:161:
RELEASE_ASSERT(!isRunningCompositorAnimation());
Can you explain why you can assert this?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
File Source/core/animation/Player.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.h:34: #include "CSSPropertyNames.h"
Superfluous

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.h:41: class Element;
Superfluous

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.h:49: static double effectiveTime(double time) {
return isNull(time) ? 0 : time; }
Why is this now in the header? I don't see it used anywhere outside the cpp. If
it does need to be public, Player seems like an odd place for it to live. Maybe
Timing or TimedItem?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:715: return true;
It's a shame that the logic regarding these 3 properties is repeated twice here
and then again in shouldCompositeForAnimation(). Can we share it?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:122: bool
hasPendingCandidatesForCompositorAnimation() const;
Where is this used?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:170: bool
hasPendingCandidateWhichShouldComposite(bool inCompositingMode) const;
Can you rename the param to make clear what's in compositing mode?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSPendingAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.cpp:57:
player->setStartTime(player->timeline().currentTime());
Time is frozen here, so all players get exactly the same time, right? Can we
assert or comment this?

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSPendingAnimations.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.h:39: class CSSPendingAnimations
FINAL {
Can you add a brief class comment.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.h:43: void
notifyCompositorAnimationStarted(double);
Should name this param

https://codereview.chromium.org/23874019/diff/1053002/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:579: // FIXME: handle this under
web-animations
How does this affect behavior? Is it flag-flip blocking?

[dstockwell, 2013-11-18 05:30:33]

Addressed comments in patch sets 10 and 11.

https://codereview.chromium.org/23874019/diff/313001/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/313001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:42: return
renderer.style()->isRunningAcceleratedAnimation();
On 2013/11/17 22:55:23, shans wrote:
> Should this be isRunningAnimation?

This was removed.

https://codereview.chromium.org/23874019/diff/313001/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:44: }
On 2013/11/17 22:55:23, shans wrote:
> You've refactored the old CSS path somewhat, or this clause wouldn't need to
be
> here. Can you add a comment about it somewhere, or perhaps split the refactor
as
> a separate patch?

I unrefactored it.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.cpp:152: if
(!isCandidateForCompositorAnimation())
On 2013/11/18 01:02:06, shans wrote:
> If this is part of the this method, then it should be maybeStart

Done.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.h:61: bool startCompositorAnimations();
On 2013/11/18 01:02:06, shans wrote:
> startOnCompositor?

Sounds good, but let's defer renaming until the end or after review.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.h:62: bool isRunningCompositorAnimation(const
Element* = 0, CSSPropertyID = CSSPropertyInvalid) const;
On 2013/11/18 01:02:06, shans wrote:
> isRunningOnCompositor?

Sounds good, but let's defer renaming until the end or after review.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/Animation.h:63: void cancelCompositorAnimations();
On 2013/11/18 01:02:06, shans wrote:
> Some comments about when these can be called would be good. For example, it
> seems like it's fine to speculatively call cancel, but one is expected to call
> start only once.

Done.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
File Source/core/animation/AnimationStack.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/An...
Source/core/animation/AnimationStack.cpp:52: for (size_t i = 0; i <
m_activeAnimations.size(); ++i)
On 2013/11/18 05:03:03, Steve Block wrote:
> Needs braces

Done.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:328: // FIXME: Compositor only
non-zero, integer iteration counts.
On 2013/11/18 01:02:06, shans wrote:
> I liked your old FIXME better than your new FIXME

This is being fixed in a separate patch.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:485: // ASSERT(values[i].first ==
chained->m_segments[i].m_min);
On 2013/11/18 01:02:06, shans wrote:
> Just remove then?

This is being fixed in a separate patch.

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

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.h:64: virtual bool
startCompositorAnimation(const Element&, const Timing&, const AnimationEffect&,
Vector<int>& startedAnimationIds);
On 2013/11/18 01:02:06, shans wrote:
> Plus One. Should this FIXME be in Animation.h too?

I think it's fine to just have it here, if this is changed it will propagate
back up as necessary.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/In...
File Source/core/animation/InertAnimation.h (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/In...
Source/core/animation/InertAnimation.h:47: bool
isCandidateForCompositorAnimation() const;
On 2013/11/18 01:02:06, shans wrote:
> Does this need an implementation?

Removed.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Ke...
File Source/core/animation/KeyframeAnimationEffect.h (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Ke...
Source/core/animation/KeyframeAnimationEffect.h:84: bool affects(CSSPropertyID
property)
On 2013/11/18 05:03:03, Steve Block wrote:
> I know the spelling is different, but having an 'effect' class with an
'affects'
> method seems to be inviting confusion. Maybe 'doesAnimate' or
> 'modifiesProperty'?

Taking a poll amongst the team on this one.

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Pl...
File Source/core/animation/Player.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/animation/Pl...
Source/core/animation/Player.cpp:89: return
animation->isRunningCompositorAnimation();
On 2013/11/18 01:02:06, shans wrote:
> This is confusing. Why doesn't this method map cleanly through?

This method no longer exists.

https://codereview.chromium.org/23874019/diff/573001/Source/core/frame/animat...
File Source/core/frame/animation/AnimationController.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/frame/animat...
Source/core/frame/animation/AnimationController.cpp:484:
ASSERT(!RuntimeEnabledFeatures::webAnimationsCSSEnabled());
On 2013/11/18 05:03:03, Steve Block wrote:
> Shouldn't we just assert in the constructor?

I removed these for now. But I think this object is still created, we should
disable it in a separate patch.

https://codereview.chromium.org/23874019/diff/573001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/23874019/diff/573001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1933:
renderer()->animation().notifyAnimationStarted(renderer(), time);
On 2013/11/18 01:02:06, shans wrote:
> (Not required for this CL): I think we need to draw up a diagram of all the
> places that the Web Animations engine is poked by / pokes the rest of Blink

Will do, but I think we should move as much as we can to places like
CompositorAnimations, DocumentTimelineServicing etc.

[dstockwell, 2013-11-18 06:11:20]

PTAL, addressed comments in Patch Set 12.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/ActiveAnimations.cpp:89: if (property ==
CSSPropertyInvalid)
On 2013/11/18 05:03:03, Steve Block wrote:
> Do you need this? I don't see a call site with CSSPropertyInvalid. If you do
> need it, you should probably overload the method, as discussed.

Removed.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/ActiveAnimations.h:49: bool
shouldCompositeForAnimation(const RenderObject&, bool inCompositingMode);
On 2013/11/18 05:03:03, Steve Block wrote:
> Ordering in .h and .cpp should match

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:127: return isNull(timeToNextIteration) ?
activeEndTime - localTime : timeToNextIteration;
On 2013/11/18 05:03:03, Steve Block wrote:
> Does this assume that timeToNextIteration is always <= the time to the end of
> the animation? Can we assert this?

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:156: if
(CompositorAnimations::instance()->startCompositorAnimation(*m_target.get(),
specified(), *effect(), m_acceleratedAnimationIds)) {
On 2013/11/18 05:03:03, Steve Block wrote:
> Given that startCompositorAnimation() returns a vector of animation IDs, the
> name really should be plural.

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:158: return true;
On 2013/11/18 05:03:03, Steve Block wrote:
> Can you invert this 'if' to make all the early-outs 'false'.

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.cpp:175: return
toKeyframeAnimationEffect(m_effect.get())->affects(property);
On 2013/11/18 05:03:03, Steve Block wrote:
> Hmm, why isn't affects() on AnimationEffect, rather than just
> KeyframeAnimationEffect? It seems like the concept makes sense for any effect.
> 
> I guess that for CSS, we only need to consider KAE, but most of the Web
> Animations model logic supports the general case. I think we should at least
> have a comment somewhere explaining what doesn't support the general case.

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.h:64: void cancelCompositorAnimations();
On 2013/11/18 05:03:03, Steve Block wrote:
> The use of 'Animation' here seems awkward given the class name. I guess the
> problem is that a single 'Animation' can map to several 'compositor
animations'.
> I'm not sure I can think of better names though.
> 
> Perhaps we should just make isCandidateForCompositorAnimations() and
> isRunningCompositorAnimations() plural to make it more clear?

I've made everything plural for now.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/Animation.h:86: Vector<int> m_acceleratedAnimationIds;
On 2013/11/18 05:03:03, Steve Block wrote:
> We should be consistent with naming: 'compositorAnimation' vs
> 'acceleratedAnimation'.
> 
> Also probably warrants a comment.

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
File Source/core/animation/AnimationEffect.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/A...
Source/core/animation/AnimationEffect.h:63: virtual bool affects(CSSPropertyID)
{ return false; };
On 2013/11/18 05:03:03, Steve Block wrote:
> It looks like my comment in Animation::affects() is moot. I guess you don't
need
> toKeyframeAnimationEffect() there.

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/D...
File Source/core/animation/DocumentTimeline.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/D...
Source/core/animation/DocumentTimeline.cpp:72: PassRefPtr<Player>
DocumentTimeline::createPlayer(TimedItem* child)
On 2013/11/18 05:03:03, Steve Block wrote:
> This should return a raw pointer, not a PRP. This object holds a ref to the
> newly created Player, so we can't know that the caller wants to take a ref.
> timeline.createPlayer()->pause() is valid and shouldn't churn the ref count.
> 
> Same is true of play().
> 
> Could you pull the addition of this method (and fixing the PRP) into a
> prerequisite patch?

Hmm, maybe I'm confused about how this works. I thought that since we have a
RefPtr here, doing a release() is essentially the same cost?

Either a deref happens in the caller (if we release here) or here (if we return
a raw pointer).

Will split this out after discussing.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/D...
Source/core/animation/DocumentTimeline.cpp:172: ++started;
On 2013/11/18 05:03:03, Steve Block wrote:
> I think this is wrong, but it was before too. See
> https://codereview.chromium.org/66953009

Will wait and rebase.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/K...
File Source/core/animation/KeyframeAnimationEffect.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/K...
Source/core/animation/KeyframeAnimationEffect.h:84: bool affects(CSSPropertyID
property) OVERRIDE
On 2013/11/18 05:03:03, Steve Block wrote:
> virtual

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
File Source/core/animation/Player.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.cpp:161:
RELEASE_ASSERT(!isRunningCompositorAnimation());
On 2013/11/18 05:03:03, Steve Block wrote:
> Can you explain why you can assert this?

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
File Source/core/animation/Player.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.h:34: #include "CSSPropertyNames.h"
On 2013/11/18 05:03:03, Steve Block wrote:
> Superfluous

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.h:41: class Element;
On 2013/11/18 05:03:03, Steve Block wrote:
> Superfluous

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/P...
Source/core/animation/Player.h:49: static double effectiveTime(double time) {
return isNull(time) ? 0 : time; }
On 2013/11/18 05:03:03, Steve Block wrote:
> Why is this now in the header? I don't see it used anywhere outside the cpp.
If
> it does need to be public, Player seems like an odd place for it to live.
Maybe
> Timing or TimedItem?

Unused, was missed in a refactor. Removed.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.cpp:715: return true;
On 2013/11/18 05:03:03, Steve Block wrote:
> It's a shame that the logic regarding these 3 properties is repeated twice
here
> and then again in shouldCompositeForAnimation(). Can we share it?

There's a FIXME on line 702 with a possible solution.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:122: bool
hasPendingCandidatesForCompositorAnimation() const;
On 2013/11/18 05:03:03, Steve Block wrote:
> Where is this used?

Removed.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSAnimations.h:170: bool
hasPendingCandidateWhichShouldComposite(bool inCompositingMode) const;
On 2013/11/18 05:03:03, Steve Block wrote:
> Can you rename the param to make clear what's in compositing mode?

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSPendingAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.cpp:57:
player->setStartTime(player->timeline().currentTime());
On 2013/11/18 05:03:03, Steve Block wrote:
> Time is frozen here, so all players get exactly the same time, right? Can we
> assert or comment this?

Hmm, non trivial to assert this (as there are multiple timelines involved). We
could expose the clock from timeline and add isFrozen. Added a FIXME for now.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
File Source/core/animation/css/CSSPendingAnimations.h (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.h:39: class CSSPendingAnimations
FINAL {
On 2013/11/18 05:03:03, Steve Block wrote:
> Can you add a brief class comment.

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/animation/c...
Source/core/animation/css/CSSPendingAnimations.h:43: void
notifyCompositorAnimationStarted(double);
On 2013/11/18 05:03:03, Steve Block wrote:
> Should name this param

Done.

https://codereview.chromium.org/23874019/diff/1053002/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23874019/diff/1053002/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:579: // FIXME: handle this under
web-animations
On 2013/11/18 05:03:03, Steve Block wrote:
> How does this affect behavior? Is it flag-flip blocking?

Unclear, needs further investigation :(

[ajuma, 2013-11-18 21:21:16]

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:156: // FIXME: We should know
ahead of time whether these animations can be started.
Note that for the old path, there's logic to avoid sending pixel-moving filter
animations to the compositor (e.g. see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
We'll need something equivalent for the new path too.

[dstockwell, 2013-11-18 21:34:02]

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Co...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Co...
Source/core/animation/CompositorAnimations.cpp:156: // FIXME: We should know
ahead of time whether these animations can be started.
On 2013/11/18 21:21:17, ajuma wrote:
> Note that for the old path, there's logic to avoid sending pixel-moving filter
> animations to the compositor (e.g. see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> We'll need something equivalent for the new path too.

Thanks for taking a look. We have the same logic here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[shans, 2013-11-18 21:55:00]

lgtm

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:39: bool
shouldCompositeForAnimation(const RenderObject& renderer, bool
renderViewInCompositingMode)
This function name is very confusing. Which animation?

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:50: ||
activeAnimations->isRunningAnimation(CSSPropertyWebkitFilter);
FIXME: isRunningAnimation(PropertySet) once Opacity auto-promotes

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
Source/core/animation/Animation.cpp:128: return isNull(timeToNextIteration) ?
activeEndTime - localTime : timeToNextIteration;
This would be cleaner if timeToNextIteration was Infinity when no next iteration
exists.

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
Source/core/animation/Animation.h:93: return static_cast<Animation*>(timedItem);
use macro magic to generate this cast?

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Pl...
File Source/core/animation/Player.cpp (left):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Pl...
Source/core/animation/Player.cpp:162: m_pauseStartTime = nullValue();
TODO: restart compositor animations?

[Steve Block, 2013-11-18 22:57:53]

lgtm pending rebase and fix to
DocumentTimeline::numberOfActiveAnimationsForTesting()

[dstockwell, 2013-11-19 02:04:57]

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Ac...
File Source/core/animation/ActiveAnimations.cpp (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:39: bool
shouldCompositeForAnimation(const RenderObject& renderer, bool
renderViewInCompositingMode)
On 2013/11/18 21:55:01, shans wrote:
> This function name is very confusing. Which animation?

Will change this soon.

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Ac...
Source/core/animation/ActiveAnimations.cpp:50: ||
activeAnimations->isRunningAnimation(CSSPropertyWebkitFilter);
On 2013/11/18 21:55:01, shans wrote:
> FIXME: isRunningAnimation(PropertySet) once Opacity auto-promotes

Done.

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
Source/core/animation/Animation.cpp:128: return isNull(timeToNextIteration) ?
activeEndTime - localTime : timeToNextIteration;
On 2013/11/18 21:55:01, shans wrote:
> This would be cleaner if timeToNextIteration was Infinity when no next
iteration
> exists.

Let's do that in another patch.

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/An...
Source/core/animation/Animation.h:93: return static_cast<Animation*>(timedItem);
On 2013/11/18 21:55:01, shans wrote:
> use macro magic to generate this cast?

Done.

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Pl...
File Source/core/animation/Player.cpp (left):

https://codereview.chromium.org/23874019/diff/923006/Source/core/animation/Pl...
Source/core/animation/Player.cpp:162: m_pauseStartTime = nullValue();
On 2013/11/18 21:55:01, shans wrote:
> TODO: restart compositor animations?

Done.

[dstockwell, 2013-11-19 02:28:33]

PTAL

Renamed everything.

[commit-bot: I haz the power, 2013-11-19 03:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/23874019/1553001

[commit-bot: I haz the power, 2013-11-19 05:24:35]

Retried try job too often on win_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-11-19 05:46:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/23874019/1553001

[commit-bot: I haz the power, 2013-11-19 08:02:21]

Change committed as 162269

[Mike Lawther (Google), 2013-11-20 02:33:47]

Oops - please ignore Patch Set 22 :( I accidentally uploaded it to this issue.