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)
<rdar://problem/41000800>
Another subtask: Implement composite operations (webkit.org/189299).
The best website to receive sms online is https://www.receivesms.org
Created attachment 352151 [details] EWS bots run, not for review
Created attachment 352156 [details] EWS bot run, not for review.
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.
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
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
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
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
Created attachment 352163 [details] EWS bot run, not for review
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.
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
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
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
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
Created attachment 352184 [details] EWS bot run not for review
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.
Created attachment 352313 [details] EWS run, not for review
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.
Created attachment 352343 [details] EWS run, not for review
Created attachment 352350 [details] Patch
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.
Created attachment 352364 [details] Patch
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.
Created attachment 353689 [details] Patch
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.
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?
Then it seems there could be another patch just after moving the timing stuff into AnimationEffect (and updating the test results).
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! :)
And then another patch after updating playback rate.
And another after removing the task reset.
And another after the CompositeOperationAndAuto progressions.
Overall the patch looks good, but you really have to split it up :)
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)
Finally, all the tests are updated as part of webkit.org/b/191302.
All subtasks are now complete, closing this.
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);