RESOLVED FIXED 158403
Add experimental support for spring based CSS animations
https://bugs.webkit.org/show_bug.cgi?id=158403
Summary Add experimental support for spring based CSS animations
Sam Weinig
Reported 2016-06-05 15:11:08 PDT
Add experimental support for spring based CSS animations
Attachments
Patch (68.49 KB, patch)
2016-06-05 15:39 PDT, Sam Weinig
no flags
Patch (73.09 KB, patch)
2016-06-05 18:13 PDT, Sam Weinig
no flags
Patch (79.99 KB, patch)
2016-06-06 15:43 PDT, Sam Weinig
no flags
Patch (80.01 KB, patch)
2016-06-06 18:56 PDT, Sam Weinig
no flags
Patch (80.00 KB, patch)
2016-06-06 19:23 PDT, Sam Weinig
dino: review+
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite (924.62 KB, application/zip)
2016-06-06 20:14 PDT, Build Bot
no flags
Sam Weinig
Comment 1 2016-06-05 15:39:59 PDT
Darin Adler
Comment 2 2016-06-05 16:42:59 PDT
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
Comment 3 2016-06-05 17:38:20 PDT
(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
Comment 4 2016-06-05 18:13:54 PDT
Dean Jackson
Comment 5 2016-06-05 21:31:31 PDT
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 <barraclough@apple.com> 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 6 2016-06-05 21:55:14 PDT
Comment on attachment 280568 [details] Patch Clearing flags on attachment: 280568 Committed r201706: <http://trac.webkit.org/changeset/201706>
WebKit Commit Bot
Comment 7 2016-06-05 21:55:17 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 8 2016-06-06 01:20:48 PDT
(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
Comment 9 2016-06-06 10:43:15 PDT
Re-opened since this is blocked by bug 158427
Ryan Haddad
Comment 10 2016-06-06 13:30:22 PDT
(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
Comment 11 2016-06-06 15:43:21 PDT
Sam Weinig
Comment 12 2016-06-06 18:56:57 PDT
Sam Weinig
Comment 13 2016-06-06 19:23:31 PDT
Build Bot
Comment 14 2016-06-06 20:14:18 PDT
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
Comment 15 2016-06-06 20:14:23 PDT
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
Comment 16 2016-06-07 09:26:17 PDT
(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
Comment 17 2016-06-07 11:14:04 PDT
Ryan Haddad
Comment 18 2016-06-07 12:44:50 PDT
(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
Comment 19 2016-06-07 13:13:39 PDT
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
Comment 20 2016-06-07 13:28:09 PDT
CAAnimation.h has this: CA_CLASS_AVAILABLE (10.11, 9.0, 9.0, 2.0) @interface CASpringAnimation : CABasicAnimation ... @property CGFloat initialVelocity;
Note You need to log in before you can comment on or make changes to this bug.