WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(9.20 KB, patch)
2014-04-16 15:06 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(9.41 KB, patch)
2014-04-16 17:56 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Includes Xcode settings. Whoops!
(15.80 KB, patch)
2014-04-17 14:05 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Ditto.
(15.31 KB, patch)
2014-04-17 15:26 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(12.88 KB, patch)
2014-04-17 15:28 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Add build fix for EFL
(15.22 KB, patch)
2014-04-17 17:31 PDT
,
Brent Fulgham
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Dongseong Hwang
Comment 1
2012-10-28 23:57:39 PDT
Created
attachment 171163
[details]
Patch
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
Created
attachment 229485
[details]
Patch
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
Created
attachment 229509
[details]
Patch
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
Created
attachment 229582
[details]
Ditto.
Brent Fulgham
Comment 20
2014-04-17 15:28:52 PDT
Created
attachment 229583
[details]
Patch
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
Committed
r167472
: <
http://trac.webkit.org/changeset/167472
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug