Bug 181428 – Refactor timing function solving code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

Refactor timing function solving code

[Antoine Quint]

Created attachment 330804 [details]
Patch

[Antoine Quint]

Created attachment 330821 [details]
Patch

[Antoine Quint]

Created attachment 330822 [details]
Patch

[Darin Adler]

Comment on attachment 330822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330822&action=review

> Source/WebCore/platform/animation/TimingFunction.cpp:71
> +        return std::min(1.0, (floor(m_steps * inputTime) + 1) / m_steps);

Should use std::floor.

> Source/WebCore/platform/animation/TimingFunction.cpp:72
> +    return floor(m_steps * inputTime) / m_steps;

Should use std::floor.

[EWS Watchlist]

Comment on attachment 330822 [details]
Patch

Attachment 330822 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5998231

New failing tests:
imported/w3c/web-platform-tests/css/css-ui-3/caret-color-021.html
transitions/steps-timing-function.html
animations/animation-direction-reverse-timing-functions.html
animations/keyframe-multiple-timing-functions-transform.html
animations/fill-mode-iteration-count-non-integer.html
fast/css-generated-content/pseudo-animation.html
animations/multiple-animations-timing-function.html
animations/missing-keyframe-properties-timing-function.html
fast/css-generated-content/pseudo-transition.html
animations/spring-function.html
transitions/default-timing-function.html
transitions/transition-display-property-2.html
animations/trigger-container-scroll-simple.html
animations/animation-direction-reverse-timing-functions-hardware.html
animations/timing-functions.html

[EWS Watchlist]

Created attachment 330825 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 330822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330822&action=review

> Source/WebCore/platform/animation/TimingFunction.h:53
> +    double transformTime(double inputTime, double) const { return inputTime; }

Need to mark this virtual.

> Source/WebCore/platform/animation/TimingFunction.h:160
> +    double transformTime(double, double) const;

Need to mark this final.

> Source/WebCore/platform/animation/TimingFunction.h:210
> +    double transformTime(double, double) const;

Need to mark this final.

> Source/WebCore/platform/animation/TimingFunction.h:264
> +    double transformTime(double, double) const;

Need to mark this final.

[EWS Watchlist]

Comment on attachment 330822 [details]
Patch

Attachment 330822 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5998401

New failing tests:
animations/multiple-animations-timing-function.html
transitions/steps-timing-function.html
fast/css-generated-content/pseudo-animation.html
animations/keyframe-multiple-timing-functions-transform.html
animations/fill-mode-iteration-count-non-integer.html
animations/animation-direction-reverse-timing-functions.html
imported/w3c/web-platform-tests/css/css-ui-3/caret-color-021.html
fast/css-generated-content/pseudo-transition.html
animations/spring-function.html
transitions/transition-display-property-2.html
animations/animation-direction-reverse-timing-functions-hardware.html
animations/trigger-container-scroll-simple.html
animations/animation-initial-inheritance.html
animations/missing-keyframe-properties-timing-function.html
animations/timing-functions.html

[EWS Watchlist]

Created attachment 330826 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Antoine Quint]

(In reply to Darin Adler from comment #7)
> Comment on attachment 330822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330822&action=review
> 
> > Source/WebCore/platform/animation/TimingFunction.h:53
> > +    double transformTime(double inputTime, double) const { return inputTime; }
> 
> Need to mark this virtual.

There was a linking issue with a virtual method here that I couldn't fix, even with the help of some fine folks on #webkit. I'm preparing a new patch where we use a single method on TimingFunction that downcasts based on the type.

[Antoine Quint]

Created attachment 330827 [details]
Patch

[Antoine Quint]

(In reply to Darin Adler from comment #4)
> Comment on attachment 330822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330822&action=review
> 
> > Source/WebCore/platform/animation/TimingFunction.cpp:71
> > +        return std::min(1.0, (floor(m_steps * inputTime) + 1) / m_steps);
> 
> Should use std::floor.
> 
> > Source/WebCore/platform/animation/TimingFunction.cpp:72
> > +    return floor(m_steps * inputTime) / m_steps;
> 
> Should use std::floor.

The new patch uses std::floor.

[Dean Jackson]

Comment on attachment 330827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330827&action=review

> Source/WebCore/platform/animation/TimingFunction.cpp:64
> +        auto& function = *static_cast<const CubicBezierTimingFunction*>(this);

Maybe we should have is<> and downcast<> for these?

> Source/WebCore/platform/animation/TimingFunction.cpp:66
> +        auto epsilon = 1.0 / (200.0 * duration);
> +        return UnitBezier(function.x1(), function.y1(), function.x2(), function.y2()).solve(inputTime, epsilon);

Any reason you moved the epsilon to a variable? This means we no longer have the comment on why we change the accuracy for longer durations.

[Antoine Quint]

Committed r226645: <https://trac.webkit.org/changeset/226645>

[Antoine Quint]

(In reply to Dean Jackson from comment #13)
> Comment on attachment 330827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330827&action=review
> 
> > Source/WebCore/platform/animation/TimingFunction.cpp:66
> > +        auto epsilon = 1.0 / (200.0 * duration);
> > +        return UnitBezier(function.x1(), function.y1(), function.x2(), function.y2()).solve(inputTime, epsilon);
> 
> Any reason you moved the epsilon to a variable? This means we no longer have
> the comment on why we change the accuracy for longer durations.

I added it back in the commit.

[Radar WebKit Bug Importer]

<rdar://problem/36378011>