Changeset 220858 in webkit

Timestamp:

Aug 17, 2017 8:19:38 AM (7 years ago)

Author:

Antti Koivisto

Message:

RenderListItem - Avoid render tree mutation during layout
​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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/ios/fast/doctypes/002-expected.txt (modified) (1 diff)
• LayoutTests/platform/ios/fast/lists/marker-before-empty-inline-expected.txt (modified) (2 diffs)
• LayoutTests/platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt (modified) (1 diff)
• LayoutTests/platform/mac/fast/doctypes/002-expected.txt (modified) (1 diff)
• LayoutTests/platform/mac/fast/lists/marker-before-empty-inline-expected.txt (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderListItem.cpp (modified) (5 diffs)
• Source/WebCore/rendering/RenderListItem.h (modified) (2 diffs)
• Source/WebCore/rendering/TextAutoSizing.cpp (modified) (2 diffs)
• Source/WebCore/style/RenderTreeUpdater.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/platform/ios/fast/doctypes/002-expected.txt

@@ -13 +13 @@
           RenderBlock {UL} at (0,0) size 744x20
             RenderListItem {LI} at (40,0) size 704x20
+              RenderListMarker at (-18,0) size 7x19: white bullet
               RenderListMarker at (-58,0) size 7x19: bullet
-              RenderListMarker at (-18,0) size 7x19: white bullet
               RenderText {#text} at (0,0) size 256x19
                 text run at (0,0) width 256: "Both bullets should be on the same line."

trunk/LayoutTests/platform/ios/fast/lists/marker-before-empty-inline-expected.txt

@@ -80 +80 @@
             RenderBlock {UL} at (0,0) size 744x20
               RenderListItem {LI} at (40,0) size 704x20
+                RenderListMarker at (-18,0) size 7x19: white bullet
                 RenderListMarker at (-58,0) size 7x19: bullet
-                RenderListMarker at (-18,0) size 7x19: white bullet
                 RenderText {#text} at (0,0) size 29x19
                   text run at (0,0) width 29: "item"
@@ -92 +92 @@
             RenderBlock {UL} at (0,0) size 744x20
               RenderListItem {LI} at (40,0) size 704x20
+                RenderListMarker at (-18,0) size 7x19: white bullet
                 RenderListMarker at (-58,0) size 7x19: bullet
-                RenderListMarker at (-18,0) size 7x19: white bullet
                 RenderText {#text} at (0,0) size 29x19
                   text run at (0,0) width 29: "item"

trunk/LayoutTests/platform/mac/css2.1/t0805-c5520-brdr-b-01-e-expected.txt

@@ -37 +37 @@
           RenderBlock {UL} at (0,0) size 744x54
             RenderListItem {LI} at (40,0) size 704x18
+              RenderListMarker at (-17,0) size 7x18: white bullet
               RenderListMarker at (-57,0) size 7x18: bullet
-              RenderListMarker at (-17,0) size 7x18: white bullet
               RenderText {#text} at (0,0) size 77x18
                 text run at (0,0) width 77: "dummy text"

trunk/LayoutTests/platform/mac/fast/doctypes/002-expected.txt

@@ -13 +13 @@
           RenderBlock {UL} at (0,0) size 744x18
             RenderListItem {LI} at (40,0) size 704x18
+              RenderListMarker at (-17,0) size 7x18: white bullet
               RenderListMarker at (-57,0) size 7x18: bullet
-              RenderListMarker at (-17,0) size 7x18: white bullet
               RenderText {#text} at (0,0) size 256x18
                 text run at (0,0) width 256: "Both bullets should be on the same line."

trunk/LayoutTests/platform/mac/fast/lists/marker-before-empty-inline-expected.txt

@@ -80 +80 @@
             RenderBlock {UL} at (0,0) size 744x18
               RenderListItem {LI} at (40,0) size 704x18
+                RenderListMarker at (-17,0) size 7x18: white bullet
                 RenderListMarker at (-57,0) size 7x18: bullet
-                RenderListMarker at (-17,0) size 7x18: white bullet
                 RenderText {#text} at (0,0) size 29x18
                   text run at (0,0) width 29: "item"
@@ -92 +92 @@
             RenderBlock {UL} at (0,0) size 744x18
               RenderListItem {LI} at (40,0) size 704x18
+                RenderListMarker at (-17,0) size 7x18: white bullet
                 RenderListMarker at (-57,0) size 7x18: bullet
-                RenderListMarker at (-17,0) size 7x18: white bullet
                 RenderText {#text} at (0,0) size 29x18
                   text run at (0,0) width 29: "item"

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/rendering/RenderListItem.cpp

@@ -97 +97 @@
 }
 
-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()
 {
@@ -294 +273 @@
 }
 
-void RenderListItem::insertOrMoveMarkerRendererIfNeeded()
-{
-    // Sanity check the location of our marker.
-    if (!m_marker)
-        return;
-
-    // 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())
-        return;
+void RenderListItem::updateMarkerRenderer()
+{
+    ASSERT_WITH_SECURITY_IMPLICATION(!view().layoutState());
+
+    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();
@@ -321 +309 @@
 
     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();
     }
-
 }
 
@@ -342 +320 @@
 {
     StackStats::LayoutCheckPoint layoutCheckPoint;
-    ASSERT(needsLayout()); 
-
-    insertOrMoveMarkerRendererIfNeeded();
+    ASSERT(needsLayout());
+
 #if !ASSERT_DISABLED
     SetForScope<bool> inListItemLayout(m_inLayout, true);
@@ -359 +336 @@
 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();
 }

trunk/Source/WebCore/rendering/RenderListItem.h

@@ -57 +57 @@
     void didDestroyListMarker() { m_marker = nullptr; }
 
+    void updateMarkerRenderer();
+
 #if !ASSERT_DISABLED
     bool inLayout() const { return m_inLayout; }
@@ -77 +79 @@
     void positionListMarker();
 
-    void styleDidChange(StyleDifference, const RenderStyle* oldStyle) override;
-
     void addOverflowFromChildren() override;
     void computePreferredLogicalWidths() override;
 
-    void insertOrMoveMarkerRendererIfNeeded();
     inline int calcValue() const;
     void updateValueNow() const;

trunk/Source/WebCore/rendering/TextAutoSizing.cpp

@@ -34 +34 @@
 #include "Logging.h"
 #include "RenderBlock.h"
+#include "RenderListItem.h"
 #include "RenderListMarker.h"
 #include "RenderText.h"
@@ -157 +158 @@
         newParentStyle.fontCascade().update(&node->document().fontSelector());
         parentRenderer->setStyle(WTFMove(newParentStyle));
+
+        if (is<RenderListItem>(*parentRenderer))
+            downcast<RenderListItem>(*parentRenderer).updateMarkerRenderer();
     }
 

trunk/Source/WebCore/style/RenderTreeUpdater.cpp

@@ -40 +40 @@
 #include "RenderDescendantIterator.h"
 #include "RenderFullScreen.h"
+#include "RenderListItem.h"
 #include "RenderNamedFlowThread.h"
 #include "RenderQuote.h"
@@ -234 +235 @@
         updateBeforeOrAfterPseudoElement(*parent.element, AFTER);
 
-        auto* renderer = parent.element->renderer();
-        if (is<RenderBlock>(renderer))
-            FirstLetter::update(downcast<RenderBlock>(*renderer));
-
-        if (parent.element->hasCustomStyleResolveCallbacks() && parent.styleChange == Style::Detach && renderer)
-            parent.element->didAttachRenderers();
+        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)
+                parent.element->didAttachRenderers();
+        }
     }
     m_parentStack.removeLast();
@@ -567 +571 @@
             updateQuotesUpTo(&child);
     }
+    if (is<RenderListItem>(*pseudoRenderer))
+        downcast<RenderListItem>(*pseudoRenderer).updateMarkerRenderer();
 }