Add experimental support for spring based CSS animations

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Add experimental support for spring based CSS animations

[Sam Weinig]

Created attachment 280564 [details]
Patch

[Darin Adler]

Comment on attachment 280564 [details]
Patch

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

Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:380:12: error: enumeration value 'SpringFunction' not handled in switch [-Werror=switch]

> Source/WebCore/css/CSSParser.h:218
> +    bool parseSpringTimingFunctionValue(CSSParserValueList& args, double& result);

I’d use Optional<double> instead, in this new code. I know the old code does not work that way.

> Source/WebCore/css/CSSTimingFunctionValue.cpp:46
> +    StringBuilder builder;
> +    builder.appendLiteral("cubic-bezier(");
> +    builder.appendNumber(m_x1);
> +    builder.appendLiteral(", ");
> +    builder.appendNumber(m_y1);
> +    builder.appendLiteral(", ");
> +    builder.appendNumber(m_x2);
> +    builder.appendLiteral(", ");
> +    builder.appendNumber(m_y2);
> +    builder.append(')');
> +    
> +    return builder.toString();

So makeString can’t do this kind of job with the numeric conversion without temporary strings?

I’d leave out the blank line before calling toString.

> Source/WebCore/page/animation/AnimationBase.cpp:77
> +    SpringSolver solver(mass, stiffness, damping, initialVelocity);
> +    return solver.solve(t * duration);

I would be tempted to write this without the local variable, but maybe the syntax would be ugly.

> Source/WebCore/platform/animation/TimingFunction.h:244
> +        return adoptRef(*new SpringTimingFunction(1, 100, 10, 0));

Not clear why these are the default values. Might need a comment.

Maybe just have this one call the other create function.

> Source/WebCore/platform/animation/TimingFunction.h:249
> +    virtual ~SpringTimingFunction()
> +    {
> +    }

I don’t see any reason to define this explicitly. Sometimes there could be value in that if we defined it in a cpp file, but if we are writing this inline version, I think just not writing anything is a better way to express it.

> Source/WebCore/platform/animation/TimingFunction.h:284
> +    PassRefPtr<TimingFunction> clone() const override

Maybe just final instead of override.

> Source/WebCore/platform/graphics/SpringSolver.h:34
> +        m_w0 = sqrt(stiffness / mass);

Better to use std::sqrt; we normally do that so we get overloading for types. But since this is double doesn’t matter in practice.

> Source/WebCore/platform/graphics/SpringSolver.h:53
> +            t = exp(-t * m_zeta * m_w0) * (m_A * cos(m_wd * t) + m_B * sin(m_wd * t));

std::exp, std::cos, std::sin

> Source/WebCore/platform/graphics/SpringSolver.h:56
> +            t = (m_A + m_B * t) * exp(-t * m_w0);

std::exp

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2919
> +    RefPtr<PlatformCAAnimation> basicAnim = createPlatformCAAnimation(PlatformCAAnimation::Spring, keyPath);

How about auto?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:245
> +    PassRefPtr<PlatformCAAnimation> createSpringAnimation(const Animation*, const String&, bool additive);

Can the return type be RefPtr instead of PassRefPtr?

[Sam Weinig]

(In reply to comment #2)
> Comment on attachment 280564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280564&action=review
> 
> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.
> cpp:380:12: error: enumeration value 'SpringFunction' not handled in switch
> [-Werror=switch]
> 
> > Source/WebCore/css/CSSParser.h:218
> > +    bool parseSpringTimingFunctionValue(CSSParserValueList& args, double& result);
> 
> I’d use Optional<double> instead, in this new code. I know the old code does
> not work that way.

Done.

> 
> > Source/WebCore/css/CSSTimingFunctionValue.cpp:46
> > +    StringBuilder builder;
> > +    builder.appendLiteral("cubic-bezier(");
> > +    builder.appendNumber(m_x1);
> > +    builder.appendLiteral(", ");
> > +    builder.appendNumber(m_y1);
> > +    builder.appendLiteral(", ");
> > +    builder.appendNumber(m_x2);
> > +    builder.appendLiteral(", ");
> > +    builder.appendNumber(m_y2);
> > +    builder.append(')');
> > +    
> > +    return builder.toString();
> 
> So makeString can’t do this kind of job with the numeric conversion without
> temporary strings?

I can't remember where we landed on this, but I couldn't find support in makeString for numbers.  It seems like a good area to look into for me.

> 
> I’d leave out the blank line before calling toString.

Done

> 
> > Source/WebCore/page/animation/AnimationBase.cpp:77
> > +    SpringSolver solver(mass, stiffness, damping, initialVelocity);
> > +    return solver.solve(t * duration);
> 
> I would be tempted to write this without the local variable, but maybe the
> syntax would be ugly.

I like it on two lines, and it matches other similar idioms we have elsewhere (e.g. UnitBezier).

> 
> > Source/WebCore/platform/animation/TimingFunction.h:244
> > +        return adoptRef(*new SpringTimingFunction(1, 100, 10, 0));
> 
> Not clear why these are the default values. Might need a comment.

Done.

> 
> Maybe just have this one call the other create function.

Done.

> 
> > Source/WebCore/platform/animation/TimingFunction.h:249
> > +    virtual ~SpringTimingFunction()
> > +    {
> > +    }
> 
> I don’t see any reason to define this explicitly. Sometimes there could be
> value in that if we defined it in a cpp file, but if we are writing this
> inline version, I think just not writing anything is a better way to express
> it.

Done.

> 
> > Source/WebCore/platform/animation/TimingFunction.h:284
> > +    PassRefPtr<TimingFunction> clone() const override
> 
> Maybe just final instead of override.

Since the whole class is marked final, what value is added in marking this one final?

> 
> > Source/WebCore/platform/graphics/SpringSolver.h:34
> > +        m_w0 = sqrt(stiffness / mass);
> 
> Better to use std::sqrt; we normally do that so we get overloading for
> types. But since this is double doesn’t matter in practice.

Done.

> 
> > Source/WebCore/platform/graphics/SpringSolver.h:53
> > +            t = exp(-t * m_zeta * m_w0) * (m_A * cos(m_wd * t) + m_B * sin(m_wd * t));
> 
> std::exp, std::cos, std::sin

Done.

> 
> > Source/WebCore/platform/graphics/SpringSolver.h:56
> > +            t = (m_A + m_B * t) * exp(-t * m_w0);
> 
> std::exp

Done.

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2919
> > +    RefPtr<PlatformCAAnimation> basicAnim = createPlatformCAAnimation(PlatformCAAnimation::Spring, keyPath);
> 
> How about auto?

Done.

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:245
> > +    PassRefPtr<PlatformCAAnimation> createSpringAnimation(const Animation*, const String&, bool additive);
> 
> Can the return type be RefPtr instead of PassRefPtr?

Done.

[Sam Weinig]

Created attachment 280568 [details]
Patch

[Dean Jackson]

Comment on attachment 280568 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Adds experimental support for a new CSS animation timing function that uses
> +        spring to model the time function. To use it you replace your normal timing
> +        function, be it cubic-bezier or steps, with a new function called spring().
> +        For instance, for a transition you would write:
> +
> +            transition-timing-function: spring(1 100 10 0);

In our documentation for this, which I'll start on tomorrow, we need to explain how duration relates to the spring function.

> Source/WebCore/ChangeLog:120
>  2016-06-03  Gavin & Ellie Barraclough  <[e-mail]>

Unrelated. Nice!

> Source/WebCore/css/CSSParser.cpp:5239
> +        // For cubic bezier, 4 values must be specified (comma-separated).

We should propose making the commas optional.

> Source/WebCore/css/CSSParser.cpp:5249
> +        if (!x1)
> +            return nullptr;
> +        if (x1.value() < 0 || x1.value() > 1)
> +            return nullptr;

Do these in one condition?

> Source/WebCore/css/CSSParser.cpp:5258
> +        if (!x2)
>              return nullptr;
> -        if (!parseCubicBezierTimingFunctionValue(*args, y1))
> +        if (x2.value() < 0 || x2.value() > 1)

And here?

> Source/WebCore/css/CSSParser.cpp:5270
> +        // For a spring, 4 values must be specified (space-separated).
> +        if (!args || args->size() != 4)

Add FIXME that says we'll make some/all of the arguments optional.

> Source/WebCore/css/CSSParser.cpp:5285
> +        if (!stiffness)
> +            return nullptr;
> +        if (stiffness.value() <= 0)
> +            return nullptr;

By now I'm guessing you like having explicit tests, so ignore my suggestions above.

> Source/WebCore/platform/animation/TimingFunction.h:254
> +            const SpringTimingFunction& otherString = *static_cast<const SpringTimingFunction*>(&other);
> +
> +            return m_mass == otherString.m_mass && m_stiffness == otherString.m_stiffness && m_damping == otherString.m_damping && m_initialVelocity == otherString.m_initialVelocity;

Remove blank line.

> Source/WebCore/platform/graphics/SpringSolver.h:41
> +            m_B = (m_zeta * m_w0 + -initialVelocity) / m_wd;

Weird that this isn't .... m_w0 - initialVelocity

> Source/WebCore/platform/graphics/SpringSolver.h:45
> +            m_B = -initialVelocity + m_w0;

And this isn't m_w0 - initialVelocity

> LayoutTests/animations/spring-function.html:4
> +<html lang="en">
> +<head>
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">

Not needed.

> LayoutTests/animations/spring-function.html:44
> +</head>
> +<body>

Ditto.

> LayoutTests/animations/spring-function.html:51
> +</body>
> +</html>

Ditto.

> LayoutTests/animations/script-tests/spring-computed-style.js:33
> +testComputedSpring("Basic", "spring(1 100 10 0)", "spring(1 100 10 0)");
> +testComputedSpring("Negative Velocity", "spring(1 100 10 -10)", "spring(1 100 10 -10)");
> +testComputedSpring("Positive Velocity", "spring(1 100 10 10)", "spring(1 100 10 10)");
> +testComputedSpring("Zero Damping", "spring(1 100 0 10)", "spring(1 100 0 10)");
> +testComputedSpring("Minimum Values", "spring(1 1 0 -999999)", "spring(1 1 0 -999999)");

Test floats (makes sure we didn't look only for integers)? Test exponential numbers?

> LayoutTests/animations/script-tests/spring-computed-style.js:46
> +testComputedSpring("No parameters", "spring()", "ease");
> +testComputedSpring("Not enough parameters", "spring(1 100 10)", "ease");
> +testComputedSpring("Too many parameters", "spring(1 100 10 0 0)", "ease");
> +testComputedSpring("Illegal Mass (< 0)", "spring(-1 100 10 0)", "ease");
> +testComputedSpring("Illegal Mass (== 0)", "spring(0 100 10 0)", "ease");
> +testComputedSpring("Illegal Stiffness (< 0)", "spring(1 -1 10 0)", "ease");
> +testComputedSpring("Illegal Stiffness (== 0)", "spring(1 0 10 0)", "ease");
> +testComputedSpring("Illegal Damping (< 0)", "spring(1 100 -1 0)", "ease");

Test junk? spring(a b c d).

[WebKit Commit Bot]

Comment on attachment 280568 [details]
Patch

Clearing flags on attachment: 280568

Committed r201706: <http://trac.webkit.org/changeset/201706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #6)
> Comment on attachment 280568 [details]
> Patch
> 
> Clearing flags on attachment: 280568
> 
> Committed r201706: <http://trac.webkit.org/changeset/201706>

It broke the Apple Mac Yosemite build, see build.webkit.org for details.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158427

[Ryan Haddad]

(In reply to comment #8)
> (In reply to comment #6)
> > Comment on attachment 280568 [details]
> > Patch
> > 
> > Clearing flags on attachment: 280568
> > 
> > Committed r201706: <http://trac.webkit.org/changeset/201706>
> 
> It broke the Apple Mac Yosemite build, see build.webkit.org for details.

https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/15774

[Sam Weinig]

Created attachment 280639 [details]
Patch

[Sam Weinig]

Created attachment 280659 [details]
Patch

[Sam Weinig]

Created attachment 280661 [details]
Patch

[Build Bot]

Comment on attachment 280661 [details]
Patch

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

New failing tests:
animations/spring-function.html
animations/spring-parsing.html
animations/spring-computed-style.html

[Build Bot]

Created attachment 280665 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Sam Weinig]

(In reply to comment #14)
> Comment on attachment 280661 [details]
> Patch
> 
> Attachment 280661 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/1455786
> 
> New failing tests:
> animations/spring-function.html
> animations/spring-parsing.html
> animations/spring-computed-style.html

Failures are due to Xcode not recompiling InternalSettingsGenerated, not a real failure.

[Sam Weinig]

Committed r201759: <http://trac.webkit.org/changeset/201759>

[Ryan Haddad]

(In reply to comment #17)
> Committed r201759: <http://trac.webkit.org/changeset/201759>

The Yosemite build still appears to be failing after the attempted build fix in https://trac.webkit.org/changeset/201764

https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/15825

/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:783:34: error: instance method '-setInitialVelocity:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]

[Alex Christensen]

Yosemite builders don't like this:
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:783:34: error: instance method '-setInitialVelocity:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
                [springAnimation setInitialVelocity:function.initialVelocity()];

[Alex Christensen]

CAAnimation.h has this:

CA_CLASS_AVAILABLE (10.11, 9.0, 9.0, 2.0)
@interface CASpringAnimation : CABasicAnimation
...
@property CGFloat initialVelocity;