Title: [229069] trunk
Revision
229069
Author
grao...@webkit.org
Date
2018-02-27 13:05:02 -0800 (Tue, 27 Feb 2018)

Log Message

[Web Animations] Correct implementation of pending tasks and promises
https://bugs.webkit.org/show_bug.cgi?id=183161

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

Update test expectations with progressions (+32 WPT PASS).

* web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/finish-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/finished-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/onfinish-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/pause-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/pending-expected.txt:
* web-platform-tests/web-animations/interfaces/Animation/startTime-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/canceling-an-animation-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/pausing-an-animation-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/play-states-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/reversing-an-animation-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation-expected.txt:
* web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation-expected.txt:

Source/WebCore:

We had an incorrect implementation of the spec due to two misinterpretations.

The first one is about pending tasks (play and pause) which the spec says should
be performed by "scheduling a task". In WebCore, this means using postTask() on a
ScriptExecutionContext, such as Document. One of the big practical changes is that
calling play() on an animation correctly sets its startTime to null (unresolved)
immediately after the call to play() returns before setting it to a resolved value
when the task is performed asynchronously. As a result, the playState is now always
accurate.

The second one is about promises where new promises need to be created in certain
situations called out by the spec. We used to call clear() on them, but this merely
resets the fulfillment or rejection state of the promise, while the spec requires
a different object to be returned for the promise. We now create our promises using
makeUniqueRef<> when new promise objects are expected to be created.

This patch also corrects a few smaller bugs and spec compliant issues, called out
below, related to pending tasks and promises uncovered while looking at relevant
WPT tests.

* animation/DocumentTimeline.h: Expose the Document used to create this timeline such
that it may be used by WebAnimation objects registered for this timeline when scheduling
a task is required via postTask().
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::WebAnimation):
(WebCore::WebAnimation::setBindingsStartTime):
(WebCore::WebAnimation::setCurrentTime):
(WebCore::WebAnimation::cancel): Ensure the finished promise has not already been fulfilled
before rejecting it. While the spec does not specifically call this out, a promise may not
be rejected after being fulfilled, and we would hit an ASSERT if we didn't also check that
it was in the correct pending state before attemping to reject it.
(WebCore::WebAnimation::resetPendingTasks):
(WebCore::WebAnimation::finish):
(WebCore::WebAnimation::updateFinishedState):
(WebCore::WebAnimation::finishNotificationSteps):
(WebCore::WebAnimation::play): We used to only check for a pending pause task before canceling
that task, but the spec says to check for either a pending pause or play task (ie. pending())
and to cancel whichever is scheduled.
(WebCore::WebAnimation::runPendingPlayTask): We were missing an assertion called out by the
spec when running a pending task.
(WebCore::WebAnimation::pause):
(WebCore::WebAnimation::runPendingPauseTask):
(WebCore::WebAnimation::updatePendingTasks): We now use postTask() on the animation's associated
timeline's document to schedule pending tasks for which the criteria to run are met, ie. there
is an associated timeline.
* animation/WebAnimation.h:

Modified Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,3 +1,27 @@
+2018-02-27  Antoine Quint  <grao...@apple.com>
+
+        [Web Animations] Correct implementation of pending tasks and promises
+        https://bugs.webkit.org/show_bug.cgi?id=183161
+
+        Reviewed by Dean Jackson.
+
+        Update test expectations with progressions (+32 WPT PASS).
+
+        * web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/finish-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/finished-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/onfinish-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/pause-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/pending-expected.txt:
+        * web-platform-tests/web-animations/interfaces/Animation/startTime-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/canceling-an-animation-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/pausing-an-animation-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/play-states-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/reversing-an-animation-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation-expected.txt:
+        * web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation-expected.txt:
+
 2018-02-26  Antoine Quint  <grao...@apple.com>
 
         [Web Animations] Update the playState implementation

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,6 +1,6 @@
 
 PASS Element.animate() creates an animation with the correct timeline when called on an element in a document without a browsing context 
 PASS The timeline associated with an animation trigger on an element in a document without a browsing context is inactive 
-FAIL Replacing the timeline of an animation targetting an element in a document without a browsing context leaves it in the pending state assert_true: The animation should be initially pending expected true got false
-FAIL Replacing the timeline of an animation targetting an element in a document without a browsing context and then adopting that element causes it to start updating style assert_equals: Style should be updated expected "0" but got "0.2800000011920929"
+FAIL Replacing the timeline of an animation targetting an element in a document without a browsing context leaves it in the pending state assert_true: The animation should still be pending after replacing the document timeline expected true got false
+PASS Replacing the timeline of an animation targetting an element in a document without a browsing context and then adopting that element causes it to start updating style 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finish-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finish-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finish-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -5,7 +5,7 @@
 PASS Test exceptions when finishing infinite animation 
 PASS Test finishing of animation 
 PASS Test finishing of animation with a current time past the effect end 
-FAIL Test finishing of reversed animation assert_equals: After finishing a reversed animation the currentTime should be set to zero expected 0 but got -0
+PASS Test finishing of reversed animation 
 PASS Test finishing of reversed animation with a current time less than zero 
 PASS Test finish() while paused 
 PASS Test finish() while pause-pending with positive playbackRate 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finished-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finished-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finished-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,6 +1,4 @@
 
-Harness Error (TIMEOUT), message = null
-
 PASS Test pausing then playing does not change the finished promise 
 PASS Test restarting a finished animation 
 PASS Test restarting a reversed finished animation 
@@ -10,16 +8,16 @@
 PASS The finished promise is fulfilled with its Animation 
 PASS finished promise is rejected when an animation is canceled by calling cancel() 
 PASS canceling an already-finished animation replaces the finished promise 
-FAIL Test finished promise changes for animation duration changes assert_not_equals: Finished promise should change after lengthening the duration causes the animation to become active got disallowed value object "[object Promise]"
+FAIL Test finished promise changes for animation duration changes assert_false: shortening of the animation duration should resolve the finished promise expected false got true
 PASS Test finished promise changes when playbackRate == 0 
-TIMEOUT Test finished promise resolves when reaching to the natural boundary. Test timed out
-NOTRUN Test finished promise changes when a prior finished promise resolved and the animation falls out finished state 
+PASS Test finished promise resolves when reaching to the natural boundary. 
+PASS Test finished promise changes when a prior finished promise resolved and the animation falls out finished state 
 PASS Test no new finished promise generated when finished state is checked asynchronously 
 PASS Test new finished promise generated when finished state is checked synchronously 
-NOTRUN Test synchronous finished promise resolved even if finished state is changed soon 
-NOTRUN Test synchronous finished promise resolved even if asynchronous finished promise happens just before synchronous promise 
-NOTRUN Test finished promise is not resolved when the animation falls out finished state immediately 
-NOTRUN Test finished promise is not resolved once the animation falls out finished state even though the current finished promise is generated soon after animation state became finished 
-NOTRUN Finished promise should be resolved after the ready promise is resolved 
-NOTRUN Finished promise should be rejected after the ready promise is rejected 
+PASS Test synchronous finished promise resolved even if finished state is changed soon 
+PASS Test synchronous finished promise resolved even if asynchronous finished promise happens just before synchronous promise 
+PASS Test finished promise is not resolved when the animation falls out finished state immediately 
+PASS Test finished promise is not resolved once the animation falls out finished state even though the current finished promise is generated soon after animation state became finished 
+PASS Finished promise should be resolved after the ready promise is resolved 
+PASS Finished promise should be rejected after the ready promise is rejected 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/onfinish-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/onfinish-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/onfinish-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,5 +1,5 @@
 
-FAIL onfinish event is fired when the currentTime < 0 and the playbackRate < 0 assert_equals: event.currentTime should be zero expected 0 but got -0
+PASS onfinish event is fired when the currentTime < 0 and the playbackRate < 0 
 PASS onfinish event is fired when the currentTime > 0 and the playbackRate > 0 
 PASS onfinish event is fired when animation.finish() is called 
 PASS onfinish event is not fired when paused 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pause-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pause-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pause-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,6 +1,6 @@
 
 PASS pause() a running animation 
-FAIL pause() from idle assert_true: initially pause-pending expected true got false
+PASS pause() from idle 
 PASS pause() from idle with a negative playbackRate 
 PASS pause() from idle with a negative playbackRate and endless effect 
 PASS pause() on a finished animation 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pending-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pending-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pending-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,4 +1,4 @@
 
-FAIL reports true -> false when initially played assert_true: expected true got false
-FAIL reports true -> false when paused assert_true: expected true got false
+PASS reports true -> false when initially played 
+PASS reports true -> false when paused 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/startTime-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/startTime-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/interfaces/Animation/startTime-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,8 +1,8 @@
 
 PASS startTime of a newly created (idle) animation is unresolved 
-FAIL startTime of a play-pending animation is unresolved assert_equals: startTime is unresolved expected (object) null but got (number) 72
+PASS startTime of a play-pending animation is unresolved 
 PASS startTime of a pause-pending animation is unresolved 
-FAIL startTime of a play-pending animation created using Element.animate shortcut is unresolved assert_equals: startTime is unresolved expected (object) null but got (number) 72
+PASS startTime of a play-pending animation created using Element.animate shortcut is unresolved 
 PASS startTime is resolved when running 
 PASS startTime and currentTime are unresolved when animation is cancelled 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/canceling-an-animation-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/canceling-an-animation-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/canceling-an-animation-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,8 +1,11 @@
+CONSOLE MESSAGE: Unhandled Promise Rejection: AbortError: The operation was aborted.
 
-FAIL A play-pending ready promise should be rejected when the animation is canceled assert_equals: ready promise is rejected with AbortError expected "AbortError" but got "Error"
-FAIL A pause-pending ready promise should be rejected when the animation is canceled assert_equals: ready promise is rejected with AbortError expected "AbortError" but got "Error"
+Harness Error (FAIL), message = The operation was aborted.
+
+PASS A play-pending ready promise should be rejected when the animation is canceled 
+PASS A pause-pending ready promise should be rejected when the animation is canceled 
 PASS When an animation is canceled, it should create a resolved Promise 
-FAIL The ready promise should be replaced when the animation is canceled assert_not_equals: got disallowed value object "[object Promise]"
+PASS The ready promise should be replaced when the animation is canceled 
 PASS The finished promise should NOT be rejected if the animation is already idle 
 PASS The cancel event should NOT be fired if the animation is already idle 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/pausing-an-animation-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/pausing-an-animation-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/pausing-an-animation-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,3 +1,3 @@
 
-FAIL A pending ready promise should be resolved and not replaced when the animation is paused assert_equals: expected object "[object Promise]" but got object "[object Promise]"
+PASS A pending ready promise should be resolved and not replaced when the animation is paused 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/play-states-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/play-states-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/play-states-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -9,6 +9,6 @@
 PASS reports 'running' when playback rate > 0 and current time = 0 
 PASS reports 'running' when playback rate = 0 and current time = 0 
 PASS reports 'finished' when playback rate < 0 and current time = 0 
-FAIL reports 'finished' when playback rate > 0 and current time = target effect end and there is a pending play task assert_equals: Sanity check: start time should be unresolved expected (object) null but got (number) 121
-FAIL reports 'running' when playback rate > 0 and current time < target effect end and there is a pending play task assert_equals: Sanity check: start time should be unresolved expected (object) null but got (number) 121
+PASS reports 'finished' when playback rate > 0 and current time = target effect end and there is a pending play task 
+PASS reports 'running' when playback rate > 0 and current time < target effect end and there is a pending play task 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/reversing-an-animation-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/reversing-an-animation-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/reversing-an-animation-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -2,7 +2,7 @@
 PASS Reversing an animation inverts the playback rate 
 PASS Reversing an animation plays a pausing animation 
 PASS Reversing an animation maintains the same current time 
-FAIL Reversing an animation does not cause it to leave the pending state assert_true: The animation is pending before we call reverse expected true got false
+PASS Reversing an animation does not cause it to leave the pending state 
 PASS Reversing an animation does not cause it to resolve the ready promise 
 PASS Reversing an animation when playbackRate > 0 and currentTime > effect end should make it play from the end 
 PASS Reversing an animation when playbackRate > 0 and currentTime < 0 should make it play from the end 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -3,7 +3,7 @@
 PASS Setting an unresolved start time an animation without an active timeline does not clear the current time 
 PASS Setting the start time clears the hold time 
 PASS Setting an unresolved start time sets the hold time 
-FAIL Setting the start time resolves a pending ready promise assert_true: Animation is in play-pending state expected true got false
-FAIL Setting the start time resolves a pending pause task assert_true: Animation is in pause-pending state expected true got false
+FAIL Setting the start time resolves a pending ready promise assert_true: Ready promise callback called after setting startTime expected true got false
+FAIL Setting the start time resolves a pending pause task assert_true: Ready promise callback called after setting startTime expected true got false
 PASS Setting the start time updates the finished state 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,7 +1,7 @@
 
-FAIL If new effect is null and old effect is not null, we reset the pending tasks and ready promise is rejected assert_true: expected true got false
-FAIL If animation has a pending pause task, reschedule that task to run as soon as animation is ready. assert_true: expected true got false
-FAIL If animation has a pending play task, reschedule that task to run as soon as animation is ready to play new effect. assert_true: expected true got false
+PASS If new effect is null and old effect is not null, we reset the pending tasks and ready promise is rejected 
+PASS If animation has a pending pause task, reschedule that task to run as soon as animation is ready. 
+PASS If animation has a pending play task, reschedule that task to run as soon as animation is ready to play new effect. 
 PASS When setting the effect of an animation to the effect of an existing animation, the existing animation's target effect should be set to null. 
 PASS After setting the target effect of animation to the target effect of an existing animation, the target effect's timing is updated to reflect the current time of the new animation. 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation-expected.txt (229068 => 229069)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation-expected.txt	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation-expected.txt	2018-02-27 21:05:02 UTC (rev 229069)
@@ -4,15 +4,15 @@
 PASS After setting timeline on an idle animation without a start time it is still idle 
 PASS After setting timeline on an idle animation with a start time it is running 
 PASS After setting timeline on an idle animation with a sufficiently ancient start time it is finished 
-FAIL After setting timeline on a play-pending animation it begins playing after pending assert_true: Animation is still play-pending after setting timeline expected true got false
-FAIL After setting timeline on a pause-pending animation it becomes paused after pending assert_true: Animation is still pause-pending after setting timeline expected true got false
+PASS After setting timeline on a play-pending animation it begins playing after pending 
+PASS After setting timeline on a pause-pending animation it becomes paused after pending 
 PASS After clearing timeline on paused animation it is still paused 
 PASS After clearing timeline on finished animation it is idle 
 PASS After clearing timeline on running animation it is idle 
 PASS After clearing timeline on idle animation it is still idle 
-FAIL After clearing timeline on play-pending animation it is still pending assert_true: expected true got false
-FAIL After clearing and re-setting timeline on play-pending animation it begins to play assert_true: expected true got false
-FAIL After clearing timeline on a pause-pending animation it is still pending assert_true: expected true got false
-FAIL After clearing and re-setting timeline on a pause-pending animation it completes pausing assert_true: expected true got false
+PASS After clearing timeline on play-pending animation it is still pending 
+PASS After clearing and re-setting timeline on play-pending animation it begins to play 
+PASS After clearing timeline on a pause-pending animation it is still pending 
+PASS After clearing and re-setting timeline on a pause-pending animation it completes pausing 
 PASS After clearing and re-setting timeline on an animation in the middle of an aborted pause, it continues playing using the same start time 
 

Modified: trunk/Source/WebCore/ChangeLog (229068 => 229069)


--- trunk/Source/WebCore/ChangeLog	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/Source/WebCore/ChangeLog	2018-02-27 21:05:02 UTC (rev 229069)
@@ -1,3 +1,57 @@
+2018-02-27  Antoine Quint  <grao...@apple.com>
+
+        [Web Animations] Correct implementation of pending tasks and promises
+        https://bugs.webkit.org/show_bug.cgi?id=183161
+
+        Reviewed by Dean Jackson.
+
+        We had an incorrect implementation of the spec due to two misinterpretations.
+
+        The first one is about pending tasks (play and pause) which the spec says should
+        be performed by "scheduling a task". In WebCore, this means using postTask() on a
+        ScriptExecutionContext, such as Document. One of the big practical changes is that
+        calling play() on an animation correctly sets its startTime to null (unresolved)
+        immediately after the call to play() returns before setting it to a resolved value
+        when the task is performed asynchronously. As a result, the playState is now always
+        accurate.
+
+        The second one is about promises where new promises need to be created in certain
+        situations called out by the spec. We used to call clear() on them, but this merely
+        resets the fulfillment or rejection state of the promise, while the spec requires
+        a different object to be returned for the promise. We now create our promises using
+        makeUniqueRef<> when new promise objects are expected to be created.
+
+        This patch also corrects a few smaller bugs and spec compliant issues, called out
+        below, related to pending tasks and promises uncovered while looking at relevant
+        WPT tests.
+
+        * animation/DocumentTimeline.h: Expose the Document used to create this timeline such
+        that it may be used by WebAnimation objects registered for this timeline when scheduling
+        a task is required via postTask().
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::WebAnimation):
+        (WebCore::WebAnimation::setBindingsStartTime):
+        (WebCore::WebAnimation::setCurrentTime):
+        (WebCore::WebAnimation::cancel): Ensure the finished promise has not already been fulfilled
+        before rejecting it. While the spec does not specifically call this out, a promise may not
+        be rejected after being fulfilled, and we would hit an ASSERT if we didn't also check that
+        it was in the correct pending state before attemping to reject it.
+        (WebCore::WebAnimation::resetPendingTasks):
+        (WebCore::WebAnimation::finish):
+        (WebCore::WebAnimation::updateFinishedState):
+        (WebCore::WebAnimation::finishNotificationSteps):
+        (WebCore::WebAnimation::play): We used to only check for a pending pause task before canceling
+        that task, but the spec says to check for either a pending pause or play task (ie. pending())
+        and to cancel whichever is scheduled.
+        (WebCore::WebAnimation::runPendingPlayTask): We were missing an assertion called out by the
+        spec when running a pending task.
+        (WebCore::WebAnimation::pause):
+        (WebCore::WebAnimation::runPendingPauseTask):
+        (WebCore::WebAnimation::updatePendingTasks): We now use postTask() on the animation's associated
+        timeline's document to schedule pending tasks for which the criteria to run are met, ie. there
+        is an associated timeline.
+        * animation/WebAnimation.h:
+
 2018-02-27  Wenson Hsieh  <wenson_hs...@apple.com>
 
         [Extra zoom mode] Implement additional SPI for adjusting viewport shrink-to-fit behavior

Modified: trunk/Source/WebCore/animation/DocumentTimeline.h (229068 => 229069)


--- trunk/Source/WebCore/animation/DocumentTimeline.h	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/Source/WebCore/animation/DocumentTimeline.h	2018-02-27 21:05:02 UTC (rev 229069)
@@ -49,6 +49,8 @@
     static Ref<DocumentTimeline> create(Document&, PlatformDisplayID);
     ~DocumentTimeline();
 
+    Document* document() const { return m_document.get(); }
+
     std::optional<Seconds> currentTime() override;
     void pause() override;
 

Modified: trunk/Source/WebCore/animation/WebAnimation.cpp (229068 => 229069)


--- trunk/Source/WebCore/animation/WebAnimation.cpp	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/Source/WebCore/animation/WebAnimation.cpp	2018-02-27 21:05:02 UTC (rev 229069)
@@ -60,8 +60,8 @@
 
 WebAnimation::WebAnimation(Document& document)
     : ActiveDOMObject(&document)
-    , m_readyPromise(*this, &WebAnimation::readyPromiseResolve)
-    , m_finishedPromise(*this, &WebAnimation::finishedPromiseResolve)
+    , m_readyPromise(makeUniqueRef<ReadyPromise>(*this, &WebAnimation::readyPromiseResolve))
+    , m_finishedPromise(makeUniqueRef<FinishedPromise>(*this, &WebAnimation::finishedPromiseResolve))
 {
     suspendIfNeeded();
 }
@@ -245,7 +245,7 @@
     if (pending()) {
         setTimeToRunPendingPauseTask(TimeToRunPendingTask::NotScheduled);
         setTimeToRunPendingPlayTask(TimeToRunPendingTask::NotScheduled);
-        m_readyPromise.resolve(*this);
+        m_readyPromise->resolve(*this);
     }
 
     // 7. Run the procedure to update an animation’s finished state for animation with the did seek flag set to true, and the synchronously notify flag set to false.
@@ -368,7 +368,7 @@
         // 3. Cancel the pending pause task.
         setTimeToRunPendingPauseTask(TimeToRunPendingTask::NotScheduled);
         // 4. Resolve animation's current ready promise with animation.
-        m_readyPromise.resolve(*this);
+        m_readyPromise->resolve(*this);
     }
 
     // 3. Run the procedure to update an animation's finished state for animation with the did seek flag set to true, and the synchronously notify flag set to false.
@@ -462,10 +462,11 @@
         resetPendingTasks();
 
         // 2. Reject the current finished promise with a DOMException named "AbortError".
-        m_finishedPromise.reject(Exception { AbortError });
+        if (!m_finishedPromise->isFulfilled())
+            m_finishedPromise->reject(Exception { AbortError });
 
         // 3. Let current finished promise be a new (pending) Promise object.
-        m_finishedPromise.clear();
+        m_finishedPromise = makeUniqueRef<FinishedPromise>(*this, &WebAnimation::finishedPromiseResolve);
 
         // 4. Create an AnimationPlaybackEvent, cancelEvent.
         // 5. Set cancelEvent's type attribute to cancel.
@@ -526,11 +527,11 @@
         setTimeToRunPendingPauseTask(TimeToRunPendingTask::NotScheduled);
 
     // 4. Reject animation's current ready promise with a DOMException named "AbortError".
-    m_readyPromise.reject(Exception { AbortError });
+    m_readyPromise->reject(Exception { AbortError });
 
     // 5. Let animation's current ready promise be the result of creating a new resolved Promise object.
-    m_readyPromise.clear();
-    m_readyPromise.resolve(*this);
+    m_readyPromise = makeUniqueRef<ReadyPromise>(*this, &WebAnimation::readyPromiseResolve);
+    m_readyPromise->resolve(*this);
 }
 
 ExceptionOr<void> WebAnimation::finish()
@@ -564,13 +565,13 @@
         // 2. Cancel the pending pause task.
         setTimeToRunPendingPauseTask(TimeToRunPendingTask::NotScheduled);
         // 3. Resolve the current ready promise of animation with animation.
-        m_readyPromise.resolve(*this);
+        m_readyPromise->resolve(*this);
     }
 
     // 6. If there is a pending play task and start time is resolved, cancel that task and resolve the current ready promise of animation with animation.
     if (hasPendingPlayTask() && m_startTime) {
         setTimeToRunPendingPlayTask(TimeToRunPendingTask::NotScheduled);
-        m_readyPromise.resolve(*this);
+        m_readyPromise->resolve(*this);
     }
 
     // 7. Run the procedure to update an animation's finished state animation with the did seek flag set to true, and the synchronously notify flag set to true.
@@ -634,7 +635,7 @@
     auto currentFinishedState = playState() == PlayState::Finished;
 
     // 5. If current finished state is true and the current finished promise is not yet resolved, perform the following steps:
-    if (currentFinishedState && !m_finishedPromise.isFulfilled()) {
+    if (currentFinishedState && !m_finishedPromise->isFulfilled()) {
         if (synchronouslyNotify == SynchronouslyNotify::Yes) {
             // If synchronously notify is true, cancel any queued microtask to run the finish notification steps for this animation,
             // and run the finish notification steps immediately.
@@ -650,8 +651,8 @@
 
     // 6. If current finished state is false and animation's current finished promise is already resolved, set animation's current
     // finished promise to a new (pending) Promise object.
-    if (!currentFinishedState && m_finishedPromise.isFulfilled())
-        m_finishedPromise.clear();
+    if (!currentFinishedState && m_finishedPromise->isFulfilled())
+        m_finishedPromise = makeUniqueRef<FinishedPromise>(*this, &WebAnimation::finishedPromiseResolve);
 }
 
 void WebAnimation::scheduleMicrotaskIfNeeded()
@@ -684,7 +685,7 @@
         return;
 
     // 2. Resolve animation's current finished promise object with animation.
-    m_finishedPromise.resolve(*this);
+    m_finishedPromise->resolve(*this);
 
     // 3. Create an AnimationPlaybackEvent, finishEvent.
     // 4. Set finishEvent's type attribute to finish.
@@ -741,8 +742,9 @@
     }
 
     // 4. If animation has a pending play task or a pending pause task,
-    if (hasPendingPauseTask()) {
+    if (pending()) {
         // 1. Cancel that task.
+        setTimeToRunPendingPauseTask(TimeToRunPendingTask::NotScheduled);
         setTimeToRunPendingPlayTask(TimeToRunPendingTask::NotScheduled);
         // 2. Set has pending ready promise to true.
         hasPendingReadyPromise = true;
@@ -758,7 +760,7 @@
 
     // 7. If has pending ready promise is false, let animation's current ready promise be a new (pending) Promise object.
     if (!hasPendingReadyPromise)
-        m_readyPromise.clear();
+        m_readyPromise = makeUniqueRef<ReadyPromise>(*this, &WebAnimation::readyPromiseResolve);
 
     // 8. Schedule a task to run as soon as animation is ready.
     setTimeToRunPendingPlayTask(TimeToRunPendingTask::WhenReady);
@@ -782,10 +784,13 @@
 
     m_timeToRunPendingPlayTask = TimeToRunPendingTask::NotScheduled;
 
-    // 1. Let ready time be the time value of the timeline associated with animation at the moment when animation became ready.
+    // 1. Assert that at least one of animation’s start time or hold time is resolved.
+    ASSERT(m_startTime || m_holdTime);
+
+    // 2. Let ready time be the time value of the timeline associated with animation at the moment when animation became ready.
     auto readyTime = m_timeline->currentTime();
 
-    // 2. If animation's start time is unresolved, perform the following steps:
+    // 3. If animation's start time is unresolved, perform the following steps:
     if (!m_startTime) {
         // 1. Let new start time be the result of evaluating ready time - hold time / animation playback rate for animation.
         // If the animation playback rate is zero, let new start time be simply ready time.
@@ -799,10 +804,10 @@
         setStartTime(newStartTime);
     }
 
-    // 3. Resolve animation's current ready promise with animation.
-    m_readyPromise.resolve(*this);
+    // 4. Resolve animation's current ready promise with animation.
+    m_readyPromise->resolve(*this);
 
-    // 4. Run the procedure to update an animation's finished state for animation with the did seek flag set to false, and the synchronously notify flag set to false.
+    // 5. Run the procedure to update an animation's finished state for animation with the did seek flag set to false, and the synchronously notify flag set to false.
     updateFinishedState(DidSeek::No, SynchronouslyNotify::No);
 }
 
@@ -846,7 +851,7 @@
 
     // 6. If has pending ready promise is false, set animation's current ready promise to a new (pending) Promise object.
     if (!hasPendingReadyPromise)
-        m_readyPromise.clear();
+        m_readyPromise = makeUniqueRef<ReadyPromise>(*this, &WebAnimation::readyPromiseResolve);
 
     // 7. Schedule a task to be executed at the first possible moment after the user agent has performed any processing necessary
     //    to suspend the playback of animation's target effect, if any.
@@ -919,7 +924,7 @@
     setStartTime(std::nullopt);
 
     // 4. Resolve animation's current ready promise with animation.
-    m_readyPromise.resolve(*this);
+    m_readyPromise->resolve(*this);
 
     // 5. Run the procedure to update an animation's finished state for animation with the did seek flag set to false, and the
     //    synchronously notify flag set to false.
@@ -928,12 +933,24 @@
 
 void WebAnimation::updatePendingTasks()
 {
-    if (m_timeToRunPendingPauseTask == TimeToRunPendingTask::ASAP && m_timeline)
-        runPendingPauseTask();
+    if (hasPendingPauseTask() && is<DocumentTimeline>(m_timeline)) {
+        if (auto document = downcast<DocumentTimeline>(*m_timeline).document()) {
+            document->postTask([this, protectedThis = makeRef(*this)] (auto&) {
+                if (this->hasPendingPauseTask() && m_timeline)
+                    this->runPendingPauseTask();
+            });
+        }
+    }
 
     // FIXME: This should only happen if we're ready, at the moment we think we're ready if we have a timeline.
-    if (m_timeToRunPendingPlayTask == TimeToRunPendingTask::WhenReady && m_timeline)
-        runPendingPlayTask();
+    if (hasPendingPlayTask() && is<DocumentTimeline>(m_timeline)) {
+        if (auto document = downcast<DocumentTimeline>(*m_timeline).document()) {
+            document->postTask([this, protectedThis = makeRef(*this)] (auto&) {
+                if (this->hasPendingPlayTask() && m_timeline)
+                    this->runPendingPlayTask();
+            });
+        }
+    }
 }
 
 Seconds WebAnimation::timeToNextRequiredTick(Seconds timelineTime) const

Modified: trunk/Source/WebCore/animation/WebAnimation.h (229068 => 229069)


--- trunk/Source/WebCore/animation/WebAnimation.h	2018-02-27 20:33:30 UTC (rev 229068)
+++ trunk/Source/WebCore/animation/WebAnimation.h	2018-02-27 21:05:02 UTC (rev 229069)
@@ -35,6 +35,7 @@
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
 #include <wtf/Seconds.h>
+#include <wtf/UniqueRef.h>
 
 namespace WebCore {
 
@@ -79,10 +80,10 @@
     bool pending() const { return hasPendingPauseTask() || hasPendingPlayTask(); }
 
     using ReadyPromise = DOMPromiseProxyWithResolveCallback<IDLInterface<WebAnimation>>;
-    ReadyPromise& ready() { return m_readyPromise; }
+    ReadyPromise& ready() { return m_readyPromise.get(); }
 
     using FinishedPromise = DOMPromiseProxyWithResolveCallback<IDLInterface<WebAnimation>>;
-    FinishedPromise& finished() { return m_finishedPromise; }
+    FinishedPromise& finished() { return m_finishedPromise.get(); }
 
     void cancel();
     ExceptionOr<void> finish();
@@ -144,8 +145,8 @@
     bool m_isStopped { false };
     bool m_finishNotificationStepsMicrotaskPending;
     bool m_scheduledMicrotask;
-    ReadyPromise m_readyPromise;
-    FinishedPromise m_finishedPromise;
+    UniqueRef<ReadyPromise> m_readyPromise;
+    UniqueRef<FinishedPromise> m_finishedPromise;
     TimeToRunPendingTask m_timeToRunPendingPlayTask { TimeToRunPendingTask::NotScheduled };
     TimeToRunPendingTask m_timeToRunPendingPauseTask { TimeToRunPendingTask::NotScheduled };
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to