Bug 186518 - [Web Animations] Update API to match latest spec
Summary: [Web Animations] Update API to match latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-11 05:11 PDT by Antoine Quint
Modified: 2018-11-06 03:41 PST (History)
6 users (show)

See Also:


Attachments
EWS bots run, not for review (1.00 MB, patch)
2018-10-12 02:28 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
EWS bot run, not for review. (1.00 MB, patch)
2018-10-12 04:20 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.39 MB, application/zip)
2018-10-12 05:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.96 MB, application/zip)
2018-10-12 05:48 PDT, Build Bot
no flags Details
EWS bot run, not for review (1.02 MB, patch)
2018-10-12 06:02 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.38 MB, application/zip)
2018-10-12 09:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.49 MB, application/zip)
2018-10-12 11:34 PDT, Build Bot
no flags Details
EWS bot run not for review (1.04 MB, patch)
2018-10-12 11:39 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
EWS run, not for review (1.04 MB, patch)
2018-10-15 07:01 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
EWS run, not for review (1.04 MB, patch)
2018-10-15 10:45 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2018-10-15 11:36 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2018-10-15 12:48 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (1.04 MB, patch)
2018-11-02 05:20 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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);