Attachment 340306 Details for Bug 185299 – Patch

[patch] Patch

bug-185299-20180514134120.patch (text/plain), 10.38 KB, created by Antoine Quint on 2018-05-14 04:41:21 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-05-14 04:41:21 PDT

Size: 10.38 KB

patch

obsolete

>Subversion Revision: 231741
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index cf540aec2a73503bea3bb0d3d9ba349723b88337..770a80686aaac5b96741d517340f0c5be4e25a8b 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2018-05-14  Antoine Quint  <graouts@apple.com>
>+
>+        REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
>+        https://bugs.webkit.org/show_bug.cgi?id=185299
>+        <rdar://problem/39630230>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In r230574, the fix for webkit.org/b/184518, we changed the processing order in GraphicsLayerCA::updateAnimations() to first
>+        process m_uncomittedAnimations and then m_animationsToProcess, so we are guaranteed animations exist before we attempt to pause
>+        or seek them. This broke interrupting and resuming hardware animations (such as an interrupted CSS Transition or an animation
>+        running in a non-visible tab) since a pause operation recorded _before_ an animation was added would be paused anyway since
>+        the animation was now first added, and then paused. The fix is simply to clear any pending AnimationProcessingAction for a
>+        newly-uncommitted animation.
>+
>+        Test: transitions/interrupted-transition-hardware.html
>+
>+        * platform/graphics/ca/GraphicsLayerCA.cpp:
>+        (WebCore::GraphicsLayerCA::createAnimationFromKeyframes):
>+        (WebCore::GraphicsLayerCA::appendToUncommittedAnimations):
>+        (WebCore::GraphicsLayerCA::createTransformAnimationsFromKeyframes):
>+        * platform/graphics/ca/GraphicsLayerCA.h:
>+        (WebCore::GraphicsLayerCA::LayerPropertyAnimation::LayerPropertyAnimation):
>+
> 2018-05-13  Dirk Schulze  <krit@webkit.org>
> 
>         Implement SVGGeometryElement's isPointInFill and isPointInStroke
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>index 2b35a97ddca24e6e5c66bad33a836433b0c9bc6e..0a02a35feb4af22bef0bf52738e315d88bda82e2 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>@@ -3015,7 +3015,7 @@ bool GraphicsLayerCA::createAnimationFromKeyframes(const KeyframeValueList& valu
>     if (!valuesOK)
>         return false;
> 
>-    m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
>+    appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
> 
>     return true;
> }
>@@ -3042,7 +3042,7 @@ bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& val
>     if (!validMatrices)
>         return false;
> 
>-    m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
>+    appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, 0, timeOffset));
>     return true;
> }
> 
>@@ -3104,12 +3104,21 @@ bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& val
>         
>         ASSERT(valuesOK);
> 
>-        m_uncomittedAnimations.append(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, internalFilterPropertyIndex, timeOffset));
>+        appendToUncommittedAnimations(LayerPropertyAnimation(caAnimation.releaseNonNull(), animationName, valueList.property(), animationIndex, internalFilterPropertyIndex, timeOffset));
>     }
> 
>     return true;
> }
> 
>+void GraphicsLayerCA::appendToUncommittedAnimations(LayerPropertyAnimation&& animation)
>+{
>+    // Since we're adding a new animation, make sure we clear any pending AnimationProcessingAction for this animation
>+    // as these are applied after we've committed new animations.
>+    m_animationsToProcess.remove(animation.m_name);
>+
>+    m_uncomittedAnimations.append(WTFMove(animation));
>+}
>+
> bool GraphicsLayerCA::createFilterAnimationsFromKeyframes(const KeyframeValueList& valueList, const Animation* animation, const String& animationName, Seconds timeOffset)
> {
> #if ENABLE(FILTERS_LEVEL_2)
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>index 5011ccd71840bd0a6f32530d2c3aad31b9a8675c..ee41cd43d468ad62bc4130ca9b74b62027f29de9 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>@@ -459,8 +459,29 @@ private:
>         moveOrCopyAnimations(Copy, fromLayer, toLayer);
>     }
> 
>+    // This represents the animation of a single property. There may be multiple transform animations for
>+    // a single transition or keyframe animation, so index is used to distinguish these.
>+    struct LayerPropertyAnimation {
>+        LayerPropertyAnimation(Ref<PlatformCAAnimation>&& caAnimation, const String& animationName, AnimatedPropertyID property, int index, int subIndex, Seconds timeOffset)
>+            : m_animation(WTFMove(caAnimation))
>+            , m_name(animationName)
>+            , m_property(property)
>+            , m_index(index)
>+            , m_subIndex(subIndex)
>+            , m_timeOffset(timeOffset)
>+        { }
>+
>+        RefPtr<PlatformCAAnimation> m_animation;
>+        String m_name;
>+        AnimatedPropertyID m_property;
>+        int m_index;
>+        int m_subIndex;
>+        Seconds m_timeOffset;
>+    };
>+
>     bool appendToUncommittedAnimations(const KeyframeValueList&, const TransformOperations*, const Animation*, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool isMatrixAnimation);
>     bool appendToUncommittedAnimations(const KeyframeValueList&, const FilterOperation*, const Animation*, const String& animationName, int animationIndex, Seconds timeOffset);
>+    void appendToUncommittedAnimations(LayerPropertyAnimation&&);
> 
>     enum LayerChange : uint64_t {
>         NoChange                                = 0,
>@@ -572,26 +593,6 @@ private:
>     RetainPtr<CGImageRef> m_uncorrectedContentsImage;
>     RetainPtr<CGImageRef> m_pendingContentsImage;
>     
>-    // This represents the animation of a single property. There may be multiple transform animations for
>-    // a single transition or keyframe animation, so index is used to distinguish these.
>-    struct LayerPropertyAnimation {
>-        LayerPropertyAnimation(Ref<PlatformCAAnimation>&& caAnimation, const String& animationName, AnimatedPropertyID property, int index, int subIndex, Seconds timeOffset)
>-            : m_animation(WTFMove(caAnimation))
>-            , m_name(animationName)
>-            , m_property(property)
>-            , m_index(index)
>-            , m_subIndex(subIndex)
>-            , m_timeOffset(timeOffset)
>-        { }
>-
>-        RefPtr<PlatformCAAnimation> m_animation;
>-        String m_name;
>-        AnimatedPropertyID m_property;
>-        int m_index;
>-        int m_subIndex;
>-        Seconds m_timeOffset;
>-    };
>-    
>     // Uncommitted transitions and animations.
>     Vector<LayerPropertyAnimation> m_uncomittedAnimations;
>     
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 2ce1d0a52ca71d966faed23858b0e5627ecb2fe6..58a069a49fffe737e1675e4065dec8ba3604fff5 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-05-14  Antoine Quint  <graouts@apple.com>
>+
>+        REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
>+        https://bugs.webkit.org/show_bug.cgi?id=185299
>+        <rdar://problem/39630230>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a new test where we interrupt a transition and check that upon returning to the original value,
>+        an animated value is still used and not the initial value. This test fails prior to this patch.
>+
>+        * transitions/interrupted-transition-hardware-expected.html: Added.
>+        * transitions/interrupted-transition-hardware.html: Added.
>+
> 2018-05-13  Andy VanWagoner  <andy@vanwagoner.family>
> 
>         [INTL] Improve spec & test262 compliance for Intl APIs
>diff --git a/LayoutTests/transitions/interrupted-transition-hardware-expected.html b/LayoutTests/transitions/interrupted-transition-hardware-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..ff73df0b5db2e6810464b908d8bc313d5a8fd6dc
>--- /dev/null
>+++ b/LayoutTests/transitions/interrupted-transition-hardware-expected.html
>@@ -0,0 +1,22 @@
>+<!DOCTYPE html>
>+
>+<html>
>+<head>
>+    <style>
>+
>+        div {
>+            position: absolute;
>+            left: 0;
>+            top: 0;
>+            height: 100px;
>+            width: 100px;
>+            transform: translateX(25px);
>+            background-color: green;
>+        }
>+
>+    </style>
>+</head>
>+<body>
>+    <div id="cover"></div>
>+</body>
>+</html>
>diff --git a/LayoutTests/transitions/interrupted-transition-hardware.html b/LayoutTests/transitions/interrupted-transition-hardware.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..dc88027a688abeafd8be65cd4b02fb841218d0d9
>--- /dev/null
>+++ b/LayoutTests/transitions/interrupted-transition-hardware.html
>@@ -0,0 +1,59 @@
>+<!DOCTYPE html>
>+
>+<html>
>+<head>
>+    <style>
>+
>+        div {
>+            position: absolute;
>+            left: 0;
>+            top: 0;
>+            height: 100px;
>+            width: 100px;
>+        }
>+        
>+        #test {
>+            background-color: red;
>+            transition: transform 4000s linear;
>+            transition-delay: -2000s;
>+            transform: none;
>+        }
>+
>+        #test.transitions {
>+            transform: translateX(100px);
>+        }
>+
>+        #cover {
>+            transform: translateX(25px);
>+            background-color: green;
>+        }
>+
>+    </style>
>+</head>
>+<body>
>+
>+    <div id="test"></div>
>+    <div id="cover"></div>
>+
>+    <script type="text/javascript">
>+
>+        if (window.testRunner)
>+            testRunner.waitUntilDone();
>+
>+        const test = document.getElementById("test");
>+
>+        requestAnimationFrame(() => {
>+            test.classList.add("transitions");
>+            requestAnimationFrame(() => {
>+                test.classList.remove("transitions");
>+                requestAnimationFrame(() => {
>+                    if (window.testRunner)
>+                        testRunner.notifyDone();
>+                });
>+            });
>+        });
>+        
>+    </script>
>+
>+</body>
>+</html>

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185299: 339541 | 339582 | 340306