Attachment 350193 Details for Bug 189784 – Patch

[patch] Patch

bug-189784-20180920154040.patch (text/plain), 12.57 KB, created by Antoine Quint on 2018-09-20 06:40:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-09-20 06:40:41 PDT

Size: 12.57 KB

patch

obsolete

>Subversion Revision: 236247
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c8c3b8bab552e8c3c62742b3e3bdc58472b823ab..559fbd5d7f98d32ed14307018e9864a1a94b5b3f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,41 @@
>+2018-09-20  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] DocumentTimeline::updateAnimations() is called endlessly
>+        https://bugs.webkit.org/show_bug.cgi?id=189784
>+        <rdar://problem/41705679>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: webanimations/accelerated-animation-interruption-display-none.html
>+
>+        We have code that keeps queueing pending accelerated actions for an animation that does not have a renderer until it has one
>+        so that we can deal with situations where animations are ready to commited before its composited renderer is available. This
>+        code ended up running continuously when an element with an accelerated animation had its renderer removed without the animation
>+        being removed itself, such as setting "display: none" on an element with an acceelerated CSS Animation targeting it.
>+
>+        We fix this by queueing up a "Stop" accelerated action when updating the accelerated state if there is no renderer for the current
>+        animation target. Then, we no longer re-queue pending accelerated actions if the last queued operation is "Stop". This ensures that
>+        we no longer queue actions endlessly when there is no longer a visible animation.
>+
>+        To test this, we add a new internals.numberOfAnimationTimelineInvalidations() method that indicates the number of times the current
>+        document's animation timeline was invalidated.
>+
>+        * animation/DocumentTimeline.cpp:
>+        (WebCore::DocumentTimeline::updateAnimations):
>+        (WebCore::DocumentTimeline::numberOfAnimationTimelineInvalidationsForTesting const):
>+        * animation/DocumentTimeline.h:
>+        * animation/KeyframeEffectReadOnly.cpp:
>+        (WebCore::KeyframeEffectReadOnly::updateAcceleratedAnimationState): If the animation target does not have a renderer and it's still
>+        marked as running, enqueue a "Stop" accelerated action.
>+        (WebCore::KeyframeEffectReadOnly::addPendingAcceleratedAction): If we enqueue a "Stop" accelerated action, remove any other queued
>+        action so that we only process the "Stop" action, which would have superseded all previously queued actions anyway.
>+        (WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions): Only re-queue pending accelerated actions when a composited renderer
>+        is not yet available if we don't have a "Stop" action queued.
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::numberOfAnimationTimelineInvalidations const):
>+        * testing/Internals.h:
>+        * testing/Internals.idl:
>+
> 2018-09-20  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] Provide a way to query accelerated animations for internal testing
>diff --git a/Source/WebCore/animation/DocumentTimeline.cpp b/Source/WebCore/animation/DocumentTimeline.cpp
>index 772b491038916f3ab44f1cde2449c46d532b9aaa..3c139480ec43f3a71c4d718819d51878b88d6a17 100644
>--- a/Source/WebCore/animation/DocumentTimeline.cpp
>+++ b/Source/WebCore/animation/DocumentTimeline.cpp
>@@ -291,6 +291,8 @@ void DocumentTimeline::animationResolutionTimerFired()
> 
> void DocumentTimeline::updateAnimations()
> {
>+    m_numberOfAnimationTimelineInvalidationsForTesting++;
>+
>     for (const auto& animation : animations())
>         animation->runPendingTasks();
> 
>@@ -499,4 +501,9 @@ Vector<std::pair<String, double>> DocumentTimeline::acceleratedAnimationsForElem
>     return { };
> }
> 
>+unsigned DocumentTimeline::numberOfAnimationTimelineInvalidationsForTesting() const
>+{
>+    return m_numberOfAnimationTimelineInvalidationsForTesting;
>+}
>+
> } // namespace WebCore
>diff --git a/Source/WebCore/animation/DocumentTimeline.h b/Source/WebCore/animation/DocumentTimeline.h
>index 9e336f2d6dc572920a964a66db587c3725233638..e555b15a6ccfa19eaedc9aec3f941e892c075d51 100644
>--- a/Source/WebCore/animation/DocumentTimeline.h
>+++ b/Source/WebCore/animation/DocumentTimeline.h
>@@ -76,6 +76,7 @@ public:
>     WEBCORE_EXPORT bool animationsAreSuspended();
>     WEBCORE_EXPORT unsigned numberOfActiveAnimationsForTesting() const;
>     WEBCORE_EXPORT Vector<std::pair<String, double>> acceleratedAnimationsForElement(Element&) const;    
>+    WEBCORE_EXPORT unsigned numberOfAnimationTimelineInvalidationsForTesting() const;
> 
> private:
>     DocumentTimeline(Document&, Seconds);
>@@ -101,6 +102,7 @@ private:
>     Timer m_animationScheduleTimer;
>     HashSet<RefPtr<WebAnimation>> m_acceleratedAnimationsPendingRunningStateChange;
>     Vector<Ref<AnimationPlaybackEvent>> m_pendingAnimationEvents;
>+    unsigned m_numberOfAnimationTimelineInvalidationsForTesting { 0 };
> 
> #if !USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
>     void animationResolutionTimerFired();
>diff --git a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
>index 0fe00d87439a83aa19b2c8edb554265e89583e6b..fa387eff935a81e84cd59071503d86f356a354a4 100644
>--- a/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
>+++ b/Source/WebCore/animation/KeyframeEffectReadOnly.cpp
>@@ -1199,8 +1199,11 @@ void KeyframeEffectReadOnly::updateAcceleratedAnimationState()
>     if (!m_shouldRunAccelerated)
>         return;
> 
>-    if (!renderer())
>+    if (!renderer()) {
>+        if (isRunningAccelerated())
>+            addPendingAcceleratedAction(AcceleratedAction::Stop);
>         return;
>+    }
> 
>     auto localTime = animation()->currentTime();
> 
>@@ -1225,6 +1228,10 @@ void KeyframeEffectReadOnly::updateAcceleratedAnimationState()
>     if (playState == WebAnimation::PlayState::Finished) {
>         if (isRunningAccelerated())
>             addPendingAcceleratedAction(AcceleratedAction::Stop);
>+        else {
>+            m_lastRecordedAcceleratedAction = AcceleratedAction::Stop;
>+            m_pendingAcceleratedActions.clear();
>+        }
>         return;
>     }
> 
>@@ -1237,6 +1244,8 @@ void KeyframeEffectReadOnly::updateAcceleratedAnimationState()
> 
> void KeyframeEffectReadOnly::addPendingAcceleratedAction(AcceleratedAction action)
> {
>+    if (action == AcceleratedAction::Stop)
>+        m_pendingAcceleratedActions.clear();
>     m_pendingAcceleratedActions.append(action);
>     if (action != AcceleratedAction::Seek)
>         m_lastRecordedAcceleratedAction = action;
>@@ -1269,7 +1278,8 @@ void KeyframeEffectReadOnly::applyPendingAcceleratedActions()
> 
>     auto* renderer = this->renderer();
>     if (!renderer || !renderer->isComposited()) {
>-        animation()->acceleratedStateDidChange();
>+        if (m_lastRecordedAcceleratedAction != AcceleratedAction::Stop || m_pendingAcceleratedActions.last() != AcceleratedAction::Stop)
>+            animation()->acceleratedStateDidChange();
>         return;
>     }
> 
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index 56b5e258479adfdb740202a72549b1e379b4c159..8685487e27b3ea75270d1f23ce7f62294da62d50 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -1076,6 +1076,13 @@ Vector<Internals::AcceleratedAnimation> Internals::acceleratedAnimationsForEleme
>     return animations;
> }
> 
>+unsigned Internals::numberOfAnimationTimelineInvalidations() const
>+{
>+    if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled())
>+        return frame()->document()->timeline().numberOfAnimationTimelineInvalidationsForTesting();
>+    return 0;
>+}
>+
> ExceptionOr<RefPtr<Element>> Internals::pseudoElement(Element& element, const String& pseudoId)
> {
>     if (pseudoId != "before" && pseudoId != "after")
>diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
>index 4407d1e6f258cb8bdd7adb981520bd51e6e8c870..8dd31678b47899ceb4306c76b962048fdd6564fc 100644
>--- a/Source/WebCore/testing/Internals.h
>+++ b/Source/WebCore/testing/Internals.h
>@@ -206,6 +206,7 @@ public:
>         double speed;
>     };
>     Vector<AcceleratedAnimation> acceleratedAnimationsForElement(Element&);
>+    unsigned numberOfAnimationTimelineInvalidations() const;
> 
>     // For animations testing, we need a way to get at pseudo elements.
>     ExceptionOr<RefPtr<Element>> pseudoElement(Element&, const String&);
>diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
>index 55fabf415f959b933607e3e795b62d0c7b5071e4..94457182f52befb778ad6909cc0c76f9637f0f6b 100644
>--- a/Source/WebCore/testing/Internals.idl
>+++ b/Source/WebCore/testing/Internals.idl
>@@ -211,6 +211,7 @@ enum CompositingPolicy {
> 
>     // Web Animations testing.
>     sequence<AcceleratedAnimation> acceleratedAnimationsForElement(Element element);
>+    unsigned long numberOfAnimationTimelineInvalidations();
> 
>     // For animations testing, we need a way to get at pseudo elements.
>     [MayThrowException] Element? pseudoElement(Element element, DOMString pseudoId);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 460d3adf96b9faa4ce0d9a56d489a4b32d3d5245..0e5b27df6ce027fa5ee07a0dfd0ed20cc897b7c0 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-09-20  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] DocumentTimeline::updateAnimations() is called endlessly
>+        https://bugs.webkit.org/show_bug.cgi?id=189784
>+        <rdar://problem/41705679>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a new test that checks that setting "display: none" on an element with an accelerated CSS animation on it
>+        will no longer update the animation timeline.
>+
>+        * webanimations/accelerated-animation-interruption-display-none-expected.txt: Added.
>+        * webanimations/accelerated-animation-interruption-display-none.html: Added.
>+
> 2018-09-19  Yacine Bandou  <yacine.bandou@softathome.com>
> 
>         [EME][WPE] Add WebM initData in the expected result of clearkey-generate-request-disallowed-input test
>diff --git a/LayoutTests/webanimations/accelerated-animation-interruption-display-none-expected.txt b/LayoutTests/webanimations/accelerated-animation-interruption-display-none-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..270ab7cd846d3f776fd62655adb1ea5f34e7bc79
>--- /dev/null
>+++ b/LayoutTests/webanimations/accelerated-animation-interruption-display-none-expected.txt
>@@ -0,0 +1,3 @@
>+
>+PASS Interrupting an animation by setting 'display: none' should stop invalidating the animation timeline. 
>+
>diff --git a/LayoutTests/webanimations/accelerated-animation-interruption-display-none.html b/LayoutTests/webanimations/accelerated-animation-interruption-display-none.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..9c7a13b45b6453a845d58e205c9b4e3ec854654c
>--- /dev/null
>+++ b/LayoutTests/webanimations/accelerated-animation-interruption-display-none.html
>@@ -0,0 +1,57 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style>
>+
>+#target {
>+    width: 100px;
>+    height: 100px;
>+    background-color: black;
>+    animation: foo linear 10s;
>+}
>+
>+@keyframes foo {
>+    from { transform: "translateX(100px)" };
>+    to { transform: "none" };
>+}
>+
>+</style>
>+</head>
>+<body>
>+<script src="../resources/testharness.js"></script>
>+<script src="../resources/testharnessreport.js"></script>
>+<div id="target"></div>
>+
>+<script>
>+
>+function waitNFrames(numberOfFrames, continuation)
>+{
>+    let elapsedFrames = 0;
>+    (function rAFCallback() {
>+        if (elapsedFrames++ >= numberOfFrames)
>+            continuation();
>+        else
>+            requestAnimationFrame(rAFCallback);
>+    })();
>+}
>+
>+async_test(t => {
>+    const initialNumberOfTimelineInvalidations = internals.numberOfAnimationTimelineInvalidations();
>+    waitNFrames(3, () => {
>+        assert_greater_than(internals.numberOfAnimationTimelineInvalidations(), initialNumberOfTimelineInvalidations, "There should be updates made to the timeline as an animation is set up.");
>+
>+        document.getElementById("target").style.display = "none";
>+
>+        waitNFrames(2, () => {
>+            const numberOfTimelineInvalidationsAfterInterruption = internals.numberOfAnimationTimelineInvalidations();
>+            requestAnimationFrame(() => {
>+                assert_equals(internals.numberOfAnimationTimelineInvalidations(), numberOfTimelineInvalidationsAfterInterruption, "There should not be any updates made to the timeline after interrupting the single running animation.");
>+                t.done();
>+            });
>+        });
>+    });
>+}, "Interrupting an animation by setting 'display: none' should stop invalidating the animation timeline.");
>+
>+</script>
>+</body>
>+</html>

Flags:

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

Actions: View | Formatted Diff | Diff

Attachments on bug 189784: 350193 | 350229