Attachment 340319 Details for Bug 185612 – Patch

[patch] Patch

bug-185612-20180514173442.patch (text/plain), 19.93 KB, created by Antoine Quint on 2018-05-14 08:34:43 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antoine Quint

Created: 2018-05-14 08:34:43 PDT

Size: 19.93 KB

patch

obsolete

>Subversion Revision: 231741
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index cf540aec2a73503bea3bb0d3d9ba349723b88337..e063b3730318098217c722a65fef10fcfcb4c7f0 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-05-14  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
>+        https://bugs.webkit.org/show_bug.cgi?id=185612
>+        <rdar://problem/39579344>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a new internals.pseudoElement() method to obtain a pseudo element matching a given pseudo-id. This is necessary to be able to move off
>+        internals.pauseTransitionAtTimeOnPseudoElement() and internals.pauseAnimationAtTimeOnPseudoElement() for Web Animations testing.
>+
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::pseudoElement):
>+        * testing/Internals.h:
>+        * testing/Internals.idl:
>+
> 2018-05-13  Dirk Schulze  <krit@webkit.org>
> 
>         Implement SVGGeometryElement's isPointInFill and isPointInStroke
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index 81a4009533f9cdf19b263f862513ba82b9335b40..09aa0a975cc4dac4ae100f88a0502d2286be5826 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -1048,6 +1048,14 @@ ExceptionOr<bool> Internals::pauseTransitionAtTimeOnPseudoElement(const String&
>     return frame()->animation().pauseTransitionAtTime(*pseudoElement, property, pauseTime);
> }
> 
>+ExceptionOr<RefPtr<Element>> Internals::pseudoElement(Element& element, const String& pseudoId)
>+{
>+    if (pseudoId != "before" && pseudoId != "after")
>+        return Exception { InvalidAccessError };
>+
>+    return pseudoId == "before" ? element.beforePseudoElement() : element.afterPseudoElement();
>+}
>+
> ExceptionOr<String> Internals::elementRenderTreeAsText(Element& element)
> {
>     element.document().updateStyleIfNeeded();
>diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
>index 1b83bcb18de773c5f34a5060572d4985ab9df2c1..08236374e5d4b82b2f9e2abdc34f124361937c5e 100644
>--- a/Source/WebCore/testing/Internals.h
>+++ b/Source/WebCore/testing/Internals.h
>@@ -195,6 +195,9 @@ public:
>     ExceptionOr<bool> pauseTransitionAtTimeOnElement(const String& propertyName, double pauseTime, Element&);
>     ExceptionOr<bool> pauseTransitionAtTimeOnPseudoElement(const String& property, double pauseTime, Element&, const String& pseudoId);
> 
>+    // For animations testing, we need a way to get at pseudo elements.
>+    ExceptionOr<RefPtr<Element>> pseudoElement(Element&, const String&);
>+
>     Node* treeScopeRootNode(Node&);
>     Node* parentTreeScope(Node&);
> 
>diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
>index 8d9c267cdde0d465dbff7734e92b660c478fc388..cb508caf3f020758db9668eba2dc2310d7d64a27 100644
>--- a/Source/WebCore/testing/Internals.idl
>+++ b/Source/WebCore/testing/Internals.idl
>@@ -148,6 +148,9 @@ enum EventThrottlingBehavior {
>     [MayThrowException] boolean pauseTransitionAtTimeOnElement(DOMString propertyName, unrestricted double pauseTime, Element element);
>     [MayThrowException] boolean pauseTransitionAtTimeOnPseudoElement(DOMString property, unrestricted double pauseTime, Element element, DOMString pseudoId);
> 
>+    // For animations testing, we need a way to get at pseudo elements.
>+    [MayThrowException] Element? pseudoElement(Element element, DOMString pseudoId);
>+
>     DOMString visiblePlaceholder(Element element);
>     void selectColorInColorChooser(HTMLInputElement element, DOMString colorValue);
>     [MayThrowException] sequence<DOMString> formControlStateOfPreviousHistoryItem();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 2ce1d0a52ca71d966faed23858b0e5627ecb2fe6..45552d1170160b452b46e91760172bddf39acb41 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,28 @@
>+2018-05-14  Antoine Quint  <graouts@apple.com>
>+
>+        [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
>+        https://bugs.webkit.org/show_bug.cgi?id=185612
>+        <rdar://problem/39579344>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Some tests that were opting into the new animation engine were using internals methods (pauseAnimationAtTimeOnElement, pauseTransitionAtTimeOnElement, etc.)
>+        that enforce the creation of animations in the old animation engine. Meanwhile, the code that toggles the animation engine used based on HTML comments is run
>+        prior to teardown of the previous test and so a test running with the new engine would run with the legacy engine during teardown. These two factors would
>+        cause `ASSERT(!frame().animation().hasAnimations())` to fail under FrameView::didDestroyRenderTree().
>+
>+        We update tests that use these internals method to use the Web Animations API instead and opt into the new animation engine if they didn't already do that.
>+
>+        * animations/animation-hit-test-transform.html:
>+        * animations/keyframes-dynamic-expected.txt:
>+        * animations/keyframes-dynamic.html:
>+        * animations/missing-from-to-expected.txt:
>+        * animations/missing-from-to-transforms-expected.txt:
>+        * animations/missing-from-to-transforms.html:
>+        * animations/missing-from-to.html:
>+        * fast/css-generated-content/pseudo-animation.html:
>+        * transitions/transition-hit-test-transform.html:
>+
> 2018-05-13  Andy VanWagoner  <andy@vanwagoner.family>
> 
>         [INTL] Improve spec & test262 compliance for Intl APIs
>diff --git a/LayoutTests/animations/animation-hit-test-transform.html b/LayoutTests/animations/animation-hit-test-transform.html
>index 49f50e76a36844bea46aba83b7f8c11da130daae..7807bc7aca62770b31f10a861e864ff49f135811 100644
>--- a/LayoutTests/animations/animation-hit-test-transform.html
>+++ b/LayoutTests/animations/animation-hit-test-transform.html
>@@ -1,5 +1,4 @@
>-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
>-   "http://www.w3.org/TR/html4/loose.dtd">
>+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
> 
> <html lang="en">
> <head>
>@@ -56,20 +55,15 @@
>      
>         function doTest()
>         {
>-            if (window.testRunner) {
>-                var target = document.getElementById("target");
>-                if (!internals.pauseAnimationAtTimeOnElement("anim", 2.0, target)) {
>-                    document.getElementById('result').innerHTML = "FAIL: Failed to pause animation"
>-                    testRunner.notifyDone();
>-                    return;
>-                }
>-        
>-                checkResults();
>+            var target = document.getElementById("target");
>+            if (!pauseAnimationAtTimeOnElement("anim", 2.0, target)) {
>+                document.getElementById('result').innerHTML = "FAIL: Failed to pause animation"
>                 testRunner.notifyDone();
>+                return;
>             }
>-            else {
>-                window.setTimeout("checkResults()", 2000);
>-            }
>+    
>+            checkResults();
>+            testRunner.notifyDone();
>         }
>     
>         function startTest()
>diff --git a/LayoutTests/animations/keyframes-dynamic-expected.txt b/LayoutTests/animations/keyframes-dynamic-expected.txt
>index d209f184d6490251fc051a78bd14d82d5c962854..cf6e10cbbb9723939112aab5e26f1be223e141ae 100644
>--- a/LayoutTests/animations/keyframes-dynamic-expected.txt
>+++ b/LayoutTests/animations/keyframes-dynamic-expected.txt
>@@ -1,7 +1,8 @@
> This test performs an animation of the left property. It should stop for 100ms at 100px and 200px We test for those values 50ms after it has stopped at each position. The animations for the three boxes are inserted by JavaScript. The first box's keyframes remain in the stylesheet. The second box's keyframes are removed after its animation starts (but it should animate). The third box's keyframes are removed before its animation starts, and it should not animate.
>-PASS - box3 animation was not running
> PASS - "left" property for "box1" element at 0.3s saw something close to: 100
> PASS - "left" property for "box1" element at 0.7s saw something close to: 200
> PASS - "left" property for "box2" element at 0.3s saw something close to: 100
> PASS - "left" property for "box2" element at 0.7s saw something close to: 200
>+PASS - "left" property for "box3" element at 0.3s saw something close to: 0
>+PASS - "left" property for "box3" element at 0.7s saw something close to: 0
> 
>diff --git a/LayoutTests/animations/keyframes-dynamic.html b/LayoutTests/animations/keyframes-dynamic.html
>index 9d78f3d29baa894ffd555655e8cf8a09e59b8cfa..cdd40c141953d35b11e9e3a5fbfd1aff4405efaa 100644
>--- a/LayoutTests/animations/keyframes-dynamic.html
>+++ b/LayoutTests/animations/keyframes-dynamic.html
>@@ -1,5 +1,4 @@
>-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
>-   "http://www.w3.org/TR/html4/loose.dtd">
>+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
> 
> <html lang="en">
> <head>
>@@ -28,6 +27,8 @@
>       ["anim1", 0.7, "box1", "left", 200, 1],
>       ["anim2", 0.3, "box2", "left", 100, 1],
>       ["anim2", 0.7, "box2", "left", 200, 1],
>+      ["anim3", 0.3, "box3", "left", 0, 0],
>+      ["anim3", 0.7, "box3", "left", 0, 0],
>     ];
> 
>     function addKeyframes() {
>@@ -64,13 +65,6 @@
>         style.sheet.removeRule(box3Index);
> 
>         runAnimationTest(expectedValues);
>-
>-        if (window.testRunner) {
>-            if (internals.pauseAnimationAtTimeOnElement("anim3", 0.1, box3))
>-                result += "FAIL - box3 animation was running<br>";
>-            else
>-                result += "PASS - box3 animation was not running<br>";
>-        }
>     }
>     
>     window.addEventListener('DOMContentLoaded', addKeyframes, false);
>diff --git a/LayoutTests/animations/missing-from-to-expected.txt b/LayoutTests/animations/missing-from-to-expected.txt
>index 4ddf27dbc07794c22da5decfd3c57c80a747b9e2..5b3ff41249046b3d8343bb8edc1391302bb493bc 100644
>--- a/LayoutTests/animations/missing-from-to-expected.txt
>+++ b/LayoutTests/animations/missing-from-to-expected.txt
>@@ -1,5 +1,4 @@
> This test performs animations of the left property on five boxes over 2 seconds. Box 1 has all keyframes. Box 2 has a missing "from" keyframe. Box 3 has a missing "to" keyframe. Box 4 has both "from" and "to" keyframes missing, but other keyframes which should trigger the generation of "from" and "to". Box 5 has no keyframes, and should not animate. The test takes 3 snapshots each and expects each result to be within a specified range.
>-PASS - box5 animation was not running
> PASS - "left" property for "box1" element at 0.4s saw something close to: 20
> PASS - "left" property for "box1" element at 1s saw something close to: 20
> PASS - "left" property for "box1" element at 1.6s saw something close to: 15
>@@ -12,4 +11,7 @@ PASS - "left" property for "box3" element at 1.6s saw something close to: 15
> PASS - "left" property for "box4" element at 0.4s saw something close to: 20
> PASS - "left" property for "box4" element at 1s saw something close to: 25
> PASS - "left" property for "box4" element at 1.6s saw something close to: 15
>+PASS - "left" property for "box5" element at 0.4s saw something close to: 10
>+PASS - "left" property for "box5" element at 1s saw something close to: 10
>+PASS - "left" property for "box5" element at 1.6s saw something close to: 10
> 
>diff --git a/LayoutTests/animations/missing-from-to-transforms-expected.txt b/LayoutTests/animations/missing-from-to-transforms-expected.txt
>index a40bc1da98982cb976de8641b6710342756c0418..e5da3b26e4625fd457df8571f27834b9b6969b28 100644
>--- a/LayoutTests/animations/missing-from-to-transforms-expected.txt
>+++ b/LayoutTests/animations/missing-from-to-transforms-expected.txt
>@@ -1,5 +1,4 @@
> This test performs animations of the transform property on five boxes over 2 seconds. Box 1 has all keyframes. Box 2 has a missing "from" keyframe. Box 3 has a missing "to" keyframe. Box 4 has both "from" and "to" keyframes missing, but other keyframes which should trigger the generation of "from" and "to". Box 5 has no keyframes, and should not animate. The test takes 3 snapshots each and expects each result to be within a specified range.
>-PASS - box5 animation was not running
> PASS - "webkitTransform.4" property for "box1" element at 0.4s saw something close to: 20
> PASS - "webkitTransform.4" property for "box1" element at 1s saw something close to: 20
> PASS - "webkitTransform.4" property for "box1" element at 1.6s saw something close to: 15
>@@ -12,4 +11,7 @@ PASS - "webkitTransform.4" property for "box3" element at 1.6s saw something clo
> PASS - "webkitTransform.4" property for "box4" element at 0.4s saw something close to: 20
> PASS - "webkitTransform.4" property for "box4" element at 1s saw something close to: 25
> PASS - "webkitTransform.4" property for "box4" element at 1.6s saw something close to: 15
>+PASS - "webkitTransform.4" property for "box5" element at 0.4s saw something close to: 10
>+PASS - "webkitTransform.4" property for "box5" element at 1s saw something close to: 10
>+PASS - "webkitTransform.4" property for "box5" element at 1.6s saw something close to: 10
> 
>diff --git a/LayoutTests/animations/missing-from-to-transforms.html b/LayoutTests/animations/missing-from-to-transforms.html
>index debeaa3a520735f64444fbf20c00cd728fab475c..e86fef6fe324d4251d346b6beba908c5f89fdde6 100644
>--- a/LayoutTests/animations/missing-from-to-transforms.html
>+++ b/LayoutTests/animations/missing-from-to-transforms.html
>@@ -71,7 +71,7 @@
>   <script src="resources/animation-test-helpers.js" type="text/javascript" charset="utf-8"></script>
>   <script type="text/javascript" charset="utf-8">
> 
>-    const expectedValues = [
>+    runAnimationTest([
>       // [animation-name, time, element-id, property, expected-value, tolerance]
>       ["anim1", 0.4, "box1", "webkitTransform.4", 20, 2],
>       ["anim1", 1.0, "box1", "webkitTransform.4", 20, 2],
>@@ -84,18 +84,11 @@
>       ["anim3", 1.6, "box3", "webkitTransform.4", 15, 2],
>       ["anim4", 0.4, "box4", "webkitTransform.4", 20, 2],
>       ["anim4", 1.0, "box4", "webkitTransform.4", 25, 2],
>-      ["anim4", 1.6, "box4", "webkitTransform.4", 15, 2]
>-    ];
>-
>-    runAnimationTest(expectedValues, function() {
>-      if (window.testRunner) {
>-          var box5Element = document.getElementById('box5');
>-          if (internals.pauseAnimationAtTimeOnElement("anim5", 0.1, box5Element))
>-              result += "FAIL - box5 animation was running<br>";
>-          else 
>-              result += "PASS - box5 animation was not running<br>";
>-      }
>-    });
>+      ["anim4", 1.6, "box4", "webkitTransform.4", 15, 2],
>+      ["anim5", 0.4, "box5", "webkitTransform.4", 10, 0],
>+      ["anim5", 1.0, "box5", "webkitTransform.4", 10, 0],
>+      ["anim5", 1.6, "box5", "webkitTransform.4", 10, 0]
>+    ]);
>     
>   </script>
> </head>
>diff --git a/LayoutTests/animations/missing-from-to.html b/LayoutTests/animations/missing-from-to.html
>index 851707f24dc7f7f492959962a4797c2f25a9de73..28c1f6ceb043f80397527debb0d48a02e0ec51ac 100644
>--- a/LayoutTests/animations/missing-from-to.html
>+++ b/LayoutTests/animations/missing-from-to.html
>@@ -1,5 +1,4 @@
>-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
>-   "http://www.w3.org/TR/html4/loose.dtd">
>+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
> 
> <html lang="en">
> <head>
>@@ -71,7 +70,7 @@
>   <script src="resources/animation-test-helpers.js" type="text/javascript" charset="utf-8"></script>
>   <script type="text/javascript" charset="utf-8">
> 
>-    const expectedValues = [
>+    runAnimationTest([
>       // [animation-name, time, element-id, property, expected-value, tolerance]
>       ["anim1", 0.4, "box1", "left", 20, 2],
>       ["anim1", 1.0, "box1", "left", 20, 2],
>@@ -84,18 +83,11 @@
>       ["anim3", 1.6, "box3", "left", 15, 2],
>       ["anim4", 0.4, "box4", "left", 20, 2],
>       ["anim4", 1.0, "box4", "left", 25, 2],
>-      ["anim4", 1.6, "box4", "left", 15, 2]
>-    ];
>-
>-    runAnimationTest(expectedValues, function() {
>-      if (window.testRunner) {
>-          var box5Element = document.getElementById('box5');
>-          if (internals.pauseAnimationAtTimeOnElement("anim5", 0.1, box5Element))
>-              result += "FAIL - box5 animation was running<br>";
>-          else 
>-              result += "PASS - box5 animation was not running<br>";
>-      }
>-    });
>+      ["anim4", 1.6, "box4", "left", 15, 2],
>+      ["anim5", 0.4, "box5", "left", 10, 0],
>+      ["anim5", 1.0, "box5", "left", 10, 0],
>+      ["anim5", 1.6, "box5", "left", 10, 0]
>+    ]);
>     
>   </script>
> </head>
>diff --git a/LayoutTests/fast/css-generated-content/pseudo-animation.html b/LayoutTests/fast/css-generated-content/pseudo-animation.html
>index 3a0c6c49a8c3f2514f76833836589d68e92cf717..b05132ce33e34a81594bde6c6acf7fea19c8fb9f 100644
>--- a/LayoutTests/fast/css-generated-content/pseudo-animation.html
>+++ b/LayoutTests/fast/css-generated-content/pseudo-animation.html
>@@ -1,4 +1,4 @@
>-<!DOCTYPE html>
>+<!DOCTYPE html>
> 
> <script src="../../resources/js-test-pre.js"></script>
> 
>@@ -78,6 +78,25 @@ function getPseudoComputedTop(id)
> // FIXME: This test should be modified so subpixel doesn't cause off by one
> // below and it no longer needs shouldBeCloseTo.
> 
>+function pauseAnimationAtTimeOnPseudoElement(animationName, time, element, pseudoId)
>+{
>+    const pseudoElement = internals.pseudoElement(element, pseudoId);
>+    if (!pseudoElement) {
>+        console.log("Failed to find pseudo element");
>+        return;
>+    }
>+
>+    const animations = pseudoElement.getAnimations();
>+    for (let animation of animations) {
>+        if (animation instanceof CSSAnimation && animation.animationName == animationName && animation.effect.getKeyframes().length) {
>+            animation.currentTime = time * 1000;
>+            animation.pause();
>+            return true;
>+        }
>+    }
>+    return false;
>+}
>+
> function testAnimation(id)
> {
>     var div = document.getElementById(id);
>@@ -85,11 +104,11 @@ function testAnimation(id)
>     window.div = div;
>     shouldBe('div.offsetWidth', '52');
>     if (window.internals) {
>-        internals.pauseAnimationAtTimeOnPseudoElement('example', 1.0, div, id);
>+        pauseAnimationAtTimeOnPseudoElement('example', 1.0, div, id);
>         shouldBeCloseTo('div.offsetWidth', 20, 1);
>         computedTop = getPseudoComputedTop(id);
>         shouldBeCloseTo('computedTop', 170, 1);
>-        internals.pauseAnimationAtTimeOnPseudoElement('example', 2.0, div, id);
>+        pauseAnimationAtTimeOnPseudoElement('example', 2.0, div, id);
>         shouldBeCloseTo('div.offsetWidth', 12, 1);
>         computedTop = getPseudoComputedTop(id);
>         shouldBeCloseTo('computedTop', 200, 1);
>diff --git a/LayoutTests/transitions/transition-hit-test-transform.html b/LayoutTests/transitions/transition-hit-test-transform.html
>index 1c0a035b68b63f90216941899cde56b293c2f031..88748161f8458b07f681518737fe146a4c644d01 100644
>--- a/LayoutTests/transitions/transition-hit-test-transform.html
>+++ b/LayoutTests/transitions/transition-hit-test-transform.html
>@@ -1,4 +1,4 @@
>-<!DOCTYPE html>
>+<!DOCTYPE html>
> 
> <html>
> <head>
>@@ -49,7 +49,7 @@
>         {
>             if (window.testRunner) {
>                 var target = document.getElementById('target');
>-                if (!internals.pauseTransitionAtTimeOnElement("-webkit-transform", 2.0, target))
>+                if (!pauseTransitionAtTimeOnElement("-webkit-transform", 2.0, target))
>                     throw("Transition is not running");
>         
>                 checkResults();

Flags:

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

Actions: View | Formatted Diff | Diff

Attachments on bug 185612: 340319 | 340327