RESOLVED FIXED 100632
Make RenderLayerBacking get the timingFunction of the correct animation.
https://bugs.webkit.org/show_bug.cgi?id=100632
Summary Make RenderLayerBacking get the timingFunction of the correct animation.
Dongseong Hwang
Reported 2012-10-28 23:49:48 PDT
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.
Attachments
Patch (9.79 KB, patch)
2012-10-28 23:57 PDT, Dongseong Hwang
no flags
Patch (9.20 KB, patch)
2014-04-16 15:06 PDT, Brent Fulgham
no flags
Patch (9.41 KB, patch)
2014-04-16 17:56 PDT, Brent Fulgham
no flags
Includes Xcode settings. Whoops! (15.80 KB, patch)
2014-04-17 14:05 PDT, Brent Fulgham
no flags
Ditto. (15.31 KB, patch)
2014-04-17 15:26 PDT, Brent Fulgham
no flags
Patch (12.88 KB, patch)
2014-04-17 15:28 PDT, Brent Fulgham
no flags
Add build fix for EFL (15.22 KB, patch)
2014-04-17 17:31 PDT, Brent Fulgham
simon.fraser: review+
Dongseong Hwang
Comment 1 2012-10-28 23:57:39 PDT
Dongseong Hwang
Comment 2 2012-10-29 02:30:17 PDT
I think this bug follow up Bug 60303.
Simon Fraser (smfr)
Comment 3 2012-10-29 09:09:35 PDT
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?
Dongseong Hwang
Comment 4 2012-10-29 15:01:30 PDT
(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.
Simon Fraser (smfr)
Comment 5 2012-10-29 15:06:09 PDT
(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.
Dongseong Hwang
Comment 6 2012-10-29 15:17:43 PDT
(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.
Anders Carlsson
Comment 7 2014-02-05 11:08:47 PST
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.
Brent Fulgham
Comment 8 2014-04-16 14:15:54 PDT
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.
Brent Fulgham
Comment 9 2014-04-16 15:06:57 PDT
Brent Fulgham
Comment 10 2014-04-16 15:08:34 PDT
Updated ~2 year old patch so it can be reviewed/applied now.
Simon Fraser (smfr)
Comment 11 2014-04-16 15:17:35 PDT
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.
Brent Fulgham
Comment 12 2014-04-16 17:47:39 PDT
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.
Brent Fulgham
Comment 13 2014-04-16 17:56:00 PDT
Simon Fraser (smfr)
Comment 14 2014-04-16 19:51:30 PDT
(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.
Brent Fulgham
Comment 15 2014-04-16 20:01:09 PDT
(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?
Simon Fraser (smfr)
Comment 16 2014-04-16 20:22:08 PDT
Let's chat about this tomorrow.
Brent Fulgham
Comment 17 2014-04-17 14:05:57 PDT
Created attachment 229573 [details] Includes Xcode settings. Whoops!
Simon Fraser (smfr)
Comment 18 2014-04-17 14:19:43 PDT
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*.
Brent Fulgham
Comment 19 2014-04-17 15:26:33 PDT
Brent Fulgham
Comment 20 2014-04-17 15:28:52 PDT
Brent Fulgham
Comment 21 2014-04-17 17:31:55 PDT
Created attachment 229600 [details] Add build fix for EFL
Brent Fulgham
Comment 22 2014-04-17 17:45:22 PDT
Note You need to log in before you can comment on or make changes to this bug.