Bug 53513 – LayoutTests/animations/play-state.html has wrong behavior with accelerated compositing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Marrin]

The top blue box is animated using -webkit-transform. It should stop after 2 sec and then continue from where it left off. But it continues to animation and then after 1 more second it hops back to where the red box is and continues its animation. The test passes because when we see the animation is stopped we simply return the current software value so we miss the fact that the animation is still running.

This is most likely a regression caused by recent restructuring of the accelerated animation code. We are probably failing to remove the hardware animation when a pause comes in.

[Chris Marrin]

Created attachment 81322 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 81322 [details]
Patch

The copy ctor should take a const&

[Chris Marrin]

Created attachment 81500 [details]
Patch

[Eric Seidel (no email)]

This feels kinda strange to use Foo::create(*foo.get()).  I wonder if an explicit ->copy() call would be claeaner since there you can do Foo::create(*this) which I think is more common to see.

Not sure.  I'm trying to think of places where we have objects was pass around as pointer and yet have copy constructors.  I can't think of any.

[James Robinson]

Comment on attachment 81500 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1510
> +    RefPtr<PlatformCAAnimation> newAnim = PlatformCAAnimation::create(*(curAnim.get()));

nit: *curAnim should do the same thing as *(curAnim.get()) for all of the WTF smart pointer types thanks to the operator* overload on them.

[Chris Marrin]

(In reply to comment #5)
> (From update of attachment 81500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81500&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1510
> > +    RefPtr<PlatformCAAnimation> newAnim = PlatformCAAnimation::create(*(curAnim.get()));
> 
> nit: *curAnim should do the same thing as *(curAnim.get()) for all of the WTF smart pointer types thanks to the operator* overload on them.

Good point. Actually this whole thing seems wrong. I put in my changelog that I was messing with the "copy constructor". Simon called me on it, saying that I was using the wrong signature for copy constructors, so I naively fixed that. The problem is that this isn't really a copy constructor at all. You can't call the constructor at all in fact. You have to go through the create method which doesn't have any implicit copy semantics as far as C++ is concerned.

So I think I should go back to the old call signature. Maybe i should call this method "copy()" rather than "create()" although that would break with convention a bit.

[Eric Seidel (no email)]

You could just have a ->copy() method on the instance which returns a new instance, no?

[Chris Marrin]

(In reply to comment #7)
> You could just have a ->copy() method on the instance which returns a new instance, no?

Sure, I've never seen that pattern. Typically these functions are called create() with the same signature as the ctor. But I'll call it copy to avoid the local confusion.

[Darin Adler]

(In reply to comment #8)
> Sure, I've never seen that pattern. Typically these functions are called create() with the same signature as the ctor. But I'll call it copy to avoid the local confusion.

I looked around for examples, and found them in these files:

CSSStyleDeclaration.h
CSSValueList.h
EditingStyle.h
FormData.h
HistoryItem.h
SVGPathByteStream.h
SVGRenderStyle.h
SVGRenderStyleDefs.h
SharedBuffer.h
StorageAreaImpl.h
StorageMap.h
StorageNamespace.h
StyleBackgroundData.h
StyleBoxData.h
StyleFlexibleBoxData.h
StyleInheritedData.h
StyleMarqueeData.h
StyleMultiColData.h
StyleRareInheritedData.h
StyleRareNonInheritedData.h
StyleSurroundData.h
StyleTransformData.h
StyleVisualData.h

[Chris Marrin]

Created attachment 81659 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 81659 [details]
Patch

copy() should be a const method on the original, that returns a copy.

[Chris Marrin]

Created attachment 81736 [details]
Patch

[Build Bot]

Attachment 81736 [details] did not build on win:
Build output: http://queues.webkit.org/results/7791258

[Simon Fraser (smfr)]

Comment on attachment 81736 [details]
Patch

r=me assuming you fix the Windows build failure:
PlatformCAAnimationWin.cpp(190) : error C3861: 'setNonZeroBeginTimeFlag': identifier not found

[Chris Marrin]

Committed r78051: <http://trac.webkit.org/changeset/78051>

[Dean Jackson]

*** Bug 54261 has been marked as a duplicate of this bug. ***