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.
Created attachment 81322 [details] Patch
Comment on attachment 81322 [details] Patch The copy ctor should take a const&
Created attachment 81500 [details] Patch
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.
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.
(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.
You could just have a ->copy() method on the instance which returns a new instance, no?
(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.
(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
Created attachment 81659 [details] Patch
Comment on attachment 81659 [details] Patch copy() should be a const method on the original, that returns a copy.
Created attachment 81736 [details] Patch
Attachment 81736 [details] did not build on win: Build output: http://queues.webkit.org/results/7791258
Comment on attachment 81736 [details] Patch r=me assuming you fix the Windows build failure: PlatformCAAnimationWin.cpp(190) : error C3861: 'setNonZeroBeginTimeFlag': identifier not found
Committed r78051: <http://trac.webkit.org/changeset/78051>
*** Bug 54261 has been marked as a duplicate of this bug. ***