When an element has multiple animations that have different timingFunctions, the progress of each animation should be calculated using its respective timingFunction. But at this point, the timingFunction of the first animation is only used in RenderLayerBacking::startAnimation(), regardless of how many animations the element has. getAnimationFromStyleByName() was introduced in r89462 so that the timingFunction of the correct animation searched by its name will be used. This patch moves the function to KeyframeValue::timingFunction() so that RenderLayerBacking reuses this method.
Created attachment 171163 [details] Patch
I think this bug follow up Bug 60303.
Comment on attachment 171163 [details] Patch The RenderLayerBacking change isn't tested by the test; a pixel test would be required for that. Do we have sufficient test coverage with timing functions inside keyframes to be confident that you're not breaking those?
(In reply to comment #3) > (From update of attachment 171163 [details]) Thanks for review! > The RenderLayerBacking change isn't tested by the test; a pixel test would be required for that. Is it because expectedValues can not reflect the progress of animations by GraphicsLayer? Could you explain how we make a pixel test on animations? > Do we have sufficient test coverage with timing functions inside keyframes to be confident that you're not breaking those? I think we do not have sufficient test coverage with timing functions inside keyframes. I can make a test like animations/keyframe-timing-functions.html for the coverage. However, I have no idea how we perform a pixel test. I want your advice.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 171163 [details] [details]) > Thanks for review! > > > The RenderLayerBacking change isn't tested by the test; a pixel test would be required for that. > > Is it because expectedValues can not reflect the progress of animations by GraphicsLayer? Could you explain how we make a pixel test on animations? Right, expected values are just computed at the RenderStyle/AnimationController level. GraphicsLayer animation can only be tested via pixel tests. > > Do we have sufficient test coverage with timing functions inside keyframes to be confident that you're not breaking those? > > I think we do not have sufficient test coverage with timing functions inside keyframes. I can make a test like animations/keyframe-timing-functions.html for the coverage. However, I have no idea how we perform a pixel test. I want your advice. runAnimationTest() has a param to do a pixel test.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 171163 [details] [details] [details]) > Right, expected values are just computed at the RenderStyle/AnimationController level. GraphicsLayer animation can only be tested via pixel tests. > runAnimationTest() has a param to do a pixel test. Thanks, I'll try.
Comment on attachment 171163 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Comment on attachment 171163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171163&action=review There looks like a logic error/dead code in your implementation of timingFunction. Please fix! :-) > Source/WebCore/rendering/style/KeyframeList.cpp:38 > + return keyframeStyle->animations()->animation(i) ? keyframeStyle->animations()->animation(i)->timingFunction() : 0; This looks wrong. We would blow up here if !keyframeStyle->animations()->animation(i), so either this test isn't necessary, or we need to be checking before we look at the name. Given that the original code didn't do this, I think we can just omit the test.
Created attachment 229485 [details] Patch
Updated ~2 year old patch so it can be reviewed/applied now.
Comment on attachment 229485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229485&action=review > Source/WebCore/rendering/style/KeyframeList.cpp:30 > +const PassRefPtr<TimingFunction> KeyframeValue::timingFunction(const AtomicString& name) const This should return a raw cost TimingFunction*; it's not transferring ownership. Also the function should be timingFunctionForAnimation(). > Source/WebCore/rendering/style/KeyframeList.cpp:38 > + if (name == keyframeStyle->animations()->animation(i).name()) > + return keyframeStyle->animations()->animation(i).timingFunction(); Cache keyframeStyle->animations()->animation(i) in a variable.
Comment on attachment 229485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229485&action=review >> Source/WebCore/rendering/style/KeyframeList.cpp:30 >> +const PassRefPtr<TimingFunction> KeyframeValue::timingFunction(const AtomicString& name) const > > This should return a raw cost TimingFunction*; it's not transferring ownership. > > Also the function should be timingFunctionForAnimation(). Although the original helper function in KeyFrameAnimation.cpp only returned a bare pointer, several of the places in RenderLayerBacking expect to consume a RefPtr. I'd like to return the PassRefPtr so we keep a proper retain count across the various places using the timing function.
Created attachment 229509 [details] Patch
(In reply to comment #12) > Although the original helper function in KeyFrameAnimation.cpp only returned a bare pointer, several of the places in RenderLayerBacking expect to consume a RefPtr. I'd like to return the PassRefPtr so we keep a proper retain count across the various places using the timing function. But PassRefPtr implies transfer of ownership. Returning a raw pointer is appropriate if the caller is just going to take another reference.
(In reply to comment #14) > (In reply to comment #12) > > Although the original helper function in KeyFrameAnimation.cpp only returned a bare pointer, several of the places in RenderLayerBacking expect to consume a RefPtr. I'd like to return the PassRefPtr so we keep a proper retain count across the various places using the timing function. > > But PassRefPtr implies transfer of ownership. Returning a raw pointer is appropriate if the caller is just going to take another reference. I always thought of it as expressing the intention of the receiver to "hold a reference" on the object until it is finished being used. I'm happy using bare pointers, but consider the implementation of GraphicLayer.h. The AnimationValue constructor wants to be given a PassRefPtr. If I just call adoptRef on the passed pointer, won't it just destroy that function object once the AnimationValue leaves scope? Or are you suggesting we modify GraphicsLayer to only hold a bare pointer to the timing function? Or should I be cloning the function pointer somehow?
Let's chat about this tomorrow.
Created attachment 229573 [details] Includes Xcode settings. Whoops!
Comment on attachment 229573 [details] Includes Xcode settings. Whoops! View in context: https://bugs.webkit.org/attachment.cgi?id=229573&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:93 > - RefPtr<TimingFunction> m_timingFunction; > + const TimingFunction* m_timingFunction; I think this still needs to be a RefPtr. We have no guarantee that the AnimationValue won't outlive whoever holds onto the TimingFunction*.
Created attachment 229582 [details] Ditto.
Created attachment 229583 [details] Patch
Created attachment 229600 [details] Add build fix for EFL
Committed r167472: <http://trac.webkit.org/changeset/167472>