Bug 77646 – [chromium] Plumb animation started notifications from CCLayerTreeHost to GraphicsLayerChromium

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

We must notify the GraphicsLayer's client when we've started an animation on this layer. We need to get the animation started results (which are plumbed to CCLayerTreeHost) to their respective GraphicsLayerChromiums so that this notification can take place.

Created attachment 125148 [details]
Patch

[Nat Duca]

Comment on attachment 125148 [details]
Patch

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

Looks good but we need to go to an AnimationDelegate model rather than a generic LayerChromiumClient.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:78
> +    m_layer->setClient(this);

People would like us to use an animation delegate model.

E.g. CCLayerChromium::setAnimationDelegate(this)

Where CCLayerChromiumAnimationDelegate has our notifyAnimationStarted method.

We should only set the delegate when we have an animation. Though, we don't have to clear the delegate when stop having an animation --- just make sure you clear it during destruction. :)

Created attachment 126145 [details]
Patch

[Nat Duca]

Comment on attachment 126145 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:79
>  

I'd like this to only be set when you add an animation. Once set, it can be left on forever. Essentially, remove this line, then on the *Animation methods, put a call like setLayerAnimationDelegateIfNeeded() before using the layer. That should set the delegate, if its not already set.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:88
> +        m_layer->clearLayerAnimationDelegate();

Why clear and not just setAnimationDelegate(0)?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:94
> +        m_layer->clearLayerAnimationDelegate();

what's this? And why is it conditional on m_transformLayer?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:378
> +    return primaryLayer()->addAnimation(values, boxSize, animation, animationName, timeOffset);

is primaryLayer always going to have an animation delegate pointing at us?  What if primaryLayer is a transform layer?

Again, i'd prefer we did a setLayerDelegate(primaryLayer()) call here to prevent this mess.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:46
> +class GraphicsLayerChromium : public GraphicsLayer,

Check webkit style, is this normative?

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:223
> +    void clearLayerAnimationDelegate() { m_layerAnimationDelegate = 0; }

Not sure this makes sense when you have a setter above.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:270
> +    setAnimationResultsRecursive(results.get(), m_rootLayer.get());

if you use a const reference to the CCAnimationResults, then you can just do *results here. I definitely prefer being const-correct in this case --- we're not mutating the results.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:606
> +{

Is there any way to normalize between the name "setAnimationResults" and "nofityAnimations"? It'd be cool if it was CCLTH::notifyLayersOfAnimationResults(CCAnimationResults*) or something. Please dont use that name, it kinda sucks. But hopefully you get my point.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:232
>  

Looks like you did a bugfix in the animationcontrollerimpl. You should have a test for the controller that catches this mistake, if possible... if not, nbd. But its a good corner case you uncovered.

Do you have tests for the layer delegate being called correctly? Smells like a test for the CCLayerTreeHostTest file

Created attachment 126595 [details]
Patch

[James Robinson]

Comment on attachment 126595 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

don't change copyright header dates when modifying the file, just leave them alone

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:227
> +    friend class CCLayerTreeHost;

It would be much, much better to have notifyAnimationStarted() be public than to friend CCLayerTreeHost

Created attachment 128809 [details]
Patch

Created attachment 128811 [details]
Patch

Noteworthy changes:
 - There are now two types of animation event
   1) Started (webkit needs this notification)
   2) Finished (GraphicsLayerChromium needs this to update its animation id map)

 - I now test that these events get sent in response to an animation being started and finished (CCLayerTreeHostTest.cpp)

(In reply to comment #6)
> (From update of attachment 126595 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126595&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:2
> > + * Copyright (C) 2012 Google Inc. All rights reserved.
> 
> don't change copyright header dates when modifying the file, just leave them alone
> 
Done.
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:227
> > +    friend class CCLayerTreeHost;
> 
> It would be much, much better to have notifyAnimationStarted() be public than to friend CCLayerTreeHost
Done.

[James Robinson]

Comment on attachment 128811 [details]
Patch

I think there's an issue with startTime here - on the impl side we're using the monotonicallyIncreasingTime() clock, but GraphicsLayerClient is expecting the currentTime() timebase.  I think we need a re-normalization pass on the time values somewhere (probably GraphicsLayerChromium) in case the timebases are different for the two time sources.  As we discussed offline I think it's fine to file a bug on that issue separately and take care of it before we turn this on by default since it'll be pretty isolated from the rest of this patch, which looks great.

R=me!

[WebKit Review Bot]

Comment on attachment 128811 [details]
Patch

Clearing flags on attachment: 128811

Committed r108880: <http://trac.webkit.org/changeset/108880>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.