Merge r220858 - RenderListItem - Avoid render tree mutation during la…

…yout 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:
WebKit · Aug 18, 2017 · b81a71e · b81a71e
1 parent b754a98
commit b81a71e
Show file tree

Hide file tree

Showing 11 changed files with 95 additions and 63 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -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-16  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [HarfBuzz] Decomposed Vietnamese characters are rendered incorrectly

diff --git a/LayoutTests/platform/ios/fast/doctypes/002-expected.txt b/LayoutTests/platform/ios/fast/doctypes/002-expected.txt
@@ -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

diff --git a/LayoutTests/platform/ios/fast/lists/marker-before-empty-inline-expected.txt b/LayoutTests/platform/ios/fast/lists/marker-before-empty-inline-expected.txt
@@ -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

diff --git a/LayoutTests/platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt b/LayoutTests/platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt
@@ -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

diff --git a/LayoutTests/platform/mac/fast/doctypes/002-expected.txt b/LayoutTests/platform/mac/fast/doctypes/002-expected.txt
@@ -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

diff --git a/LayoutTests/platform/mac/fast/lists/marker-before-empty-inline-expected.txt b/LayoutTests/platform/mac/fast/lists/marker-before-empty-inline-expected.txt
@@ -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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -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-18  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r220854.

diff --git a/Source/WebCore/rendering/RenderListItem.cpp b/Source/WebCore/rendering/RenderListItem.cpp
@@ -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();
 }
 

diff --git a/Source/WebCore/rendering/RenderListItem.h b/Source/WebCore/rendering/RenderListItem.h
@@ -56,6 +56,8 @@ class RenderListItem final : public RenderBlockFlow {
 
     void didDestroyListMarker() { m_marker = nullptr; }
 
+    void updateMarkerRenderer();
+
 #if !ASSERT_DISABLED
     bool inLayout() const { return m_inLayout; }
 #endif
@@ -76,12 +78,9 @@ class RenderListItem final : public RenderBlockFlow {
 
     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();

diff --git a/Source/WebCore/rendering/TextAutoSizing.cpp b/Source/WebCore/rendering/TextAutoSizing.cpp
@@ -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) {

diff --git a/Source/WebCore/style/RenderTreeUpdater.cpp b/Source/WebCore/style/RenderTreeUpdater.cpp
@@ -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)