WebCore:
authorbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 May 2009 00:16:35 +0000 (00:16 +0000)
committerbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 May 2009 00:16:35 +0000 (00:16 +0000)
2009-05-07  Eric Seidel  <eric@webkit.org>

        Reviewed by Beth Dakin.

        Fix regression caused by r41469, add test case to prevent it from
        happening again.
        https://bugs.webkit.org/show_bug.cgi?id=25252

        hasLayer() was true during removeOnlyThisLayer()/
        updateLayerPositions()
        which caused updateLayerPosition()'s walk up the render tree to
        include offsets from the layer we were about to remove.

        I'm not 100% convinced that this wasn't a bug in
        updateLayerPosition() or in RenderBoxModelObject::styleDidChange,
        because the layer in question is not the containing block for the
        block which gets laid out wrong. But this restores the previous
        behavior and adds a test.  So the next time someone is in here re-
        factoring, they will at least know if they break something.

        Test: fast/layers/remove-only-this-layer-update.html

        * rendering/RenderBoxModelObject.cpp:
        (WebCore::RenderBoxModelObject::destroyLayer):
        * rendering/RenderLayer.cpp:
        (WebCore::RenderLayer::removeOnlyThisLayer):
        * rendering/RenderObject.cpp:
        (WebCore::RenderObject::destroy):
        * rendering/RenderWidget.cpp:
        (WebCore::RenderWidget::destroy):

LayoutTests:

2009-05-07  Eric Seidel  <eric@webkit.org>

        Reviewed by Beth Dakin.

        Fix regression caused by r41469, add test case to prevent it from
        happening again.
        https://bugs.webkit.org/show_bug.cgi?id=25252

        * fast/layers/remove-only-this-layer-update.html: Added.
        * platform/mac/fast/layers/remove-only-this-layer-update-expected.checksum: Added.
        * platform/mac/fast/layers/remove-only-this-layer-update-expected.png: Added.
        * platform/mac/fast/layers/remove-only-this-layer-update-expected.txt: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/layers/remove-only-this-layer-update.html [new file with mode: 0644]
LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.checksum [new file with mode: 0644]
LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.txt [new file with mode: 0644]
WebCore/ChangeLog
WebCore/rendering/RenderBoxModelObject.cpp
WebCore/rendering/RenderLayer.cpp
WebCore/rendering/RenderObject.cpp
WebCore/rendering/RenderWidget.cpp

index b7ac2ae..f552699 100644 (file)
@@ -1,3 +1,16 @@
+2009-05-07  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Beth Dakin.
+
+        Fix regression caused by r41469, add test case to prevent it from 
+        happening again.
+        https://bugs.webkit.org/show_bug.cgi?id=25252
+
+        * fast/layers/remove-only-this-layer-update.html: Added.
+        * platform/mac/fast/layers/remove-only-this-layer-update-expected.checksum: Added.
+        * platform/mac/fast/layers/remove-only-this-layer-update-expected.png: Added.
+        * platform/mac/fast/layers/remove-only-this-layer-update-expected.txt: Added.
+
 2009-05-07  David Hyatt  <hyatt@apple.com>
 
         Restore intrinsic margins to all form controls on Mac and Windows.
diff --git a/LayoutTests/fast/layers/remove-only-this-layer-update.html b/LayoutTests/fast/layers/remove-only-this-layer-update.html
new file mode 100644 (file)
index 0000000..d6a468e
--- /dev/null
@@ -0,0 +1,19 @@
+<body style="margin:0px">
+    <div style="width: 100px; height: 100px; position: absolute; left: 100px; top: 100px; background-color: red">FAIL</div>
+    <div id="outer" style="opacity: 0.5; margin: 100px">
+        <div style="position: relative; background-color: green; width: 100px; height: 100px">PASS</div>
+    </div>
+You should see a 100x100 green rect at 100x100 above with the word PASS.  There should be no red on this page.  This is a test case for https://bugs.webkit.org/show_bug.cgi?id=25252
+
+    <script>
+        function doTest() {
+            document.getElementById("outer").style.opacity = 1.0;
+            if (window.layoutTestController)
+                layoutTestController.notifyDone();
+        }
+        if (window.layoutTestController)
+            layoutTestController.waitUntilDone();
+        // Delay until after the first paint
+        setTimeout(doTest, 0);
+    </script>
+</body>
diff --git a/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.checksum b/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.checksum
new file mode 100644 (file)
index 0000000..0a90d4e
--- /dev/null
@@ -0,0 +1 @@
+472ca17a009ad9a51e760c9f8a5b542d
\ No newline at end of file
diff --git a/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.png b/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.png
new file mode 100644 (file)
index 0000000..5fd0073
Binary files /dev/null and b/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.png differ
diff --git a/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.txt b/LayoutTests/platform/mac/fast/layers/remove-only-this-layer-update-expected.txt
new file mode 100644 (file)
index 0000000..ef0d1d4
--- /dev/null
@@ -0,0 +1,22 @@
+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 (0,100) size 800x500
+      RenderBlock {DIV} at (100,0) size 600x100
+      RenderBlock (anonymous) at (0,200) size 800x36
+        RenderText {#text} at (0,0) size 782x36
+          text run at (0,0) width 495: "You should see a 100x100 green rect at 100x100 above with the word PASS. "
+          text run at (495,0) width 234: "There should be no red on this page. "
+          text run at (729,0) width 53: "This is a"
+          text run at (0,18) width 383: "test case for https://bugs.webkit.org/show_bug.cgi?id=25252"
+        RenderText {#text} at (0,0) size 0x0
+        RenderText {#text} at (0,0) size 0x0
+layer at (100,100) size 100x100
+  RenderBlock (positioned) {DIV} at (100,100) size 100x100 [bgcolor=#FF0000]
+    RenderText {#text} at (0,0) size 36x18
+      text run at (0,0) width 36: "FAIL"
+layer at (100,100) size 100x100
+  RenderBlock (relative positioned) {DIV} at (0,0) size 100x100 [bgcolor=#008000]
+    RenderText {#text} at (0,0) size 39x18
+      text run at (0,0) width 39: "PASS"
index 64452c3..d15df84 100644 (file)
@@ -1,3 +1,34 @@
+2009-05-07  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Beth Dakin.
+
+        Fix regression caused by r41469, add test case to prevent it from 
+        happening again.
+        https://bugs.webkit.org/show_bug.cgi?id=25252
+  
+        hasLayer() was true during removeOnlyThisLayer()/
+        updateLayerPositions()
+        which caused updateLayerPosition()'s walk up the render tree to 
+        include offsets from the layer we were about to remove.
+  
+        I'm not 100% convinced that this wasn't a bug in 
+        updateLayerPosition() or in RenderBoxModelObject::styleDidChange, 
+        because the layer in question is not the containing block for the 
+        block which gets laid out wrong. But this restores the previous 
+        behavior and adds a test.  So the next time someone is in here re-
+        factoring, they will at least know if they break something.
+  
+        Test: fast/layers/remove-only-this-layer-update.html
+
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::destroyLayer):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::removeOnlyThisLayer):
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::destroy):
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::destroy):
+
 2009-05-07  Dmitry Titov  <dimich@chromium.org>
 
         Attempt to fix GTK build.
index f995b0f..9612218 100644 (file)
@@ -57,11 +57,10 @@ RenderBoxModelObject::~RenderBoxModelObject()
 
 void RenderBoxModelObject::destroyLayer()
 {
-    ASSERT(hasLayer());
+    ASSERT(!hasLayer()); // Callers should have already called setHasLayer(false)
     ASSERT(m_layer);
     m_layer->destroy(renderArena());
     m_layer = 0;
-    setHasLayer(false);
 }
 
 void RenderBoxModelObject::destroy()
index 5830ae9..e2e9a44 100644 (file)
@@ -861,7 +861,11 @@ void RenderLayer::removeOnlyThisLayer()
 {
     if (!m_parent)
         return;
-    
+
+    // Mark that we are about to lose our layer. This makes render tree
+    // walks ignore this layer while we're removing it.
+    m_renderer->setHasLayer(false);
+
 #if USE(ACCELERATED_COMPOSITING)
     compositor()->layerWillBeRemoved(m_parent, this);
 #endif
@@ -883,7 +887,7 @@ void RenderLayer::removeOnlyThisLayer()
         RenderLayer* next = current->nextSibling();
         removeChild(current);
         parent->addChild(current, nextSib);
-        current->updateLayerPositions();
+        current->updateLayerPositions(); // Depends on hasLayer() already being false for proper layout.
         current = next;
     }
 
index ae93aa5..b03ec50 100644 (file)
@@ -1860,8 +1860,10 @@ void RenderObject::destroy()
 
     // FIXME: Would like to do this in RenderBoxModelObject, but the timing is so complicated that this can't easily
     // be moved into RenderBoxModelObject::destroy.
-    if (hasLayer())
+    if (hasLayer()) {
+        setHasLayer(false);
         toRenderBoxModelObject(this)->destroyLayer();
+    }
     arenaDelete(renderArena(), this);
 }
 
index 11bedc1..7d9418c 100644 (file)
@@ -96,6 +96,7 @@ void RenderWidget::destroy()
 
     if (hasLayer()) {
         layer()->clearClipRects();
+        setHasLayer(false);
         destroyLayer();
     }