RenderListItem - Avoid render tree mutation during layout
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 Aug 2017 15:19:38 +0000 (15:19 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 17 Aug 2017 15:19:38 +0000 (15:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=175666

Reviewed by Andreas Kling.

Source/WebCore:

Mutations should be done by RenderTreeUpdater only.

* rendering/RenderListItem.cpp:
(WebCore::RenderListItem::updateMarkerRenderer):

    This is now called by RenderTreeUpdater only.
    Remove code dealing with this being called at layout time.
    Merged marker construction code from styleDidChange here and renamed for clarity.

(WebCore::RenderListItem::layout):
(WebCore::RenderListItem::computePreferredLogicalWidths):

    Remove mutating calls.

(WebCore::RenderListItem::styleDidChange): Deleted.
(WebCore::RenderListItem::insertOrMoveMarkerRendererIfNeeded): Deleted.
* rendering/RenderListItem.h:
* rendering/TextAutoSizing.cpp:
(WebCore::TextAutoSizingValue::adjustTextNodeSizes):

    Call updateMarkerRenderer.

* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::popParent):
(WebCore::RenderTreeUpdater::updateBeforeOrAfterPseudoElement):

    Call updateMarkerRenderer.

LayoutTests:

Changes in render tree dumps that don't affect rendering.

* platform/ios/fast/doctypes/002-expected.txt:
* platform/ios/fast/lists/marker-before-empty-inline-expected.txt:
* platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt:
* platform/mac/fast/doctypes/002-expected.txt:
* platform/mac/fast/lists/marker-before-empty-inline-expected.txt:

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

LayoutTests/ChangeLog
LayoutTests/platform/ios/fast/doctypes/002-expected.txt
LayoutTests/platform/ios/fast/lists/marker-before-empty-inline-expected.txt
LayoutTests/platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt
LayoutTests/platform/mac/fast/doctypes/002-expected.txt
LayoutTests/platform/mac/fast/lists/marker-before-empty-inline-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderListItem.cpp
Source/WebCore/rendering/RenderListItem.h
Source/WebCore/rendering/TextAutoSizing.cpp
Source/WebCore/style/RenderTreeUpdater.cpp

index b568db1..70ba16c 100644 (file)
@@ -1,3 +1,18 @@
+2017-08-17  Antti Koivisto  <antti@apple.com>
+
+        RenderListItem - Avoid render tree mutation during layout
+        https://bugs.webkit.org/show_bug.cgi?id=175666
+
+        Reviewed by Andreas Kling.
+
+        Changes in render tree dumps that don't affect rendering.
+
+        * platform/ios/fast/doctypes/002-expected.txt:
+        * platform/ios/fast/lists/marker-before-empty-inline-expected.txt:
+        * platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt:
+        * platform/mac/fast/doctypes/002-expected.txt:
+        * platform/mac/fast/lists/marker-before-empty-inline-expected.txt:
+
 2017-08-17  Ms2ger  <Ms2ger@gmail.com>
 
         REGRESSION(r220751): [GTK] Layout Test imported/w3c/web-platform-tests/fetch/api/basic/scheme-about.any.html is failing
index 1a81e0d..b83d16b 100644 (file)
@@ -12,8 +12,8 @@ layer at (0,0) size 800x180
         RenderListItem {LI} at (40,0) size 744x20
           RenderBlock {UL} at (0,0) size 744x20
             RenderListItem {LI} at (40,0) size 704x20
-              RenderListMarker at (-58,0) size 7x19: bullet
               RenderListMarker at (-18,0) size 7x19: white bullet
+              RenderListMarker at (-58,0) size 7x19: bullet
               RenderText {#text} at (0,0) size 256x19
                 text run at (0,0) width 256: "Both bullets should be on the same line."
 layer at (8,8) size 80x80
index 6cf6d02..5af3003 100644 (file)
@@ -79,8 +79,8 @@ layer at (0,0) size 800x600
           RenderBlock (anonymous) at (0,0) size 744x20
             RenderBlock {UL} at (0,0) size 744x20
               RenderListItem {LI} at (40,0) size 704x20
-                RenderListMarker at (-58,0) size 7x19: bullet
                 RenderListMarker at (-18,0) size 7x19: white bullet
+                RenderListMarker at (-58,0) size 7x19: bullet
                 RenderText {#text} at (0,0) size 29x19
                   text run at (0,0) width 29: "item"
           RenderBlock (anonymous) at (0,20) size 744x20
@@ -91,8 +91,8 @@ layer at (0,0) size 800x600
           RenderBlock {DIV} at (0,0) size 744x20
             RenderBlock {UL} at (0,0) size 744x20
               RenderListItem {LI} at (40,0) size 704x20
-                RenderListMarker at (-58,0) size 7x19: bullet
                 RenderListMarker at (-18,0) size 7x19: white bullet
+                RenderListMarker at (-58,0) size 7x19: bullet
                 RenderText {#text} at (0,0) size 29x19
                   text run at (0,0) width 29: "item"
           RenderBlock (anonymous) at (0,20) size 744x20
index a97f961..97aa62f 100644 (file)
@@ -36,8 +36,8 @@ layer at (0,0) size 800x332
         RenderListItem {LI} at (40,0) size 744x80 [border: none (2px solid #0000FF) none]
           RenderBlock {UL} at (0,0) size 744x54
             RenderListItem {LI} at (40,0) size 704x18
-              RenderListMarker at (-57,0) size 7x18: bullet
               RenderListMarker at (-17,0) size 7x18: white bullet
+              RenderListMarker at (-57,0) size 7x18: bullet
               RenderText {#text} at (0,0) size 77x18
                 text run at (0,0) width 77: "dummy text"
             RenderListItem {LI} at (40,18) size 704x18
index cbd1e8d..f102ba1 100644 (file)
@@ -12,8 +12,8 @@ layer at (0,0) size 800x176
         RenderListItem {LI} at (40,0) size 744x18
           RenderBlock {UL} at (0,0) size 744x18
             RenderListItem {LI} at (40,0) size 704x18
-              RenderListMarker at (-57,0) size 7x18: bullet
               RenderListMarker at (-17,0) size 7x18: white bullet
+              RenderListMarker at (-57,0) size 7x18: bullet
               RenderText {#text} at (0,0) size 256x18
                 text run at (0,0) width 256: "Both bullets should be on the same line."
 layer at (8,8) size 80x80
index 702d57f..2a38837 100644 (file)
@@ -79,8 +79,8 @@ layer at (0,0) size 800x600
           RenderBlock (anonymous) at (0,0) size 744x18
             RenderBlock {UL} at (0,0) size 744x18
               RenderListItem {LI} at (40,0) size 704x18
-                RenderListMarker at (-57,0) size 7x18: bullet
                 RenderListMarker at (-17,0) size 7x18: white bullet
+                RenderListMarker at (-57,0) size 7x18: bullet
                 RenderText {#text} at (0,0) size 29x18
                   text run at (0,0) width 29: "item"
           RenderBlock (anonymous) at (0,18) size 744x18
@@ -91,8 +91,8 @@ layer at (0,0) size 800x600
           RenderBlock {DIV} at (0,0) size 744x18
             RenderBlock {UL} at (0,0) size 744x18
               RenderListItem {LI} at (40,0) size 704x18
-                RenderListMarker at (-57,0) size 7x18: bullet
                 RenderListMarker at (-17,0) size 7x18: white bullet
+                RenderListMarker at (-57,0) size 7x18: bullet
                 RenderText {#text} at (0,0) size 29x18
                   text run at (0,0) width 29: "item"
           RenderBlock (anonymous) at (0,18) size 744x18
index c7c9445..c927c99 100644 (file)
@@ -1,3 +1,38 @@
+2017-08-17  Antti Koivisto  <antti@apple.com>
+
+        RenderListItem - Avoid render tree mutation during layout
+        https://bugs.webkit.org/show_bug.cgi?id=175666
+
+        Reviewed by Andreas Kling.
+
+        Mutations should be done by RenderTreeUpdater only.
+
+        * rendering/RenderListItem.cpp:
+        (WebCore::RenderListItem::updateMarkerRenderer):
+
+            This is now called by RenderTreeUpdater only.
+            Remove code dealing with this being called at layout time.
+            Merged marker construction code from styleDidChange here and renamed for clarity.
+
+        (WebCore::RenderListItem::layout):
+        (WebCore::RenderListItem::computePreferredLogicalWidths):
+
+            Remove mutating calls.
+
+        (WebCore::RenderListItem::styleDidChange): Deleted.
+        (WebCore::RenderListItem::insertOrMoveMarkerRendererIfNeeded): Deleted.
+        * rendering/RenderListItem.h:
+        * rendering/TextAutoSizing.cpp:
+        (WebCore::TextAutoSizingValue::adjustTextNodeSizes):
+
+            Call updateMarkerRenderer.
+
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::popParent):
+        (WebCore::RenderTreeUpdater::updateBeforeOrAfterPseudoElement):
+
+            Call updateMarkerRenderer.
+
 2017-08-17  Don Olmstead  <don.olmstead@sony.com>
 
         [PAL] Move SessionID into PAL
index bfafb37..5a39c3f 100644 (file)
@@ -96,27 +96,6 @@ RenderStyle RenderListItem::computeMarkerStyle() const
     return markerStyle;
 }
 
-void RenderListItem::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
-{
-    RenderBlockFlow::styleDidChange(diff, oldStyle);
-
-    if (style().listStyleType() == NoneListStyle && (!style().listStyleImage() || style().listStyleImage()->errorOccurred())) {
-        if (m_marker) {
-            m_marker->destroy();
-            ASSERT(!m_marker);
-        }
-        return;
-    }
-
-    auto newStyle = computeMarkerStyle();
-    if (m_marker)
-        m_marker->setStyle(WTFMove(newStyle));
-    else {
-        m_marker = createRenderer<RenderListMarker>(*this, WTFMove(newStyle)).leakPtr();
-        m_marker->initializeStyle();
-    }
-}
-
 void RenderListItem::insertedIntoTree()
 {
     RenderBlockFlow::insertedIntoTree();
@@ -293,16 +272,25 @@ static RenderObject* firstNonMarkerChild(RenderBlock& parent)
     return child;
 }
 
-void RenderListItem::insertOrMoveMarkerRendererIfNeeded()
+void RenderListItem::updateMarkerRenderer()
 {
-    // Sanity check the location of our marker.
-    if (!m_marker)
-        return;
+    ASSERT_WITH_SECURITY_IMPLICATION(!view().layoutState());
 
-    // FIXME: Do not even try to reposition the marker when we are not in layout
-    // until after we fixed webkit.org/b/163789.
-    if (!view().frameView().isInRenderTreeLayout())
+    if (style().listStyleType() == NoneListStyle && (!style().listStyleImage() || style().listStyleImage()->errorOccurred())) {
+        if (m_marker) {
+            m_marker->destroy();
+            ASSERT(!m_marker);
+        }
         return;
+    }
+
+    auto newStyle = computeMarkerStyle();
+    if (m_marker)
+        m_marker->setStyle(WTFMove(newStyle));
+    else {
+        m_marker = createRenderer<RenderListMarker>(*this, WTFMove(newStyle)).leakPtr();
+        m_marker->initializeStyle();
+    }
 
     RenderElement* currentParent = m_marker->parent();
     RenderBlock* newParent = getParentOfFirstLineBox(*this, *m_marker);
@@ -320,30 +308,19 @@ void RenderListItem::insertOrMoveMarkerRendererIfNeeded()
     }
 
     if (newParent != currentParent) {
-        // Removing and adding the marker can trigger repainting in
-        // containers other than ourselves, so we need to disable LayoutState.
-        LayoutStateDisabler layoutStateDisabler(view());
-        // Mark the parent dirty so that when the marker gets inserted into the tree
-        // and dirties ancestors, it stops at the parent.
-        newParent->setChildNeedsLayout(MarkOnlyThis);
-        m_marker->setNeedsLayout(MarkOnlyThis);
-
         m_marker->removeFromParent();
         newParent->addChild(m_marker, firstNonMarkerChild(*newParent));
-        m_marker->updateMarginsAndContent();
         // If current parent is an anonymous block that has lost all its children, destroy it.
         if (currentParent && currentParent->isAnonymousBlock() && !currentParent->firstChild() && !downcast<RenderBlock>(*currentParent).continuation())
             currentParent->destroy();
     }
-
 }
 
 void RenderListItem::layout()
 {
     StackStats::LayoutCheckPoint layoutCheckPoint;
-    ASSERT(needsLayout()); 
+    ASSERT(needsLayout());
 
-    insertOrMoveMarkerRendererIfNeeded();
 #if !ASSERT_DISABLED
     SetForScope<bool> inListItemLayout(m_inLayout, true);
 #endif
@@ -358,14 +335,10 @@ void RenderListItem::addOverflowFromChildren()
 
 void RenderListItem::computePreferredLogicalWidths()
 {
-#ifndef NDEBUG
-    // FIXME: We shouldn't be modifying the tree in computePreferredLogicalWidths.
-    // Instead, we should insert the marker soon after the tree construction.
-    // This is similar case to RenderCounter::computePreferredLogicalWidths()
-    // See https://bugs.webkit.org/show_bug.cgi?id=104829
-    SetLayoutNeededForbiddenScope layoutForbiddenScope(this, false);
-#endif
-    insertOrMoveMarkerRendererIfNeeded();
+    // FIXME: RenderListMarker::updateMargins() mutates margin style which affects preferred widths.
+    if (m_marker && m_marker->preferredLogicalWidthsDirty())
+        m_marker->updateMarginsAndContent();
+
     RenderBlockFlow::computePreferredLogicalWidths();
 }
 
index 8c19118..ea552cd 100644 (file)
@@ -56,6 +56,8 @@ public:
 
     void didDestroyListMarker() { m_marker = nullptr; }
 
+    void updateMarkerRenderer();
+
 #if !ASSERT_DISABLED
     bool inLayout() const { return m_inLayout; }
 #endif
@@ -76,12 +78,9 @@ private:
 
     void positionListMarker();
 
-    void styleDidChange(StyleDifference, const RenderStyle* oldStyle) override;
-
     void addOverflowFromChildren() override;
     void computePreferredLogicalWidths() override;
 
-    void insertOrMoveMarkerRendererIfNeeded();
     inline int calcValue() const;
     void updateValueNow() const;
     void explicitValueChanged();
index 15e809e..b9d9b35 100644 (file)
@@ -33,6 +33,7 @@
 #include "FontCascade.h"
 #include "Logging.h"
 #include "RenderBlock.h"
+#include "RenderListItem.h"
 #include "RenderListMarker.h"
 #include "RenderText.h"
 #include "RenderTextFragment.h"
@@ -156,6 +157,9 @@ auto TextAutoSizingValue::adjustTextNodeSizes() -> StillHasNodes
         newParentStyle.setFontDescription(fontDescription);
         newParentStyle.fontCascade().update(&node->document().fontSelector());
         parentRenderer->setStyle(WTFMove(newParentStyle));
+
+        if (is<RenderListItem>(*parentRenderer))
+            downcast<RenderListItem>(*parentRenderer).updateMarkerRenderer();
     }
 
     for (auto& node : m_autoSizedNodes) {
index e70f4a2..792937a 100644 (file)
@@ -39,6 +39,7 @@
 #include "PseudoElement.h"
 #include "RenderDescendantIterator.h"
 #include "RenderFullScreen.h"
+#include "RenderListItem.h"
 #include "RenderNamedFlowThread.h"
 #include "RenderQuote.h"
 #include "RenderTreeUpdaterFirstLetter.h"
@@ -233,12 +234,15 @@ void RenderTreeUpdater::popParent()
     if (parent.element) {
         updateBeforeOrAfterPseudoElement(*parent.element, AFTER);
 
-        auto* renderer = parent.element->renderer();
-        if (is<RenderBlock>(renderer))
-            FirstLetter::update(downcast<RenderBlock>(*renderer));
+        if (auto* renderer = parent.element->renderer()) {
+            if (is<RenderBlock>(*renderer))
+                FirstLetter::update(downcast<RenderBlock>(*renderer));
+            if (is<RenderListItem>(*renderer))
+                downcast<RenderListItem>(*renderer).updateMarkerRenderer();
 
-        if (parent.element->hasCustomStyleResolveCallbacks() && parent.styleChange == Style::Detach && renderer)
-            parent.element->didAttachRenderers();
+            if (parent.element->hasCustomStyleResolveCallbacks() && parent.styleChange == Style::Detach)
+                parent.element->didAttachRenderers();
+        }
     }
     m_parentStack.removeLast();
 }
@@ -566,6 +570,8 @@ void RenderTreeUpdater::updateBeforeOrAfterPseudoElement(Element& current, Pseud
         for (auto& child : descendantsOfType<RenderQuote>(*pseudoRenderer))
             updateQuotesUpTo(&child);
     }
+    if (is<RenderListItem>(*pseudoRenderer))
+        downcast<RenderListItem>(*pseudoRenderer).updateMarkerRenderer();
 }
 
 void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownType)