Bug 75874 – [chromium] Plumb from GraphicsLayer to the cc thread animation code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Nat Duca]

Lets get GraphicsLayer::add/pause/cancel animation hooked up to CC.

We'll use this as a meta-bug to track progress.

Created attachment 122694 [details]
Patch

Created attachment 122696 [details]
Patch

[Nat Duca]

Comment on attachment 122696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122696&action=review

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:214
> +    CCLayerAnimationController* animationController() const { return m_animationController.get(); }

On this side, can we avoid exposing the controller? I'd like either to expose methods for manipulating animations, or the controller, but not both.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:52
> +PassRefPtr<CCLayerImpl> TreeSynchronizer::synchronizeTreeRecursive(LayerChromium* layer, double currentTimeMs, CCLayerImplMap& map, bool& activatedAnimation)

erm, help me understand this. I wasn't expecting to see either.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:325
> +        hostImpl->setRootLayer(newRootLayer.release());

Lets do this separately. SynchronizeTrees should push trees. Nothign else.

Activation should be done entirely impl-thread side. I dont think you need anything specific to activation because every commit leads to a animate and draw. So in animate, you should just always call animate on all the layer controllers. Maybe you can have that path compute whether the there are any animations still pending and use that to drive another frame.

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:96
> +class FakeFloatAnimation : public CCFloatAnimationCurve {

FakeFloatAnimationCurve.

iirw, you have a fake float animaiton in another test file too.

Two options:
1. decide you'll have a common class that gets used in a few places for animation tests and make Then, make a CCAnimatonTestsCommon.h and use it.

Or, decide the test case here is specific, and name this something that is specific to this test...

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:334
> +TEST(TreeSynchronizerTest, syncAnimations)

These feel like different TESTs

Created attachment 123871 [details]
Patch

(In reply to comment #3)
> (From update of attachment 122696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122696&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:214
> > +    CCLayerAnimationController* animationController() const { return m_animationController.get(); }
> 
> On this side, can we avoid exposing the controller? I'd like either to expose methods for manipulating animations, or the controller, but not both.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:52
> > +PassRefPtr<CCLayerImpl> TreeSynchronizer::synchronizeTreeRecursive(LayerChromium* layer, double currentTimeMs, CCLayerImplMap& map, bool& activatedAnimation)
> 
> erm, help me understand this. I wasn't expecting to see either.
Both are now gone.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:325
> > +        hostImpl->setRootLayer(newRootLayer.release());
> 
> Lets do this separately. SynchronizeTrees should push trees. Nothign else.
> 
> Activation should be done entirely impl-thread side. I dont think you need anything specific to activation because every commit leads to a animate and draw. So in animate, you should just always call animate on all the layer controllers. Maybe you can have that path compute whether the there are any animations still pending and use that to drive another frame.
Done.
> 
> > Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:96
> > +class FakeFloatAnimation : public CCFloatAnimationCurve {
> 
> FakeFloatAnimationCurve.
Done.
> 
> iirw, you have a fake float animaiton in another test file too.
> 
> Two options:
> 1. decide you'll have a common class that gets used in a few places for animation tests and make Then, make a CCAnimatonTestsCommon.h and use it.
> 
> Or, decide the test case here is specific, and name this something that is specific to this test...
> 

Done. (Added CCAnimationTestCommon.h)

> > Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:334
> > +TEST(TreeSynchronizerTest, syncAnimations)
> 
> These feel like different TESTs

Done. They are now separate TESTs.

[Nat Duca]

Comment on attachment 123871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123871&action=review

Do we have naming confusion around hasActiveAnimation and"I need my animate to be called?" I'd rather we be explciit about this and say what we mean: needsAnimate or something.

So, the one thing that still is funky here is the activeAnimation --- and as you noted in our chat earlier, we still need to do the finished plumbing. I'm fine doing that as a followup tbh.

Lets talk about the active stuff tomrrow, actually. I think maybe we want a tristate "yes,no,not_sure"?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:95
> +    virtual bool addAnimation(const KeyframeValueList&, const IntSize& /*boxSize*/, const Animation*, const String& /*animationName*/, double /*timeOffset*/);

why /* */ here?
Typically, you'll just use the full name on the ones that are nonobvious, which is all of them.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:250
> +    CCLayerAnimationController* animationController() const { return m_animationController.get(); }

Maybe I'm being pedantic, but I prefer const-preserving getters. E.g. on the const accessor, return const X*, or make the method non const.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:100
> +    OwnPtr<CCActiveAnimation> toReturn(adoptPtr(new CCActiveAnimation(m_animationCurve.release(), m_group, m_targetProperty)));

Hmmm, I think its bad that the clone function is mutating the this pointer as part of clone. It should really have a const signature.

Pass the animation curve in? E.g. clone(CurveForImplThread*)?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:113
> +    other->m_startTime = m_startTime;

Any way to get common code between this and the prev function? Easy to miss a field if we add one...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:38
> +    // FIXME: implement.

Delete this. Pretty obvious we need to implement it. :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:63
> +// are kept in sync. This function does not take ownership of the impl thread controller.

2 or 3 sentence summary of how this syncing operation works.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:64
> +bool CCLayerAnimationController::pushAnimationsTo(CCLayerAnimationControllerImpl* controller)

controller-> controllerImpl so we can tell directionality of command.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:68
> +    size_t i = 0;

any reason you're declaring i up here rather than in the for body? I'd rather you declare it in the body...

since this has phases, does it make sense to break up into sub functions like you did on the impl?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:71
> +

Why are we clearing this here? I woudl have expected that we clear this earlier on in the frame lifetime...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:81
> +    // Each iteration, controller->m_activeAnimations.size() is decremented or i is incremented

Comment explaining what this loop does. It took me a fair amount of time to realize this removes animations on the impl side.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:82
> +    // guaranteeing progress towards loop termination.

Is there a way to write this so its not so bizarre of a loop? Its cool if not, but this is fragile.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:93
> +    return addedAnimation;

Why are we still returning addedAnimation? It looks like we ignore the return value...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:52
> +    bool addAnimation(const KeyframeValueList&, const IntSize& /*boxSize*/, const Animation*, const String& /*animationName*/, double /*timeOffset*/);

/* */ -> actual names where non redundant, which is all.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:53
> +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);

timeOffset --- use whatever convention we established before for time. Maybe its timeOffset? /me forgets

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:-181
> -        if (allAnimsWithSameIdAreFinished)

It seems to me that the animate function could pass in a data structure called AnimationResults which coudl itself have a vector of finished animations, as well as whehter you have to tick next frame. Then pass it down to this function as well, and push finished things onto the reults. You'd have one AnimationResults object that'd be stack allocated up in the CCLayerTreeHostImpl, etc etc etc

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:70
> +    typedef std::pair<CCActiveAnimation::GroupID, CCActiveAnimation::TargetProperty> AnimationSignature;

Struct? Hard to remember what first and second are...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:111
> +    // CCLayerAnimationControllerImplClient implementation.

Rearrange so that the implementation is up at the top of the class. The template for our classes is usually

class Foo : public X {
   static Foo create();
   // X implementation

   // Foo stuff
}

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:205
> +    CCLayerAnimationControllerImpl* animationController() const { return m_animationController.get(); }

Const preserving ... either drop the const or make two functions one that preserves constness and one that doesnt.

E.g. 
const Foo* foo()  const;
Foo* foo();

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:315
> +    // Manages animations targeting this layer.

targeting --> for

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:158
> +        hostImpl->setMayHaveActiveAnimationController(true);

See comment below.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:323
> +        hostImpl->setMayHaveActiveAnimationController(true);

setActiveAnimationsChanged()

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:60
> +    , m_mayHaveActiveAnimationController(true)

Lets talk about the CCLayer::hasActiveAnimation trick I proposed and see if we can do that.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:252
> +    TRACE_EVENT("CCLayerTreeHostImpl::animateLayers", this, 0);

So we stop ticking when we go to a background tab, but CSS requires that we deliver end events when that happens. page/AnimationController has a lot of complexity around "nextServiceTime" to handle this. How can we get this correct but not do something so complex? How about we have a CCTimer on the Impl class that we active/deactive when the impl tree goes invisible/visible. That timer can fire at a low rate --- every quarter second or something. WDYT?

(This implies that we need to send the animation finished events separate from the beginFrameAndCommit message, because frames dont begin when you're backgrounded)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:258
> +    while (toProcess.size()) {

Any reason for this vs recursion?

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:405
> +}

I'm a little brain fried but do we have tests for the cclthi actually animating when we add layerimpls in ways that might defeat our hasActiveAnimation stuff?

Created attachment 124966 [details]
Patch

(In reply to comment #6)
> (From update of attachment 123871 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123871&action=review
> 
> Do we have naming confusion around hasActiveAnimation and"I need my animate to be called?" I'd rather we be explciit about this and say what we mean: needsAnimate or something.
> 
> So, the one thing that still is funky here is the activeAnimation --- and as you noted in our chat earlier, we still need to do the finished plumbing. I'm fine doing that as a followup tbh.
> 
> Lets talk about the active stuff tomrrow, actually. I think maybe we want a tristate "yes,no,not_sure"?

CCLayerTreeHostImpl now has a boolean m_needsAnimateLayers field. Please let me know if you'd like any name changes.

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:95
> > +    virtual bool addAnimation(const KeyframeValueList&, const IntSize& /*boxSize*/, const Animation*, const String& /*animationName*/, double /*timeOffset*/);
> 
> why /* */ here?
> Typically, you'll just use the full name on the ones that are nonobvious, which is all of them.
Fixed.
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:250
> > +    CCLayerAnimationController* animationController() const { return m_animationController.get(); }
> 
> Maybe I'm being pedantic, but I prefer const-preserving getters. E.g. on the const accessor, return const X*, or make the method non const.
Was following the lead of layerTreeHost() :)  I've made the function non-const, since it effectively is.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:100
> > +    OwnPtr<CCActiveAnimation> toReturn(adoptPtr(new CCActiveAnimation(m_animationCurve.release(), m_group, m_targetProperty)));
> 
> Hmmm, I think its bad that the clone function is mutating the this pointer as part of clone. It should really have a const signature.

Fixed. The curves can now be cloned.

> 
> Pass the animation curve in? E.g. clone(CurveForImplThread*)?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:113
> > +    other->m_startTime = m_startTime;
> 
> Any way to get common code between this and the prev function? Easy to miss a field if we add one...

Turns out that the functions are quite different.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:38
> > +    // FIXME: implement.
> 
> Delete this. Pretty obvious we need to implement it. :)

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:63
> > +// are kept in sync. This function does not take ownership of the impl thread controller.
> 
> 2 or 3 sentence summary of how this syncing operation works.
I've refactored the function into 4 helper functions with self documenting names. Please let me know if anything needs to be clarified.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:64
> > +bool CCLayerAnimationController::pushAnimationsTo(CCLayerAnimationControllerImpl* controller)
> 
> controller-> controllerImpl so we can tell directionality of command.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:68
> > +    size_t i = 0;
> 
> any reason you're declaring i up here rather than in the for body? I'd rather you declare it in the body...
> 
> since this has phases, does it make sense to break up into sub functions like you did on the impl?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:71
> > +
> 
> Why are we clearing this here? I woudl have expected that we clear this earlier on in the frame lifetime...

We need to hang onto the collection of finished animations until the next commit. Once we've sync'd the controllers, we're done with this list and it can be cleared.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:81
> > +    // Each iteration, controller->m_activeAnimations.size() is decremented or i is incremented
> 
> Comment explaining what this loop does. It took me a fair amount of time to realize this removes animations on the impl side.
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:82
> > +    // guaranteeing progress towards loop termination.
> 
> Is there a way to write this so its not so bizarre of a loop? Its cool if not, but this is fragile.
The loop has been simplified slightly, but it's effectively the same. This seemed like a simple way of iterating through a collection, conditionally deleting elements, that does not require a copy of the collection. If you know of a preferred way to do this, I would be happy to change the code.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:93
> > +    return addedAnimation;
> 
> Why are we still returning addedAnimation? It looks like we ignore the return value...
Fixed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:52
> > +    bool addAnimation(const KeyframeValueList&, const IntSize& /*boxSize*/, const Animation*, const String& /*animationName*/, double /*timeOffset*/);
> 
> /* */ -> actual names where non redundant, which is all.
Fixed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:53
> > +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);
> 
> timeOffset --- use whatever convention we established before for time. Maybe its timeOffset? /me forgets
This is the variable name from the signatures in GraphicsLayer. For consistency, I was hoping to retain this name until we translate it for use by the cc animation code.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:-181
> > -        if (allAnimsWithSameIdAreFinished)
> 
> It seems to me that the animate function could pass in a data structure called AnimationResults which coudl itself have a vector of finished animations, as well as whehter you have to tick next frame. Then pass it down to this function as well, and push finished things onto the reults. You'd have one AnimationResults object that'd be stack allocated up in the CCLayerTreeHostImpl, etc etc etc
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.h:70
> > +    typedef std::pair<CCActiveAnimation::GroupID, CCActiveAnimation::TargetProperty> AnimationSignature;
> 
> Struct? Hard to remember what first and second are...
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:111
> > +    // CCLayerAnimationControllerImplClient implementation.
> 
> Rearrange so that the implementation is up at the top of the class. The template for our classes is usually
> 
> class Foo : public X {
>    static Foo create();
>    // X implementation
> 
>    // Foo stuff
> }
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:205
> > +    CCLayerAnimationControllerImpl* animationController() const { return m_animationController.get(); }
> 
> Const preserving ... either drop the const or make two functions one that preserves constness and one that doesnt.
> 
> E.g. 
> const Foo* foo()  const;
> Foo* foo();
> 
Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:315
> > +    // Manages animations targeting this layer.
> 
> targeting --> for
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:158
> > +        hostImpl->setMayHaveActiveAnimationController(true);
> 
> See comment below.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:323
> > +        hostImpl->setMayHaveActiveAnimationController(true);
> 
> setActiveAnimationsChanged()
Renamed according to your topmost comments.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:60
> > +    , m_mayHaveActiveAnimationController(true)
> 
> Lets talk about the CCLayer::hasActiveAnimation trick I proposed and see if we can do that.
This has been updated per our last conversations.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:252
> > +    TRACE_EVENT("CCLayerTreeHostImpl::animateLayers", this, 0);
> 
> So we stop ticking when we go to a background tab, but CSS requires that we deliver end events when that happens. page/AnimationController has a lot of complexity around "nextServiceTime" to handle this. How can we get this correct but not do something so complex? How about we have a CCTimer on the Impl class that we active/deactive when the impl tree goes invisible/visible. That timer can fire at a low rate --- every quarter second or something. WDYT?
That sounds great. I would like to add the background ticking behaviour in another patch. SG?
> 
> (This implies that we need to send the animation finished events separate from the beginFrameAndCommit message, because frames dont begin when you're backgrounded)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:258
> > +    while (toProcess.size()) {
> 
> Any reason for this vs recursion?
I was worried about really deep trees. Switched it to recursion.
> 
> > Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:405
> > +}
> 
> I'm a little brain fried but do we have tests for the cclthi actually animating when we add layerimpls in ways that might defeat our hasActiveAnimation stuff?

We have tests for synchronizing the animation controllers, but no tests for the 'needsAnimateLayers' code.

[Nat Duca]

Comment on attachment 124966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124966&action=review

Getting close.

We need a test in CCLayerTreeHostTest that tests that adding an animation runs the animateLayers callback.
We also need a test that verifies calling animate () on a layertreehostimpl that has a still-running animation gets its animate() called again.

> Source/WebKit/chromium/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=77229

did your changelog get messed up? This looks like the wrong bug's changelog.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:397
> +void GraphicsLayerChromium::platformChromiumLayerAnimationStarted(double startTime)

I'd just call this layerAnimationsStarted

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:101
> +    void platformChromiumLayerAnimationStarted(double startTime);

Isn't this implementing an interface?

// CCLayerAnimationDelegate implemetaiton
void blahblah()

I'd create the delegate class even if we don't have it plumbed up yet.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:122
> +void CCActiveAnimation::pushPropertiesTo(CCActiveAnimation* other) const

inline to the cloneForImplThread

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:82
> +    struct AnimationResult {

make nonnested

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:92
> +    struct AnimationResults {

make nonnested?

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:138
> +    // Does not take ownership of the given active animation.

Not sure this adds value. When you give a pointer for exchange of ownership, you're supposed to use PassOwnPtr, so the mere fact that you're not using passown explains that.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:122
> +        }

you need to m_client->setNeedsRedrawOnImplThread() here or you'll not get another frame...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:56
> +    virtual void pushAnimationResults(const CCActiveAnimation::AnimationResults&) = 0;

pushAnimationResultToMainThreadOnImplThread

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:235
> +    CCActiveAnimation::AnimationResults copy = results;

This needs to be asynchornous. How about making this a PassOwnPtr all the way down?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:542
> +{

assert main thread

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:71
> +    virtual void pushAnimationResults(const CCActiveAnimation::AnimationResults&);

We use OnImplThread suffix to denote that a method is only safe on the impl thread

Created attachment 125328 [details]
Patch

Created attachment 125404 [details]
Patch

Work in progress. Tests do not pass.

Created attachment 125431 [details]
Patch

Created attachment 125432 [details]
Patch

(In reply to comment #9)
> (From update of attachment 124966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124966&action=review
> 
> Getting close.
> 
> We need a test in CCLayerTreeHostTest that tests that adding an animation runs the animateLayers callback.
> We also need a test that verifies calling animate () on a layertreehostimpl that has a still-running animation gets its animate() called again.
Done. 
> 
> > Source/WebKit/chromium/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=77229
> 
> did your changelog get messed up? This looks like the wrong bug's changelog.
Yup :) Should be fixed now.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:397
> > +void GraphicsLayerChromium::platformChromiumLayerAnimationStarted(double startTime)
> 
> I'd just call this layerAnimationsStarted
Done.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:101
> > +    void platformChromiumLayerAnimationStarted(double startTime);
> 
> Isn't this implementing an interface?
This is coming in https://bugs.webkit.org/show_bug.cgi?id=77646. Would you be ok with putting this off until then?
> 
> // CCLayerAnimationDelegate implemetaiton
> void blahblah()
> 
> I'd create the delegate class even if we don't have it plumbed up yet.
> 
Same comment as above.

> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:122
> > +void CCActiveAnimation::pushPropertiesTo(CCActiveAnimation* other) const
> 
> inline to the cloneForImplThread
> 
Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:82
> > +    struct AnimationResult {
> 
> make nonnested
> 
Done. Added CCAnimationResults.h
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:92
> > +    struct AnimationResults {
> 
> make nonnested?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:138
> > +    // Does not take ownership of the given active animation.
> 
> Not sure this adds value. When you give a pointer for exchange of ownership, you're supposed to use PassOwnPtr, so the mere fact that you're not using passown explains that.
> 
Removed comment.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:122
> > +        }
> 
> you need to m_client->setNeedsRedrawOnImplThread() here or you'll not get another frame...
> 
I'm so sorry, Nat! I totally missed when I went through your review! This was the crucial change required to get my test to pass.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:56
> > +    virtual void pushAnimationResults(const CCActiveAnimation::AnimationResults&) = 0;
> 
> pushAnimationResultToMainThreadOnImplThread
Changed.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:235
> > +    CCActiveAnimation::AnimationResults copy = results;
> 
> This needs to be asynchornous. How about making this a PassOwnPtr all the way down?
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:542
> > +{
> 
> assert main thread
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:71
> > +    virtual void pushAnimationResults(const CCActiveAnimation::AnimationResults&);
> 
> We use OnImplThread suffix to denote that a method is only safe on the impl thread
Cool.

[Nat Duca]

Comment on attachment 125432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125432&action=review

This is awesome. Unofficial LGTM with a few nits that you can sort out with @levin and @jamesr.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:52
> +    Vector<CCAnimationResult>& startedAnimations() { return m_startedAnimations; }

should this be immutable?

const Vector<T>& started() const  { ...; }

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:51
> +void CCLayerAnimationControllerImpl::animate(double frameBeginTimeSecs, CCAnimationResults* results)

Check with jamesr about how we handle out-params in webkit, whether we do pass-by-reference or pointer, and whether the out values come before the in-values or after.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:244
> +    // FIXME: implement this. Will have to walk the tree.

Might just reference the webkit bug number where you have the patch for this, since its already done.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:260
> +    if (currentController->hasActiveAnimation()) {

is this backwards? Shouldn't it be 
currentcontroller->animate()
if hasActiveanimation()
  needsAnim = true

As implemented, this tells us whether we were active before we animated, which isn't necessarily right, right?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:578
> +        m_client->pushAnimationResultToMainThreadOnImplThread(results.release());

pushAnimationResults? Since the class being pushed is AnimationResults

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:214
> +    m_layerTreeHost->setAnimationResults(results);

I think you need an assert that we're on the impl thread and then a DebugScopedSetMainThread here so that it switches threads.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:235
> +    m_mainThreadProxy->postTask(createCCThreadTask(this, &CCThreadProxy::pushAnimationResults, AllowCrossThreadAccess(results.leakPtr())));

I think if you make CCThreadProy::pushAnimationresults(PassOwnPtr<CCAnimationResults> results), then you can pass directly instead of having to leak and adopt. Ping David Levin on gchat if you need some help doing this.

Created attachment 126515 [details]
Patch

Created attachment 126545 [details]
Patch

[Nat Duca]

Comment on attachment 126545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126545&action=review

LGTM with additional nits.

@jamesr, can you do a final review? Of course, if you've got concerns, just grab me.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:397
> +void GraphicsLayerChromium::layerAnimationStarted(double startTime)

this seems to be unused

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:545
> +}

should this setNeedsCommit? just for completeness

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:33
> +// Used to indicate that an animation event has taken place on the given layer at the given time.

Should we call this an animation started event?
E.g CCAnimationStartedEvent?

The docs would be "Indicates an animation on a particular layer started."

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:43
> +class CCAnimationResults {

Why not just do this as

typedef vector<CCAnimationStartedEvents> CCAnimationEventsVector

If we ever need to communicate other types of animation events, we can do so by changing the typedef, making a CCAnimationEvent base class, making CCAnimationStartedEvent be a subclass and done.

The side bonus of this is that you dont do results.startedAnimations().push_back(), which is just smelly imo.

> Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:53
> +    const Vector<CCAnimationResult>& statedAnimations() const { return m_startedAnimations; }

typo here --- you say stated instead of started :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:46
> +    static PassOwnPtr<CCLayerAnimationController> create()

Typically i think we put the create() impl in the cpp

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:102
> +    if (!m_pageScaleAnimation && !m_needsAnimateLayers)

Is there a way to make this whole flow less messy?

bool needsRedrawOnImplThread

if(m_pageScaleAnimation() {
  needsRedraw = true;
   .. page scale stuff
}

if (m_needsAnimateLayers)
   needsRedraw |= animateLayers()

if (needsRedraw)
 m_client->setNEedsRedrawOnImplThread

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:122
> +    m_client->setNeedsRedrawOnImplThread();

I'm a little concrned about this not being conditional on m_animate | m_pageScale. Suppose when you enter animate, you have an animation that's about to end. During the animateLayers call, it ends. We now have m_needsAnimatLayers false, but we're here asking for a redraw anyway. That's overkill. 

I think the refactoring above would clean this up so such errors wouldn't happen.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:256
> +bool CCLayerTreeHostImpl::animateLayersRecursive(CCLayerImpl* current, double frameBeginTimeSecs, CCAnimationResults& results)

Should we make make this return void and pass the bool in by ref so that its obvious what the return value is.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:271
> +}

Also note that in your current code, you're never actually clearing the m_needsAnimateLayers. You compute this local variable, but you never assign it to the actual member variable at the call site. This points to a test coverage gap. You should make anotehr test that is for "animation stops eventually after we've added an animation." Open question how to write it but clearly that can happen and its undesirable.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:115
> +// For use in the single-threaded case. In debug builds, it pretends that the

Where did we put the DebugScopedSetImplThread class? Is that in this file too? If so, thats cool. If not, lets put it wherever the other one is.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:250
> +void CCThreadProxy::pushAnimationResultsToMainThreadOnImplThread(PassOwnPtr<CCAnimationResults> results)

ugh i finally realized. This should be postAnimationResultToMainThreadOnImplThrad

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:253
> +    TRACE_EVENT("CCThreadProxy::pushAnimationResults", this, 0);

out of date string, should be pushAnimationResultsToMainThreadOnImplThread

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:560
> +void CCThreadProxy::pushAnimationResults(PassOwnPtr<CCAnimationResults> results)

and this should be setAnimationResults.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:565
> +    m_layerTreeHost->setAnimationResults(results);

and this one is correctly named.

but since i churned you on this, you're welcome to leave as is. I'm really sorry about the incorrect guidance before. :$

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:145
> +// Adapts CCLayerAnimationController for test.

Might make a note that it forces the addAnimation method to work.

Looking at this, I'd really like to do this slightly differently. Instead of making addAnimation/etc virtual on the controller, lets do the following: make the base class have a bool m_acceleratedAnimationsEnabled. The default constructor sets it to false. If false, the addAnimation method return false. If its true, it should do the activeAnimations().append stuff.

This subclass should just set the m_acceleratedAnimationsEnabled to true.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:156
> +        activeAnimations().append(CCActiveAnimation::create(adoptPtr(new FakeFloatTransition(1.0, 1.0f, 0.0f)), 0, CCActiveAnimation::Opacity));

The reason for the abovementioned refactoring is because this line here, eg. 156, isn't really test code. Rather, its the implementation code we want when the controller is turned on. The test code should be enabling this behavior in the base class, not implementing the behavior itself.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:872
> +

I think we might want to make another test that verifies that animate() stops getting called after the animation turns off.

Probably the way to do this is to get the animate set up enough that the animation turns itself off in the first call to animate. Then, hook the CCLayerTreeHostImpl's request for a redraw on the impl thread. Verify in the drawLayersOnImplThread hook that that client method wasn't called.

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:255
> +TEST_F(TreeSynchronizerTest, syncSimpleTreeThenDestroy)

Why are these using the TreeSynchronizerTest fixture for these? It seems to me that the base class you've got can be called TreeSynchronizerAnimationTest and only the animates need to  use that fixture.
Not all the tests in a the same file have to use the same fixture.

[James Robinson]

Comment on attachment 126545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126545&action=review

I don't think I'll fully paged this in, but left some comments that are hopefully helpful

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:223
> +    friend class CCLayerTreeHostTest;

this makes me really sad. if we can't write a test on the public interface, then it's a good sign that either our interface is bad or the test is bad. we have a general problem with too many unit tests depending on implementation details and not on behaviors that are important to test, which makes it really hard to do useful refactoring

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:226
> +    friend class TreeSynchronizerTest;

this too. why would TreeSynchronizerTest need to see the privates of LayerChromium? that makes no sense.  TreeSynchronizerTest should test only the TreeSynchronizer

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:47
> +CCActiveAnimation::~CCActiveAnimation() { }

in .cpp's, the braces for function definitions always get their own lines even if it's empty

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:46
>> +    static PassOwnPtr<CCLayerAnimationController> create()
> 
> Typically i think we put the create() impl in the cpp

we're pretty inconsistent on this - i think having it in the .cpp is fine. important bit is to keep the c'tor in the private: section

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:48
> +        return adoptPtr(new CCLayerAnimationController());

nit: we tend to by a slim majority omit the ()s on a no-arg c'tor call

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:51
> +    virtual ~CCLayerAnimationController() { }

please don't inline virtual d'tors, especially for classes (like this one) that have container member variables.  The problem is that doing this requires the compiler to generate code for the d'tor in every translation unit that #includes this header.  The d'tor for Vector<OwnPtr<T> > is friggin' gigantic, and we'll end up with a ton of duplicate code that the linker has to then de-duplicate and throw away. since it's a virtual call, the compiler can't actually inline the calls anyway so the linker.

this is a problem in webkit/chromium because our link-time requirements are pretty crazy (it's impossible to link in a 32-bit address space, debug builds with symbols are many gigs)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:68
> +    CCLayerAnimationController() { }

inlining the c'tor is a bad idea for the same sorts of reasons as inlining the d'tor.  Vector<OwnPtr<T> >'s constructor is pretty damn hefty

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:49
> +CCLayerAnimationControllerImpl::~CCLayerAnimationControllerImpl() { }

expand, please

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:177
> +        hostImpl->setNeedsAnimateLayers();

this is a non-obvious interaction to me...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:267
> +    // Implemented here: https://bugs.webkit.org/show_bug.cgi?id=77646

this would normally be a FIXME

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:102
>> +    if (!m_pageScaleAnimation && !m_needsAnimateLayers)
> 
> Is there a way to make this whole flow less messy?
> 
> bool needsRedrawOnImplThread
> 
> if(m_pageScaleAnimation() {
>   needsRedraw = true;
>    .. page scale stuff
> }
> 
> if (m_needsAnimateLayers)
>    needsRedraw |= animateLayers()
> 
> if (needsRedraw)
>  m_client->setNEedsRedrawOnImplThread

or how about having some helper functions? i agree this function is gettin' pretty funky

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:129
> +    // virtual for testing.

for comments in webkit, please start with a uppercase letter and end with a period (even if it isn't really a sentence)

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:156
>> +        activeAnimations().append(CCActiveAnimation::create(adoptPtr(new FakeFloatTransition(1.0, 1.0f, 0.0f)), 0, CCActiveAnimation::Opacity));
> 
> The reason for the abovementioned refactoring is because this line here, eg. 156, isn't really test code. Rather, its the implementation code we want when the controller is turned on. The test code should be enabling this behavior in the base class, not implementing the behavior itself.

floating point constants should not have trailing ".0f"s http://www.webkit.org/coding/coding-style.html "Floating point literals"

Created attachment 127649 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127649 [details]
Patch

Attachment 127649 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11541589

New failing tests:
compositing/webgl/webgl-nonpremultiplied-blend.html
compositing/iframes/iframe-content-flipping.html
platform/chromium/compositing/lost-compositor-context-with-video.html

Created attachment 127687 [details]
Patch

(In reply to comment #18)
> (From update of attachment 126545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126545&action=review
> 
> LGTM with additional nits.
> 
> @jamesr, can you do a final review? Of course, if you've got concerns, just grab me.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:397
> > +void GraphicsLayerChromium::layerAnimationStarted(double startTime)
> 
> this seems to be unused
Deleted.
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:545
> > +}
> 
> should this setNeedsCommit? just for completeness
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:33
> > +// Used to indicate that an animation event has taken place on the given layer at the given time.
> 
> Should we call this an animation started event?
> E.g CCAnimationStartedEvent?
> 
> The docs would be "Indicates an animation on a particular layer started."
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:43
> > +class CCAnimationResults {
> 
> Why not just do this as
> 
> typedef vector<CCAnimationStartedEvents> CCAnimationEventsVector
> 
> If we ever need to communicate other types of animation events, we can do so by changing the typedef, making a CCAnimationEvent base class, making CCAnimationStartedEvent be a subclass and done.
> 
> The side bonus of this is that you dont do results.startedAnimations().push_back(), which is just smelly imo.
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationResults.h:53
> > +    const Vector<CCAnimationResult>& statedAnimations() const { return m_startedAnimations; }
> 
> typo here --- you say stated instead of started :)
> 
Removed :)
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:46
> > +    static PassOwnPtr<CCLayerAnimationController> create()
> 
> Typically i think we put the create() impl in the cpp
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:102
> > +    if (!m_pageScaleAnimation && !m_needsAnimateLayers)
> 
> Is there a way to make this whole flow less messy?
> 
> bool needsRedrawOnImplThread
> 
> if(m_pageScaleAnimation() {
>   needsRedraw = true;
>    .. page scale stuff
> }
> 
> if (m_needsAnimateLayers)
>    needsRedraw |= animateLayers()
> 
> if (needsRedraw)
>  m_client->setNEedsRedrawOnImplThread
> 
Cleaned up this function. Now calls two easy-to-understand helper functions.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:122
> > +    m_client->setNeedsRedrawOnImplThread();
> 
> I'm a little concrned about this not being conditional on m_animate | m_pageScale. Suppose when you enter animate, you have an animation that's about to end. During the animateLayers call, it ends. We now have m_needsAnimatLayers false, but we're here asking for a redraw anyway. That's overkill. 
> 
> I think the refactoring above would clean this up so such errors wouldn't happen.
>
Fixed. 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:256
> > +bool CCLayerTreeHostImpl::animateLayersRecursive(CCLayerImpl* current, double frameBeginTimeSecs, CCAnimationResults& results)
> 
> Should we make make this return void and pass the bool in by ref so that its obvious what the return value is.
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:271
> > +}
> 
> Also note that in your current code, you're never actually clearing the m_needsAnimateLayers. You compute this local variable, but you never assign it to the actual member variable at the call site. This points to a test coverage gap. You should make anotehr test that is for "animation stops eventually after we've added an animation." Open question how to write it but clearly that can happen and its undesirable.
> 
m_needsAnimateLayers is set. I've modified the CCLayerTreeHostTest to check that the animation eventually stops.

> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:115
> > +// For use in the single-threaded case. In debug builds, it pretends that the
> 
> Where did we put the DebugScopedSetImplThread class? Is that in this file too? If so, thats cool. If not, lets put it wherever the other one is.
>
They're in the same spot. 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:250
> > +void CCThreadProxy::pushAnimationResultsToMainThreadOnImplThread(PassOwnPtr<CCAnimationResults> results)
> 
> ugh i finally realized. This should be postAnimationResultToMainThreadOnImplThrad
> 
Fixed.
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:253
> > +    TRACE_EVENT("CCThreadProxy::pushAnimationResults", this, 0);
> 
> out of date string, should be pushAnimationResultsToMainThreadOnImplThread
> 
Fixed.
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:560
> > +void CCThreadProxy::pushAnimationResults(PassOwnPtr<CCAnimationResults> results)
> 
> and this should be setAnimationResults.
> 
Fixed.
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:565
> > +    m_layerTreeHost->setAnimationResults(results);
> 
> and this one is correctly named.
> 
> but since i churned you on this, you're welcome to leave as is. I'm really sorry about the incorrect guidance before. :$
> 
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:145
> > +// Adapts CCLayerAnimationController for test.
> 
> Might make a note that it forces the addAnimation method to work.
> 
Done.
> Looking at this, I'd really like to do this slightly differently. Instead of making addAnimation/etc virtual on the controller, lets do the following: make the base class have a bool m_acceleratedAnimationsEnabled. The default constructor sets it to false. If false, the addAnimation method return false. If its true, it should do the activeAnimations().append stuff.
> 
> This subclass should just set the m_acceleratedAnimationsEnabled to true.
> 
This class really does stuff that's only useful for testing. I've added a FIXME to remove it (it's removed in 77229)
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:156
> > +        activeAnimations().append(CCActiveAnimation::create(adoptPtr(new FakeFloatTransition(1.0, 1.0f, 0.0f)), 0, CCActiveAnimation::Opacity));
> 
> The reason for the abovementioned refactoring is because this line here, eg. 156, isn't really test code. Rather, its the implementation code we want when the controller is turned on. The test code should be enabling this behavior in the base class, not implementing the behavior itself.
> 
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:872
> > +
> 
> I think we might want to make another test that verifies that animate() stops getting called after the animation turns off.
Done.
> 
> Probably the way to do this is to get the animate set up enough that the animation turns itself off in the first call to animate. Then, hook the CCLayerTreeHostImpl's request for a redraw on the impl thread. Verify in the drawLayersOnImplThread hook that that client method wasn't called.
> 
> > Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:255
> > +TEST_F(TreeSynchronizerTest, syncSimpleTreeThenDestroy)
> 
> Why are these using the TreeSynchronizerTest fixture for these? It seems to me that the base class you've got can be called TreeSynchronizerAnimationTest and only the animates need to  use that fixture.
> Not all the tests in a the same file have to use the same fixture.

Fixed.

(In reply to comment #19)
> (From update of attachment 126545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126545&action=review
> 
> I don't think I'll fully paged this in, but left some comments that are hopefully helpful
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:223
> > +    friend class CCLayerTreeHostTest;
> 
> this makes me really sad. if we can't write a test on the public interface, then it's a good sign that either our interface is bad or the test is bad. we have a general problem with too many unit tests depending on implementation details and not on behaviors that are important to test, which makes it really hard to do useful refactoring

I agree that this is sad, but it's very temporary. It's not possible to add animations using the public API with this patch, but this is fixed in the next patch (77229) and these lines are removed.

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:226
> > +    friend class TreeSynchronizerTest;
> 
> this too. why would TreeSynchronizerTest need to see the privates of LayerChromium? that makes no sense.  TreeSynchronizerTest should test only the TreeSynchronizer
> 
As above, this will be removed with 77229.
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:47
> > +CCActiveAnimation::~CCActiveAnimation() { }
> 
> in .cpp's, the braces for function definitions always get their own lines even if it's empty
> 
Fixed.
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:46
> >> +    static PassOwnPtr<CCLayerAnimationController> create()
> > 
> > Typically i think we put the create() impl in the cpp
> 
> we're pretty inconsistent on this - i think having it in the .cpp is fine. important bit is to keep the c'tor in the private: section
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:48
> > +        return adoptPtr(new CCLayerAnimationController());
> 
> nit: we tend to by a slim majority omit the ()s on a no-arg c'tor call
>
Fixed. 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:51
> > +    virtual ~CCLayerAnimationController() { }
> 
> please don't inline virtual d'tors, especially for classes (like this one) that have container member variables.  The problem is that doing this requires the compiler to generate code for the d'tor in every translation unit that #includes this header.  The d'tor for Vector<OwnPtr<T> > is friggin' gigantic, and we'll end up with a ton of duplicate code that the linker has to then de-duplicate and throw away. since it's a virtual call, the compiler can't actually inline the calls anyway so the linker.
> 
> this is a problem in webkit/chromium because our link-time requirements are pretty crazy (it's impossible to link in a 32-bit address space, debug builds with symbols are many gigs)
> 
Fixed.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:68
> > +    CCLayerAnimationController() { }
> 
> inlining the c'tor is a bad idea for the same sorts of reasons as inlining the d'tor.  Vector<OwnPtr<T> >'s constructor is pretty damn hefty
> 
Fixed.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:49
> > +CCLayerAnimationControllerImpl::~CCLayerAnimationControllerImpl() { }
> 
> expand, please
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:177
> > +        hostImpl->setNeedsAnimateLayers();
> 
> this is a non-obvious interaction to me...
> 
After a tree sync, an animation may have been activated, so we need to attempt to animate the layers.
Added an explanatory comment.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:267
> > +    // Implemented here: https://bugs.webkit.org/show_bug.cgi?id=77646
> 
> this would normally be a FIXME
Fixed.
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:102
> >> +    if (!m_pageScaleAnimation && !m_needsAnimateLayers)
> > 
> > Is there a way to make this whole flow less messy?
> > 
> > bool needsRedrawOnImplThread
> > 
> > if(m_pageScaleAnimation() {
> >   needsRedraw = true;
> >    .. page scale stuff
> > }
> > 
> > if (m_needsAnimateLayers)
> >    needsRedraw |= animateLayers()
> > 
> > if (needsRedraw)
> >  m_client->setNEedsRedrawOnImplThread
> 
> or how about having some helper functions? i agree this function is gettin' pretty funky
> 
Added some helper functions. Looks a lot cleaner now.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:129
> > +    // virtual for testing.
> 
> for comments in webkit, please start with a uppercase letter and end with a period (even if it isn't really a sentence)
>
Fixed. 
> >> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:156
> >> +        activeAnimations().append(CCActiveAnimation::create(adoptPtr(new FakeFloatTransition(1.0, 1.0f, 0.0f)), 0, CCActiveAnimation::Opacity));
> > 
> > The reason for the abovementioned refactoring is because this line here, eg. 156, isn't really test code. Rather, its the implementation code we want when the controller is turned on. The test code should be enabling this behavior in the base class, not implementing the behavior itself.
> 
> floating point constants should not have trailing ".0f"s http://www.webkit.org/coding/coding-style.html "Floating point literals"
Fixed.

[WebKit Review Bot]

Comment on attachment 127687 [details]
Patch

Attachment 127687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11539728

New failing tests:
compositing/webgl/webgl-nonpremultiplied-blend.html
compositing/iframes/iframe-content-flipping.html
platform/chromium/compositing/lost-compositor-context-with-video.html

[James Robinson]

Comment on attachment 127687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127687&action=review

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> +    bool addAnimation(const KeyframeValueList&, const IntSize& boxSize, const Animation*, const String& animationName, double timeOffset);

we know that we aren't sticking with strings so I think landing some code using strings is simply creating unnecessary work for us.  Keep in mind this is a large, complex codebase with lots of people contributing that we ship to millions of users on a weekly and daily basis.  Any churn that we can avoid is worth its weight in gold.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:78
> +    layer->animationController()->synchronizeAnimations(ccLayerImpl->animationController());

I'd rather you do this inside LayerChromium::pushPropertiesTo().  Ideally the synchronizer has as little logic as possible.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:47
> +CCActiveAnimation::~CCActiveAnimation() { }

more newlines, please

> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:48
> +    typedef size_t GroupID;

why do we need to bump this? are we expecting to go through > 2 billion groupids?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:29
> +#include "GraphicsLayer.h" // for KeyframeValueList

this is something that we're going to have to wrap in our own API, although we don't need to immediately. have you put much thought into how that would look?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:50
> +                                              const IntSize& /*boxSize*/,

in the implementation if you aren't going to use a parameter simply omit the name completely, no need to comment it out

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:85
> +void CCLayerAnimationController::removeAnimationsCompletedOnImplThread(CCLayerAnimationControllerImpl* controllerImpl)
> +{

any functions with specific thread requirements should ideally have thread ASSERT()s to keep us honest

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:29
> +#include "PlatformString.h"
> +#include "Vector.h"

these should be <wtf/text/WTFString.h> and <wtf/Vector.h>, respectively (although we may not end up needing the first one at all)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:48
> +CCLayerAnimationControllerImpl::~CCLayerAnimationControllerImpl() { }

expand

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:328
> +TEST_F(TreeSynchronizerAnimationTest, syncNewAnimation)

I don't think it provides us much value to have this test both the tree synchronizer and the synchronizeAnimations() function. I think a much better strategy would be to move the synchronizeAnimations() call inside of LayerChromium::pushPropertiesTo() function and rely on our other TreeSynchronizer tests to verify that the call happens. Then, test synchronizerAnimations() independently of TreeSynchronizer.  That will cut down on the number of extra fixtures / friends and boilerplate you need to generate without reducing our test coverage.

Created attachment 128023 [details]
Patch

(In reply to comment #26)
> (From update of attachment 127687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127687&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> > +    bool addAnimation(const KeyframeValueList&, const IntSize& boxSize, const Animation*, const String& animationName, double timeOffset);
> 
> we know that we aren't sticking with strings so I think landing some code using strings is simply creating unnecessary work for us.  Keep in mind this is a large, complex codebase with lots of people contributing that we ship to millions of users on a weekly and daily basis.  Any churn that we can avoid is worth its weight in gold.
> 

Fixed. Switched to IDs in this patch.

> > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:78
> > +    layer->animationController()->synchronizeAnimations(ccLayerImpl->animationController());
> 
> I'd rather you do this inside LayerChromium::pushPropertiesTo().  Ideally the synchronizer has as little logic as possible.
> 
Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:47
> > +CCActiveAnimation::~CCActiveAnimation() { }
> 
> more newlines, please
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:48
> > +    typedef size_t GroupID;
> 
> why do we need to bump this? are we expecting to go through > 2 billion groupids?
> 
No good reason. Fixed. Switched to ints for IDs.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:29
> > +#include "GraphicsLayer.h" // for KeyframeValueList
> 
> this is something that we're going to have to wrap in our own API, although we don't need to immediately. have you put much thought into how that would look?
> 
I have an idea, yes. In the patch that introduces the keyframed animation classes, I have added code in CCLayerAnimationController to translate from KeyframeValueList to a collection CCFloatKeyframes (or CCTransformKeyframes). This translation could certainly happen closer to (or in) GraphicsLayerChromium  to avoid plumbing the KeyframeValueList all the way to the CCLayerAnimationController.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:50
> > +                                              const IntSize& /*boxSize*/,
> 
> in the implementation if you aren't going to use a parameter simply omit the name completely, no need to comment it out
> 
Removed.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:85
> > +void CCLayerAnimationController::removeAnimationsCompletedOnImplThread(CCLayerAnimationControllerImpl* controllerImpl)
> > +{
> 
> any functions with specific thread requirements should ideally have thread ASSERT()s to keep us honest
> 
This function was poorly named. It does not need to execute on the impl thread. I've renamed it.
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:29
> > +#include "PlatformString.h"
> > +#include "Vector.h"
> 
> these should be <wtf/text/WTFString.h> and <wtf/Vector.h>, respectively (although we may not end up needing the first one at all)
> 
Fixed (switched to <wtf/Vector.h> and removed the string-related include).
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationControllerImpl.cpp:48
> > +CCLayerAnimationControllerImpl::~CCLayerAnimationControllerImpl() { }
> 
> expand
> 
Done.
> > Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:328
> > +TEST_F(TreeSynchronizerAnimationTest, syncNewAnimation)
> 
> I don't think it provides us much value to have this test both the tree synchronizer and the synchronizeAnimations() function. I think a much better strategy would be to move the synchronizeAnimations() call inside of LayerChromium::pushPropertiesTo() function and rely on our other TreeSynchronizer tests to verify that the call happens. Then, test synchronizerAnimations() independently of TreeSynchronizer.  That will cut down on the number of extra fixtures / friends and boilerplate you need to generate without reducing our test coverage.

Fixed. I've added a single test to the TreeSynchroniserTest that ensures that CCLayerAnimationController::synchronizeAnimations is called and I've moved all the tests for synchronizeAnimations into (the newly created) CCLayerAnimationControllerTest.cpp.

[WebKit Review Bot]

Comment on attachment 128023 [details]
Patch

Attachment 128023 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11558321

New failing tests:
compositing/webgl/webgl-nonpremultiplied-blend.html
compositing/iframes/iframe-content-flipping.html
platform/chromium/compositing/lost-compositor-context-with-video.html

[James Robinson]

(somehow I wasn't on the CC list for this bug so I haven't been notified of any changes. whoops!)

[James Robinson]

Comment on attachment 128023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128023&action=review

Great, R=me!  I have a few nits, but nothing serious.

Can you confirm whether those test failures from the cr-linux EWS bot are due to this patch and if so what's going on?  If it's nothing to do with this feel free to upload another patch with 'Reviewed by James Robinson.' lines in the ChangeLogs instead of 'Reviewed by NOBODY (OOPS!)' without a "review?" flag and ask any committer to set commit-queue+.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:82
> +// Looking at GraphicsLayerCA, this appears to be the analog to suspendAnimations, which is for testing.

note: FYI, i think this might someday be used by explicit JavaScript animation APIs. such APIs don't exist today, but dino@apple has been working on something in this area.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:61
> +    , m_needsAnimateLayers(true)

shouldn't this initially be false and be set true by the first tree sync?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:593
> +    if (!m_needsAnimateLayers || !m_rootLayerImpl.get())

nit: all WebKit smart pointer type (OwnPtr/RefPtr) have operator bool() overrides that do the expected thing and the style is to prefer these to null checking the raw pointer. this means to null-check a pointer just write "!m_rootLayerImpl" not "!m_rootLayerImpl.get()"

> Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp:104
> +    const bool synchronizedAnimations() const { return m_synchronizedAnimations; }

const on a bool return type is meaningless - it's returned by value so the caller gets their own copy anyway

Created attachment 128231 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128231 [details]
Patch

Attachment 128231 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11562558

New failing tests:
compositing/webgl/webgl-nonpremultiplied-blend.html
compositing/iframes/iframe-content-flipping.html
platform/chromium/compositing/lost-compositor-context-with-video.html

Created attachment 128279 [details]
Patch

(In reply to comment #34)
> Created an attachment (id=128279) [details]
> Patch

Things to note in this latest patch
 - Enabled threaded animation in CCLayerTreeHostTest
 - Removed an extra call to m_client->setNeedsRedrawOnImplThread in CCLayerTreeHostImpl::animateLayers
 - Added extra logic to CCLayerTreeHostImpl::animateLayers to
   a) make sure that we only request a redraw when we've actually animated.
   b) make sure that we don't walk the layer tree when threaded animations are disabled.

[James Robinson]

Comment on attachment 128279 [details]
Patch

OK!

[WebKit Review Bot]

Comment on attachment 128279 [details]
Patch

Clearing flags on attachment: 128279

Committed r108581: <http://trac.webkit.org/changeset/108581>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Committed r108593: <http://trac.webkit.org/changeset/108593>