Bug 186518 – [Web Animations] Update API to match latest spec

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

Our current Web Animations implementation covers the Web Animations API as it was specified around late 2017.

Here are the subtasks:

* Update the API to implement Animation.updatePlaybackRate() (webkit.org/b/186510)
* Implement getTiming() and updateTiming() (webkit.org/b/186511)
* Update AnimationEffectReadOnly to become AnimationEffect (webkit.org/b/186512)
* Update ComputedTimingProperties to become ComputedEffectTiming (webkit.org/b/186513)
* Update AnimationEffectTimingProperties to become EffectTiming (webkit.org/b/186514)
* Implement the OptionalEffectTiming interface (webkit.org/b/186515)
* Update KeyframeEffectReadOnly to become KeyframeEffect (webkit.org/b/186516)

[Radar WebKit Bug Importer]

<rdar://problem/41000800>

[Antoine Quint]

Another subtask: Implement composite operations (webkit.org/189299).

[Hohan Rebin]

The best website to receive sms online is https://www.receivesms.org

[Antoine Quint]

Created attachment 352151 [details]
EWS bots run, not for review

[Antoine Quint]

Created attachment 352156 [details]
EWS bot run, not for review.

[EWS Watchlist]

Attachment 352156 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffectOptions.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:179:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:550:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:595:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/animation/AnimationEffectTiming.cpp:120:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 6 in 138 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 352156 [details]
EWS bot run, not for review.

Attachment 352156 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9550168

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 352160 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 352156 [details]
EWS bot run, not for review.

Attachment 352156 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9550190

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 352162 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 352163 [details]
EWS bot run, not for review

[EWS Watchlist]

Attachment 352163 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:179:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:550:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:595:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/AnimationEffectTiming.cpp:120:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 141 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 352163 [details]
EWS bot run, not for review

Attachment 352163 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9551318

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 352163 [details]
EWS bot run, not for review

Attachment 352163 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9552095

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

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

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

[Antoine Quint]

Created attachment 352184 [details]
EWS bot run not for review

[EWS Watchlist]

Attachment 352184 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:179:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:550:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:595:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/AnimationEffectTiming.cpp:120:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 144 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 352313 [details]
EWS run, not for review

[EWS Watchlist]

Attachment 352313 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:178:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:576:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:621:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/AnimationEffect.cpp:464:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 142 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 352343 [details]
EWS run, not for review

[Antoine Quint]

Created attachment 352350 [details]
Patch

[EWS Watchlist]

Attachment 352350 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:178:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:576:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:621:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/AnimationEffect.cpp:464:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 143 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 352364 [details]
Patch

[EWS Watchlist]

Attachment 352364 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:178:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:576:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:621:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/AnimationEffect.cpp:464:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 143 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 353689 [details]
Patch

[EWS Watchlist]

Attachment 353689 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:178:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:576:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/KeyframeEffect.cpp:621:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/animation/AnimationEffect.cpp:443:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3670:  Path does not exist.  [test/expectations] [5]
Total errors found: 5 in 139 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Why aren't all these bits in a single patch?

* Update AnimationEffectReadOnly to become AnimationEffect (webkit.org/b/186512)
* Update ComputedTimingProperties to become ComputedEffectTiming (webkit.org/b/186513)
* Update AnimationEffectTimingProperties to become EffectTiming (webkit.org/b/186514)
* Implement the OptionalEffectTiming interface (webkit.org/b/186515)
* Update KeyframeEffectReadOnly to become KeyframeEffect (webkit.org/b/186516)

They are all renames right? So should be able to land as one?

[Dean Jackson]

Then it seems there could be another patch just after moving the timing stuff into AnimationEffect (and updating the test results).

[Dean Jackson]

BTW, for those following along at home (probably nobody), Antoine made a pull request that's easier to read than a 1Mb patch

https://stash.sd.apple.com/users/quinta/repos/webkit/pull-requests/4

(Apple internal unfortunately - he'll copy all the comments in manually! :)

[Dean Jackson]

And then another patch after updating playback rate.

[Dean Jackson]

And another after removing the task reset.

[Dean Jackson]

And another after the CompositeOperationAndAuto progressions.

[Dean Jackson]

Overall the patch looks good, but you really have to split it up :)

[Antoine Quint]

As suggested, I have merged these bugs all into webkit.org/b/186512.

* Update AnimationEffectReadOnly to become AnimationEffect (webkit.org/b/186512)
* Update ComputedTimingProperties to become ComputedEffectTiming (webkit.org/b/186513)
* Update AnimationEffectTimingProperties to become EffectTiming (webkit.org/b/186514)
* Implement the OptionalEffectTiming interface (webkit.org/b/186515)
* Update KeyframeEffectReadOnly to become KeyframeEffect (webkit.org/b/186516)

I've also added two other subtasks to match the latest specs:

* Update the API to allow the "auto" composite value (webkit.org/b/191300)
* Don't reset pending tasks when setting a null effect (webkit.org/b/191301)

[Antoine Quint]

Finally, all the tests are updated as part of webkit.org/b/191302.

[Antoine Quint]

All subtasks are now complete, closing this.

[Antoine Quint]

As Dean said, review was collected using an Apple-internal tool. Here are Dean's relevant comments for posterity:

Dean asked "Is that the best way to get a double value out of an optional?" about this line of code in WebAnimation.h:

    double playbackRate() const { return m_playbackRate + 0; }

That ivar is not an optional, this is to ensure we always get +0 and never -0. It was suggested by Darin on a patch long ago when returning time values for the API, I've applied it here too since a new WPT test made light of this.

Then, Dean asked "Do you need the local variable? Can't you assign timing.duration directly?" about these lines of code in KeyframeEffect::create(ExecState&, Element*, Strong<JSObject>&&, std::optional<Variant<double, KeyframeEffectOptions>>&&):

    if (WTF::holds_alternative<double>(optionsValue)) {
        Variant<double, String> duration = WTF::get<double>(optionsValue);
        timing.duration = duration;
    }

Not having the local variable would yield an error:
animation/KeyframeEffect.cpp:470:29: error: no viable overloaded '=' timing.duration = WTF::get<double>(optionsValue);

Then, Dean asked "Why aren't all these marked as ? since they are optional? Or are dictionaries always like that?" about the OptionalEffectTiming dictionary. Indeed, all members of a dictionary are optional, in fact they can even be not specified at all (undefined in JavaScript).

Then, Dean asked "Why does this need a ToJSObject?" about the BaseComputedKeyframe dictionary. This is needed because KeyframeEffect::getKeyframes(ExecState&) does some JS conversion to and from that dictionary. For instance:

    auto outputKeyframe = convertDictionaryToJS(state, *jsCast<JSDOMGlobalObject*>(state.lexicalGlobalObject()), computedKeyframe);