Summary: | Make RenderLayerBacking get the timingFunction of the correct animation. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bfulgham, bunhere, cmarcelo, commit-queue, dino, dstockwell, eric, esprehn+autocc, glenn, kondapallykalyan, luiz, noam, sergio, simon.fraser, webkit.review.bot, zeno | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Dongseong Hwang
2012-10-28 23:49:48 PDT
Created attachment 171163 [details]
Patch
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> |