Attachment 343808 Details for Bug 183834 – Patch

[patch] Patch

bug-183834-20180628171604.patch (text/plain), 11.38 KB, created by Antoine Quint on 2018-06-28 08:16:06 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-06-28 08:16:06 PDT

Size: 11.38 KB

patch

obsolete

>Subversion Revision: 233164
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 041728d2f48537e7d98007b39e98d85e309af9b8..78d6537338acc0e569577af73cb0f4ac8206df7a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-06-28  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Make imported/mozilla/css-animations/test_animation-starttime.html pass reliably
>+        https://bugs.webkit.org/show_bug.cgi?id=183834
>+        <rdar://problem/40997932>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We need to run pending tasks in the "update animations" procedure to ensure that the start time has been set
>+        to a different time than the timeline time at the time the animation was asked to play(). This ensure the
>+        timeline current time has progressed and can be queried to a different value in a requestAnimationFrame()
>+        callback.
>+
>+        When invalidating events, we need to make sure we disregard instances when an animation has and is still pending
>+        so that we wait until we change the pending state to work out which events to enqueue.
>+
>+        * animation/DeclarativeAnimation.cpp:
>+        (WebCore::DeclarativeAnimation::invalidateDOMEvents):
>+        * animation/DocumentTimeline.cpp:
>+        (WebCore::DocumentTimeline::updateAnimations):
>+        * animation/WebAnimation.cpp:
>+        (WebCore::WebAnimation::updatePendingTasks):
>+        (WebCore::WebAnimation::timeToNextRequiredTick const):
>+        (WebCore::WebAnimation::runPendingTasks):
>+        * animation/WebAnimation.h:
>+
> 2018-06-25  Antoine Quint  <graouts@apple.com>
> 
>         REGRESSION: hardware-accelerated animation fails on inline element
>diff --git a/Source/WebCore/animation/DeclarativeAnimation.cpp b/Source/WebCore/animation/DeclarativeAnimation.cpp
>index b944a2a0c69bf0e2444b094fd80d50b7ac7fcbc2..340c04ed18953155343d8530516941e0389f243d 100644
>--- a/Source/WebCore/animation/DeclarativeAnimation.cpp
>+++ b/Source/WebCore/animation/DeclarativeAnimation.cpp
>@@ -121,6 +121,9 @@ void DeclarativeAnimation::invalidateDOMEvents(Seconds elapsedTime)
>     auto* animationEffect = effect();
> 
>     auto isPending = pending();
>+    if (isPending && m_wasPending)
>+        return;
>+
>     auto iteration = animationEffect ? animationEffect->currentIteration().value_or(0) : 0;
>     auto currentPhase = animationEffect ? animationEffect->phase() : phaseWithoutEffect();
> 
>diff --git a/Source/WebCore/animation/DocumentTimeline.cpp b/Source/WebCore/animation/DocumentTimeline.cpp
>index 5ab209827fa661488f936043abad56b8b92522aa..3f088374d0103b2d5cf5c01f7ae460fd0da232f4 100644
>--- a/Source/WebCore/animation/DocumentTimeline.cpp
>+++ b/Source/WebCore/animation/DocumentTimeline.cpp
>@@ -232,6 +232,9 @@ void DocumentTimeline::animationResolutionTimerFired()
> 
> void DocumentTimeline::updateAnimations()
> {
>+    for (const auto& animation : animations())
>+        animation->runPendingTasks();
>+
>     if (m_document && hasElementAnimations()) {
>         for (const auto& elementToAnimationsMapItem : elementToAnimationsMap())
>             elementToAnimationsMapItem.key->invalidateStyleAndLayerComposition();
>diff --git a/Source/WebCore/animation/WebAnimation.cpp b/Source/WebCore/animation/WebAnimation.cpp
>index 9753c0183c094f3340524fb4222739838d1ccdcf..1f6383f3c81f1923c64b515bd4808723aa6a47fa 100644
>--- a/Source/WebCore/animation/WebAnimation.cpp
>+++ b/Source/WebCore/animation/WebAnimation.cpp
>@@ -979,25 +979,6 @@ void WebAnimation::runPendingPauseTask()
> 
> void WebAnimation::updatePendingTasks()
> {
>-    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 (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();
>-            });
>-        }
>-    }
>-
>     timingModelDidChange();
> }
> 
>@@ -1005,11 +986,17 @@ Seconds WebAnimation::timeToNextRequiredTick() const
> {
>     // If we don't have a timeline, an effect, a start time or a playback rate other than 0,
>     // there is no value to apply so we don't need to schedule invalidation.
>-    if (!m_timeline || !m_effect || !m_startTime || !m_playbackRate)
>+    if (!m_timeline || !m_effect || !m_playbackRate)
>         return Seconds::infinity();
> 
>-    // If we're in the running state, we need to schedule invalidation as soon as possible.
>-    if (playState() == PlayState::Running)
>+    if (pending())
>+        return 0_s;
>+
>+    if (!m_startTime)
>+        return Seconds::infinity();
>+
>+    // If we're in or expected to be in the running state, we need to schedule invalidation as soon as possible.
>+    if (hasPendingPlayTask() || playState() == PlayState::Running)
>         return 0_s;
> 
>     // If our current time is negative, we need to be scheduled to be resolved at the inverse
>@@ -1024,6 +1011,15 @@ Seconds WebAnimation::timeToNextRequiredTick() const
>     return Seconds::infinity();
> }
> 
>+void WebAnimation::runPendingTasks()
>+{
>+    if (hasPendingPauseTask())
>+        runPendingPauseTask();
>+
>+    if (hasPendingPlayTask())
>+        runPendingPlayTask();
>+}
>+
> void WebAnimation::resolve(RenderStyle& targetStyle)
> {
>     updateFinishedState(DidSeek::No, SynchronouslyNotify::Yes);
>diff --git a/Source/WebCore/animation/WebAnimation.h b/Source/WebCore/animation/WebAnimation.h
>index 052b94af779259c68aa81069ff6d9c54e83e111d..bb6393e49467ef957aa239e949e5c794b0a9b31c 100644
>--- a/Source/WebCore/animation/WebAnimation.h
>+++ b/Source/WebCore/animation/WebAnimation.h
>@@ -107,6 +107,7 @@ public:
> 
>     Seconds timeToNextRequiredTick() const;
>     virtual void resolve(RenderStyle&);
>+    void runPendingTasks();
>     void effectTargetDidChange(Element* previousTarget, Element* newTarget);
>     void acceleratedStateDidChange();
>     void applyPendingAcceleratedActions();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 08b97b207e4ae3b6ce1cf28ed22c528a5a64806b..eafd76ed5fa04771ec07fff03b2c6615c4585a4c 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-28  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Make imported/mozilla/css-animations/test_animation-starttime.html pass reliably
>+        https://bugs.webkit.org/show_bug.cgi?id=183834
>+        <rdar://problem/40997932>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This test now passes reliably so we remove any specific expectation. Another test needed to be modified to account for 
>+        the pending state being updated at a different time, so we just wait a frame to ensure the animation is started.
>+
>+        * TestExpectations:
>+        * compositing/visible-rect/animated.html:
>+
> 2018-06-25  Antoine Quint  <graouts@apple.com>
> 
>         REGRESSION: hardware-accelerated animation fails on inline element
>diff --git a/LayoutTests/imported/mozilla/ChangeLog b/LayoutTests/imported/mozilla/ChangeLog
>index fbdd030c5611bf44fa31ba0e394023d834ae567e..ffdf78311f881991a219adbb47d8bfdd9f8db17e 100644
>--- a/LayoutTests/imported/mozilla/ChangeLog
>+++ b/LayoutTests/imported/mozilla/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-06-28  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Make imported/mozilla/css-animations/test_animation-starttime.html pass reliably
>+        https://bugs.webkit.org/show_bug.cgi?id=183834
>+        <rdar://problem/40997932>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Mark progressions in the Mozilla CSS Animations tests.
>+
>+        * css-animations/test_animation-starttime-expected.txt:
>+
> 2018-06-25  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] Make imported/mozilla/css-animations/test_animation-pausing.html pass reliably
>diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
>index 4c2cc1873b64053353cf9db6d6514466d2223f46..7555a6a06284f4abec9daa0669defc4156138a5b 100644
>--- a/LayoutTests/TestExpectations
>+++ b/LayoutTests/TestExpectations
>@@ -1930,7 +1930,6 @@ webkit.org/b/181122 imported/w3c/web-platform-tests/web-animations/interfaces/An
> webkit.org/b/181123 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/finish.html [ Pass Failure ]
> webkit.org/b/181888 imported/w3c/web-platform-tests/web-animations/timing-model/animation-effects/current-iteration.html [ Pass Failure ]
> 
>-webkit.org/b/183834 imported/mozilla/css-animations/test_animation-starttime.html [ Pass Failure Timeout ]
> webkit.org/b/183836 imported/mozilla/css-animations/test_animations-dynamic-changes.html [ Pass Failure Timeout ]
> webkit.org/b/183837 imported/mozilla/css-transitions/test_document-get-animations.html [ Pass Failure Timeout ]
> webkit.org/b/183840 imported/mozilla/css-animations/test_document-get-animations.html [ Pass Failure Timeout ]
>diff --git a/LayoutTests/compositing/visible-rect/animated.html b/LayoutTests/compositing/visible-rect/animated.html
>index 71fa52fc826af4dd8a62b42d97e89ef81544ab4e..12a77bb16e762d4ea3da4afb8beac3b2b8dc8109 100644
>--- a/LayoutTests/compositing/visible-rect/animated.html
>+++ b/LayoutTests/compositing/visible-rect/animated.html
>@@ -34,11 +34,13 @@
>         function doTest()
>         {
>             document.getElementById('animated').addEventListener('webkitAnimationStart', function() {
>-                if (window.internals)
>-                    document.getElementById('layers').innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_VISIBLE_RECTS)
>+                requestAnimationFrame(() => {
>+                    if (window.internals)
>+                        document.getElementById('layers').innerText = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_VISIBLE_RECTS)
> 
>-                if (window.testRunner)
>-                    testRunner.notifyDone();
>+                    if (window.testRunner)
>+                        testRunner.notifyDone();
>+                });
>             }, false);
>         }
>         window.addEventListener('load', doTest, false);
>diff --git a/LayoutTests/imported/mozilla/css-animations/test_animation-starttime-expected.txt b/LayoutTests/imported/mozilla/css-animations/test_animation-starttime-expected.txt
>index ea77245983edd96e2631269da362c47b9e78704e..b98c9d13f12db335d5707f9f291de5a2e17507b5 100644
>--- a/LayoutTests/imported/mozilla/css-animations/test_animation-starttime-expected.txt
>+++ b/LayoutTests/imported/mozilla/css-animations/test_animation-starttime-expected.txt
>@@ -6,7 +6,7 @@ PASS startTime is unresolved when paused
> PASS startTime while pause-pending and play-pending 
> PASS startTime while play-pending from finished state 
> PASS startTime while play-pending from finished state using finish() 
>-FAIL Pausing should make the startTime become null assert_true: After the animation has started, startTime is greater than the time when it was started expected true got false
>+PASS Pausing should make the startTime become null 
> PASS Sanity test to check round-tripping assigning to a new animation's startTime 
> PASS Skipping forward through animation 
> PASS Skipping backwards through animation

Flags:

dino: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 183834: 343808 | 343814