Bug 100632 - Make RenderLayerBacking get the timingFunction of the correct animation.
Summary: Make RenderLayerBacking get the timingFunction of the correct animation.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-28 23:49 PDT by Dongseong Hwang
Modified: 2014-04-17 17:45 PDT (History)
16 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 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.
Comment 1 Dongseong Hwang 2012-10-28 23:57:39 PDT
Created attachment 171163 [details]
Patch
Comment 2 Dongseong Hwang 2012-10-29 02:30:17 PDT
I think this bug follow up Bug 60303.
Comment 3 Simon Fraser (smfr) 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?
Comment 4 Dongseong Hwang 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.
Comment 5 Simon Fraser (smfr) 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.
Comment 6 Dongseong Hwang 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.
Comment 7 Anders Carlsson 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.
Comment 8 Brent Fulgham 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.
Comment 9 Brent Fulgham 2014-04-16 15:06:57 PDT
Created attachment 229485 [details]
Patch
Comment 10 Brent Fulgham 2014-04-16 15:08:34 PDT
Updated ~2 year old patch so it can be reviewed/applied now.
Comment 11 Simon Fraser (smfr) 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.
Comment 12 Brent Fulgham 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.
Comment 13 Brent Fulgham 2014-04-16 17:56:00 PDT
Created attachment 229509 [details]
Patch
Comment 14 Simon Fraser (smfr) 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.
Comment 15 Brent Fulgham 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?
Comment 16 Simon Fraser (smfr) 2014-04-16 20:22:08 PDT
Let's chat about this tomorrow.
Comment 17 Brent Fulgham 2014-04-17 14:05:57 PDT
Created attachment 229573 [details]
Includes Xcode settings. Whoops!
Comment 18 Simon Fraser (smfr) 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*.
Comment 19 Brent Fulgham 2014-04-17 15:26:33 PDT
Created attachment 229582 [details]
Ditto.
Comment 20 Brent Fulgham 2014-04-17 15:28:52 PDT
Created attachment 229583 [details]
Patch
Comment 21 Brent Fulgham 2014-04-17 17:31:55 PDT
Created attachment 229600 [details]
Add build fix for EFL
Comment 22 Brent Fulgham 2014-04-17 17:45:22 PDT
Committed r167472: <http://trac.webkit.org/changeset/167472>