REGRESSION (r181720): Unnecessary layout triggered any time animated GIF advances...
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jun 2015 02:39:15 +0000 (02:39 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jun 2015 02:39:15 +0000 (02:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145733

Reviewed by Andreas Kling.

Source/WebCore:

Test: fast/images/animated-gif-no-layout.html

* rendering/RenderImage.cpp:
(WebCore::RenderImage::styleDidChange): Correctly pass ImageSizeChangeNone in cases
where we don't need to report a change in intrinsic size that happened outside the
repaintOrMarkForLayout function.
(WebCore::RenderImage::repaintOrMarkForLayout): Move work that should only be done
when size changed inside the if statement.

* testing/Internals.cpp:
(WebCore::Internals::layoutCount): Added.
* testing/Internals.h: Added layoutCount.
* testing/Internals.idl: Ditto.

LayoutTests:

* TestExpectations: Expect image failures on the animated GIF tests (the one
old one I am fixing and the one new one I am adding) because they don't yet work
under DumpRenderTree.

* fast/images/animated-gif-no-layout-expected.html: Added.
* fast/images/animated-gif-no-layout.html: Added.

* fast/images/gif-loop-count-expected.html: Added. This test was worthless as a render
tree dump test, and only valuable as a pixel test. And that hid the fact that it was
failing under WebKit1. Changing it to a reference test makes it a valuable test again.
* fast/images/gif-loop-count-expected.png: Removed.
* fast/images/gif-loop-count-expected.txt: Removed.

* platform/wk2/TestExpectations: Expect successes on these two tests.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@185310 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/images/animated-gif-no-layout-expected.html [new file with mode: 0644]
LayoutTests/fast/images/animated-gif-no-layout.html [new file with mode: 0644]
LayoutTests/fast/images/gif-loop-count-expected.html [new file with mode: 0644]
LayoutTests/fast/images/gif-loop-count-expected.png [deleted file]
LayoutTests/fast/images/gif-loop-count-expected.txt [deleted file]
LayoutTests/platform/wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderImage.cpp
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 100c0aec4a1a10a605088e0045691d51fbc22a41..51b4e4c32b4de235744981cb1758a124c5941550 100644 (file)
@@ -1,3 +1,25 @@
+2015-06-07  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r181720): Unnecessary layout triggered any time animated GIF advances to a new frame
+        https://bugs.webkit.org/show_bug.cgi?id=145733
+
+        Reviewed by Andreas Kling.
+
+        * TestExpectations: Expect image failures on the animated GIF tests (the one
+        old one I am fixing and the one new one I am adding) because they don't yet work
+        under DumpRenderTree.
+
+        * fast/images/animated-gif-no-layout-expected.html: Added.
+        * fast/images/animated-gif-no-layout.html: Added.
+
+        * fast/images/gif-loop-count-expected.html: Added. This test was worthless as a render
+        tree dump test, and only valuable as a pixel test. And that hid the fact that it was
+        failing under WebKit1. Changing it to a reference test makes it a valuable test again.
+        * fast/images/gif-loop-count-expected.png: Removed.
+        * fast/images/gif-loop-count-expected.txt: Removed.
+
+        * platform/wk2/TestExpectations: Expect successes on these two tests.
+
 2015-06-06  Alexey Proskuryakov  <ap@apple.com>
 
         Clean up tests for blocking mixed content
index 5c6a4dfbbb9aad02a517c81b023fd01b96e6fb96..fa2d2626ded9fafc0f07b223a3aebdb6d1d45478 100644 (file)
@@ -546,3 +546,6 @@ webkit.org/b/145090 inspector/debugger/break-on-uncaught-exception-throw-in-prom
 webkit.org/b/145090 inspector/debugger/break-on-uncaught-exception-throw-in-promise-rethrow-in-catch.html [ Skip ]
 webkit.org/b/145090 inspector/debugger/break-on-uncaught-exception-window-onerror.html [ Skip ]
 
+# DumpRenderTree does not allow GIFs to animate, thus animated GIF tests don't work in WebKit1.
+fast/images/animated-gif-no-layout.html [ ImageOnlyFailure ]
+fast/images/gif-loop-count.html [ ImageOnlyFailure ]
diff --git a/LayoutTests/fast/images/animated-gif-no-layout-expected.html b/LayoutTests/fast/images/animated-gif-no-layout-expected.html
new file mode 100644 (file)
index 0000000..1232788
--- /dev/null
@@ -0,0 +1,2 @@
+<div style="position:absolute;left:0;top:0;width:100;height:100;background-color:#21B14D"></div>
+<img src="resources/rgb-jpeg-green.jpg" style="position:absolute;left:0;top:150">
diff --git a/LayoutTests/fast/images/animated-gif-no-layout.html b/LayoutTests/fast/images/animated-gif-no-layout.html
new file mode 100644 (file)
index 0000000..fe2e9b8
--- /dev/null
@@ -0,0 +1,43 @@
+<script>
+    // This test waits for a GIF repaint and double checks that there is a repaint only,
+    // no layout. If the test passes, there are two green squares. If it fails, then one
+    // or both of the squares is red instead.
+
+    var layoutCountBeforeTimer;
+
+    function animationComplete()
+    {
+        document.body.offsetWidth; // Explicitly trigger a layout.
+
+        if (window.internals && window.testRunner) {
+            var count = internals.layoutCount - layoutCountBeforeTimer;
+            if (count == 0) {
+                var indicator = document.getElementById("indicator");
+                indicator.addEventListener("load", function() { testRunner.notifyDone(); })
+                indicator.src = "resources/rgb-jpeg-green.jpg";
+                return;
+            }
+            testRunner.notifyDone();
+        }
+    }
+
+    function test()
+    {
+        document.body.offsetWidth; // Explicitly trigger a layout.
+
+        if (window.internals && window.testRunner) {
+            testRunner.waitUntilDone();
+            layoutCountBeforeTimer = internals.layoutCount;
+        }
+
+        // The 200ms value here is longer than the time it takes for gif-loop-count.gif
+        // to advance from its first frame to its second. There should be no race because
+        // the GIF timer should already be scheduled and this timer should share the
+        // same time base.
+        setTimeout(animationComplete, 200);
+    }
+</script>
+<body onload="test()">
+<img src="resources/gif-loop-count.gif" style="position:absolute;left:0;top:0">
+<img id="indicator" src="resources/rgb-jpeg-red.jpg" style="position:absolute;left:0;top:150">
+</body>
diff --git a/LayoutTests/fast/images/gif-loop-count-expected.html b/LayoutTests/fast/images/gif-loop-count-expected.html
new file mode 100644 (file)
index 0000000..97c5e30
--- /dev/null
@@ -0,0 +1 @@
+<div style="position:absolute;left:0;top:0;width:100;height:100;background-color:#21B14D"></div>
diff --git a/LayoutTests/fast/images/gif-loop-count-expected.png b/LayoutTests/fast/images/gif-loop-count-expected.png
deleted file mode 100644 (file)
index 8113b17..0000000
Binary files a/LayoutTests/fast/images/gif-loop-count-expected.png and /dev/null differ
diff --git a/LayoutTests/fast/images/gif-loop-count-expected.txt b/LayoutTests/fast/images/gif-loop-count-expected.txt
deleted file mode 100644 (file)
index c33489a..0000000
+++ /dev/null
@@ -1,9 +0,0 @@
-layer at (0,0) size 800x600
-  RenderView at (0,0) size 800x600
-layer at (0,0) size 800x600
-  RenderBlock {HTML} at (0,0) size 800x600
-    RenderBody {BODY} at (8,8) size 784x584
-layer at (0,0) size 100x100
-  RenderImage {IMG} at (0,0) size 100x100
-layer at (0,0) size 100x100
-  RenderImage {IMG} at (0,0) size 100x100
index cf4d7f349173a555b93f484896be97c2121f37ae..52e72db97ed68a56cb2ce9dc854c637fad8dfbd2 100644 (file)
@@ -36,6 +36,7 @@
 ### (2) Classified failures without bug reports (yet)
 ### (3) Unclassified failures
 ### (4) Features that are not supported in WebKit2 and likely never will be
+### (5) Progressions, expected successes that are expected failures in WebKit1.
 
 
 ########################################
@@ -680,6 +681,15 @@ fast/text/shaping
 ### END OF (4) Features that are not supported in WebKit2 and likely never will be
 ########################################
 
+########################################
+### START OF (5) Progressions, expected successes that are expected failures in WebKit1.
+
+# DumpRenderTree does not allow GIFs to animate, thus animated GIF tests don't work in WebKit1.
+fast/images/animated-gif-no-layout.html [ Pass ]
+fast/images/gif-loop-count.html [ Pass ]
+
+### END OF (5) Progressions, expected successes that are expected failures in WebKit1.
+########################################
 
 ###### This file has four sections. When adding new tests, make sure to
 ###### add to the right section:
@@ -688,3 +698,4 @@ fast/text/shaping
 ### (2) Classified failures without bug reports (yet)
 ### (3) Unclassified failures
 ### (4) Features that are not supported in WebKit2 and likely never will be
+### (5) Progressions, expected successes that are expected failures in WebKit1.
index a21f51a1ca7ee561c23ae8a1ccf5fe09e2b1491f..b043fba8e50b1ae4a997b7f877b4be23ad4947bd 100644 (file)
@@ -1,3 +1,24 @@
+2015-06-07  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r181720): Unnecessary layout triggered any time animated GIF advances to a new frame
+        https://bugs.webkit.org/show_bug.cgi?id=145733
+
+        Reviewed by Andreas Kling.
+
+        Test: fast/images/animated-gif-no-layout.html
+
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::styleDidChange): Correctly pass ImageSizeChangeNone in cases
+        where we don't need to report a change in intrinsic size that happened outside the
+        repaintOrMarkForLayout function.
+        (WebCore::RenderImage::repaintOrMarkForLayout): Move work that should only be done
+        when size changed inside the if statement.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::layoutCount): Added.
+        * testing/Internals.h: Added layoutCount.
+        * testing/Internals.idl: Ditto.
+
 2015-06-07  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         Fix strncpy use in WebCore::Text::formatForDebugger
index 7c807608eaae3c22c4cfc801f11d568ce4d4d5d7..2984d57cab7f307d99f8fe0006271710621103bd 100644 (file)
@@ -214,7 +214,7 @@ void RenderImage::styleDidChange(StyleDifference diff, const RenderStyle* oldSty
     }
 #if ENABLE(CSS_IMAGE_ORIENTATION)
     if (diff == StyleDifferenceLayout && oldStyle->imageOrientation() != style().imageOrientation())
-        return repaintOrMarkForLayout(ImageSizeChangeForAltText);
+        return repaintOrMarkForLayout(ImageSizeChangeNone);
 #endif
 
 #if ENABLE(CSS_IMAGE_RESOLUTION)
@@ -222,7 +222,7 @@ void RenderImage::styleDidChange(StyleDifference diff, const RenderStyle* oldSty
         && (oldStyle->imageResolution() != style().imageResolution()
             || oldStyle->imageResolutionSnap() != style().imageResolutionSnap()
             || oldStyle->imageResolutionSource() != style().imageResolutionSource()))
-        repaintOrMarkForLayout(ImageSizeChangeForAltText);
+        repaintOrMarkForLayout(ImageSizeChangeNone);
 #endif
 }
 
@@ -304,26 +304,28 @@ void RenderImage::repaintOrMarkForLayout(ImageSizeChangeType imageSizeChange, co
 
     bool imageSourceHasChangedSize = oldIntrinsicSize != newIntrinsicSize || imageSizeChange != ImageSizeChangeNone;
 
-    if (imageSourceHasChangedSize)
+    if (imageSourceHasChangedSize) {
         setPreferredLogicalWidthsDirty(true);
 
-    // If the actual area occupied by the image has changed and it is not constrained by style then a layout is required.
-    bool imageSizeIsConstrained = style().logicalWidth().isSpecified() && style().logicalHeight().isSpecified();
-    bool needsLayout = !imageSizeIsConstrained && imageSourceHasChangedSize;
+        // If the actual area occupied by the image has changed and it is not constrained by style then a layout is required.
+        bool imageSizeIsConstrained = style().logicalWidth().isSpecified() && style().logicalHeight().isSpecified();
 
-    // FIXME: We only need to recompute the containing block's preferred size
-    // if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing).
-    // There's no easy way to detect that shrink-to-fit is needed, always force a layout.
-    bool containingBlockNeedsToRecomputePreferredSize =
-        style().logicalWidth().isPercentOrCalculated()
-        || style().logicalMaxWidth().isPercentOrCalculated()
-        || style().logicalMinWidth().isPercentOrCalculated();
+        // FIXME: We only need to recompute the containing block's preferred size
+        // if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing).
+        // There's no easy way to detect that shrink-to-fit is needed, always force a layout.
+        bool containingBlockNeedsToRecomputePreferredSize =
+            style().logicalWidth().isPercentOrCalculated()
+            || style().logicalMaxWidth().isPercentOrCalculated()
+            || style().logicalMinWidth().isPercentOrCalculated();
 
-    bool layoutSizeDependsOnIntrinsicSize = style().aspectRatioType() == AspectRatioFromIntrinsic;
+        bool layoutSizeDependsOnIntrinsicSize = style().aspectRatioType() == AspectRatioFromIntrinsic;
 
-    if (needsLayout || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize) {
-        setNeedsLayout();
-        return;
+        if (!imageSizeIsConstrained || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize) {
+            // FIXME: It's not clear that triggering a layout guarantees a repaint in all cases.
+            // But many callers do depend on this code causing a layout.
+            setNeedsLayout();
+            return;
+        }
     }
 
     if (everHadLayout() && !selfNeedsLayout()) {
index 2d29db75b78d758dc400064c20992e39998d7093..75019679fa796b098d451c320103aa72a18ccf88 100644 (file)
@@ -2232,6 +2232,14 @@ void Internals::updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks(Node*
     document->updateLayoutIgnorePendingStylesheets(Document::RunPostLayoutTasks::Synchronously);
 }
 
+unsigned Internals::layoutCount() const
+{
+    Document* document = contextDocument();
+    if (!document || !document->view())
+        return 0;
+    return document->view()->layoutCount();
+}
+
 #if !PLATFORM(IOS)
 static const char* cursorTypeToString(Cursor::Type cursorType)
 {
index ee6ce8b105a916a1f517af24d6a254c16250cb10..14e9612910e5ceeee016d6427483b6f5841ed2c1 100644 (file)
@@ -317,6 +317,7 @@ public:
 
     void updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks(ExceptionCode&);
     void updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks(Node*, ExceptionCode&);
+    unsigned layoutCount() const;
 
     PassRefPtr<ArrayBuffer> serializeObject(PassRefPtr<SerializedScriptValue>) const;
     PassRefPtr<SerializedScriptValue> deserializeBuffer(PassRefPtr<ArrayBuffer>) const;
index 5ddf81e904022fd6aea59d63a66cff1ef87e3fa6..5a124a08ccf5e5001311078052242753665b56b3 100644 (file)
@@ -298,6 +298,8 @@ enum ResourceLoadPriority {
     // specified without security checks. Unspecified means this document.
     [RaisesException] void updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks(optional Node node);
 
+    readonly attribute unsigned long layoutCount;
+
     // Returns a string with information about the mouse cursor used at the specified client location.
     [RaisesException] DOMString getCurrentCursorInfo();