Make RenderLayerBacking get the timingFunction of the correct animation.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 171163 [details]
Patch

[Dongseong Hwang]

I think this bug follow up Bug 60303.

[Simon Fraser (smfr)]

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]

(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)]

(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]

(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 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 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]

Created attachment 229485 [details]
Patch

[Brent Fulgham]

Updated ~2 year old patch so it can be reviewed/applied now.

[Simon Fraser (smfr)]

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 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]

Created attachment 229509 [details]
Patch

[Simon Fraser (smfr)]

(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]

(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)]

Let's chat about this tomorrow.

[Brent Fulgham]

Created attachment 229573 [details]
Includes Xcode settings. Whoops!

[Simon Fraser (smfr)]

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]

Created attachment 229582 [details]
Ditto.

[Brent Fulgham]

Created attachment 229583 [details]
Patch

[Brent Fulgham]

Created attachment 229600 [details]
Add build fix for EFL

[Brent Fulgham]

Committed r167472: <http://trac.webkit.org/changeset/167472>