Bug 186518

Summary: [Web Animations] Update API to match latest spec
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews, fred.wang, julibuli87, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186510
https://bugs.webkit.org/show_bug.cgi?id=186511
https://bugs.webkit.org/show_bug.cgi?id=186512
https://bugs.webkit.org/show_bug.cgi?id=186513
https://bugs.webkit.org/show_bug.cgi?id=186514
https://bugs.webkit.org/show_bug.cgi?id=186515
https://bugs.webkit.org/show_bug.cgi?id=186516
https://bugs.webkit.org/show_bug.cgi?id=191300
https://bugs.webkit.org/show_bug.cgi?id=191301
https://bugs.webkit.org/show_bug.cgi?id=191302
Attachments:
Description Flags
EWS bots run, not for review
none
EWS bot run, not for review.
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
EWS bot run, not for review
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
EWS bot run not for review
none
EWS run, not for review
none
EWS run, not for review
none
Patch
none
Patch
none
Patch none

Description Antoine Quint 2018-06-11 05:11:55 PDT
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)
Comment 1 Radar WebKit Bug Importer 2018-06-11 05:12:06 PDT
<rdar://problem/41000800>
Comment 2 Antoine Quint 2018-09-04 23:41:24 PDT
Another subtask: Implement composite operations (webkit.org/189299).
Comment 3 Hohan Rebin 2018-09-09 12:07:23 PDT
The best website to receive sms online is https://www.receivesms.org
Comment 4 Antoine Quint 2018-10-12 02:28:32 PDT
Created attachment 352151 [details]
EWS bots run, not for review
Comment 5 Antoine Quint 2018-10-12 04:20:40 PDT
Created attachment 352156 [details]
EWS bot run, not for review.
Comment 6 Build Bot 2018-10-12 04:25:16 PDT
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 7 Build Bot 2018-10-12 05:37:04 PDT
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
Comment 8 Build Bot 2018-10-12 05:37:05 PDT
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 9 Build Bot 2018-10-12 05:48:03 PDT
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
Comment 10 Build Bot 2018-10-12 05:48:05 PDT
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
Comment 11 Antoine Quint 2018-10-12 06:02:52 PDT
Created attachment 352163 [details]
EWS bot run, not for review
Comment 12 Build Bot 2018-10-12 06:07:48 PDT
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 13 Build Bot 2018-10-12 09:39:55 PDT
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
Comment 14 Build Bot 2018-10-12 09:39:57 PDT
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 15 Build Bot 2018-10-12 11:34:11 PDT
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
Comment 16 Build Bot 2018-10-12 11:34:12 PDT
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
Comment 17 Antoine Quint 2018-10-12 11:39:18 PDT
Created attachment 352184 [details]
EWS bot run not for review
Comment 18 Build Bot 2018-10-12 11:44:43 PDT
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.
Comment 19 Antoine Quint 2018-10-15 07:01:41 PDT
Created attachment 352313 [details]
EWS run, not for review
Comment 20 Build Bot 2018-10-15 07:06:39 PDT
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.
Comment 21 Antoine Quint 2018-10-15 10:45:38 PDT
Created attachment 352343 [details]
EWS run, not for review
Comment 22 Antoine Quint 2018-10-15 11:36:27 PDT
Created attachment 352350 [details]
Patch
Comment 23 Build Bot 2018-10-15 11:42:08 PDT
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.
Comment 24 Antoine Quint 2018-10-15 12:48:12 PDT
Created attachment 352364 [details]
Patch
Comment 25 Build Bot 2018-10-15 12:52:24 PDT
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.
Comment 26 Antoine Quint 2018-11-02 05:20:46 PDT
Created attachment 353689 [details]
Patch
Comment 27 Build Bot 2018-11-02 05:25:00 PDT
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.
Comment 28 Dean Jackson 2018-11-05 14:37:10 PST
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?
Comment 29 Dean Jackson 2018-11-05 14:47:17 PST
Then it seems there could be another patch just after moving the timing stuff into AnimationEffect (and updating the test results).
Comment 30 Dean Jackson 2018-11-05 14:48:27 PST
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! :)
Comment 31 Dean Jackson 2018-11-05 14:51:49 PST
And then another patch after updating playback rate.
Comment 32 Dean Jackson 2018-11-05 14:52:35 PST
And another after removing the task reset.
Comment 33 Dean Jackson 2018-11-05 14:55:29 PST
And another after the CompositeOperationAndAuto progressions.
Comment 34 Dean Jackson 2018-11-05 14:56:30 PST
Overall the patch looks good, but you really have to split it up :)
Comment 35 Antoine Quint 2018-11-06 02:49:37 PST
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)
Comment 36 Antoine Quint 2018-11-06 03:06:36 PST
Finally, all the tests are updated as part of webkit.org/b/191302.
Comment 37 Antoine Quint 2018-11-06 03:34:26 PST
All subtasks are now complete, closing this.
Comment 38 Antoine Quint 2018-11-06 03:41:17 PST
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);