Changeset 223848 in webkit

Timestamp:

Oct 23, 2017 11:59:06 AM (6 years ago)

Author:

Antti Koivisto

Message:

Remember previous child renderer during render tree update
​https://bugs.webkit.org/show_bug.cgi?id=178659

Reviewed by Zalan Bujtas.

Source/WebCore:

We shouldn't need to recompute the previous renderer, we know it already.

• style/RenderTreePosition.cpp:

(WebCore::RenderTreePosition::previousSiblingRenderer const): Deleted.

No longer needed. This was also subtly wrong as doesn't take display:contents into account.

• style/RenderTreePosition.h:
• style/RenderTreeUpdater.cpp:

(WebCore::RenderTreeUpdater::updateRenderTree):
(WebCore::RenderTreeUpdater::textRendererIsNeeded):

Use the saved previous renderer.

(WebCore::RenderTreeUpdater::updateTextRenderer):
(WebCore::RenderTreeUpdater::storePreviousRenderer):

Save the previous renderere as we walk the tree.

(WebCore::textRendererIsNeeded): Deleted.

• style/RenderTreeUpdater.h:

LayoutTests:

• fast/block/float/float-not-removed-from-pre-block-expected.txt:
• platform/mac/fast/css-generated-content/details-summary-before-after-expected.txt:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt (modified) (1 diff)
• LayoutTests/platform/mac/fast/css-generated-content/details-summary-before-after-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/style/RenderTreePosition.cpp (modified) (1 diff)
• Source/WebCore/style/RenderTreePosition.h (modified) (1 diff)
• Source/WebCore/style/RenderTreeUpdater.cpp (modified) (8 diffs)
• Source/WebCore/style/RenderTreeUpdater.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-10-23  Antti Koivisto  <antti@apple.com>
+
+        Remember previous child renderer during render tree update
+        https://bugs.webkit.org/show_bug.cgi?id=178659
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/block/float/float-not-removed-from-pre-block-expected.txt:
+        * platform/mac/fast/css-generated-content/details-summary-before-after-expected.txt:
+
 2017-10-23  Daniel Bates  <dabates@apple.com>
 

trunk/LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt

@@ -1 +1 @@
 Bug 101970: Heap-use-after-free in WebCore::RenderLayerModelObject::hasSelfPaintingLayer
 Test passes if it does not crash.
-    
+   

trunk/LayoutTests/platform/mac/fast/css-generated-content/details-summary-before-after-expected.txt

@@ -25 +25 @@
                 text run at (0,0) width 30: "after"
         RenderBlock (anonymous) at (1,77) size 782x18
-          RenderText {#text} at (0,0) size 0x0
           RenderText {#text} at (0,0) size 50x18
             text run at (0,0) width 50: "Details "

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-10-23  Antti Koivisto  <antti@apple.com>
+
+        Remember previous child renderer during render tree update
+        https://bugs.webkit.org/show_bug.cgi?id=178659
+
+        Reviewed by Zalan Bujtas.
+
+        We shouldn't need to recompute the previous renderer, we know it already.
+
+        * style/RenderTreePosition.cpp:
+        (WebCore::RenderTreePosition::previousSiblingRenderer const): Deleted.
+
+            No longer needed. This was also subtly wrong as doesn't take display:contents into account.
+
+        * style/RenderTreePosition.h:
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::updateRenderTree):
+        (WebCore::RenderTreeUpdater::textRendererIsNeeded):
+
+            Use the saved previous renderer.
+
+        (WebCore::RenderTreeUpdater::updateTextRenderer):
+        (WebCore::RenderTreeUpdater::storePreviousRenderer):
+
+            Save the previous renderere as we walk the tree.
+
+        (WebCore::textRendererIsNeeded): Deleted.
+        * style/RenderTreeUpdater.h:
+
 2017-10-23  Keith Miller  <keith_miller@apple.com>
 

trunk/Source/WebCore/style/RenderTreePosition.cpp

@@ -55 +55 @@
     if (m_nextSibling == &siblingRenderer)
         m_hasValidNextSibling = false;
-}
-
-RenderObject* RenderTreePosition::previousSiblingRenderer(const Text& textNode) const
-{
-    if (textNode.renderer())
-        return textNode.renderer()->previousSibling();
-
-    auto* parentElement = m_parent.element();
-
-    auto composedChildren = composedTreeChildren(*parentElement);
-    for (auto it = composedChildren.at(textNode), end = composedChildren.end(); it != end; --it) {
-        if (auto* renderer = it->renderer())
-            return renderer;
-    }
-    if (auto* before = parentElement->beforePseudoElement())
-        return before->renderer();
-    return nullptr;
 }
 

trunk/Source/WebCore/style/RenderTreePosition.h

@@ -50 +50 @@
     void invalidateNextSibling(const RenderObject&);
 
-    RenderObject* previousSiblingRenderer(const Text&) const;
     RenderObject* nextSiblingRenderer(const Node&) const;
 

trunk/Source/WebCore/style/RenderTreeUpdater.cpp

@@ -178 +178 @@
                 updateTextRenderer(text, textUpdate);
 
+            storePreviousRenderer(text);
             it.traverseNextSkippingChildren();
             continue;
@@ -189 +190 @@
         // there may be other updates down the tree.
         if (!elementUpdates && !element.hasDisplayContents()) {
+            storePreviousRenderer(element);
             it.traverseNextSkippingChildren();
             continue;
@@ -195 +197 @@
         if (elementUpdates)
             updateElementRenderer(element, elementUpdates->update);
+
+        storePreviousRenderer(element);
 
         bool mayHaveRenderedDescendants = element.renderer() || (element.hasDisplayContents() && shouldCreateRenderer(element, renderTreePosition().parent()));
@@ -385 +389 @@
 }
 
-static bool textRendererIsNeeded(const Text& textNode, const RenderTreePosition& renderTreePosition)
-{
-    const RenderElement& parentRenderer = renderTreePosition.parent();
+bool RenderTreeUpdater::textRendererIsNeeded(const Text& textNode)
+{
+    const RenderElement& parentRenderer = renderTreePosition().parent();
     if (!parentRenderer.canHaveChildren())
         return false;
@@ -404 +408 @@
         return true;
 
-    RenderObject* previousRenderer = renderTreePosition.previousSiblingRenderer(textNode);
+    auto* previousRenderer = parent().previousChildRenderer;
     if (previousRenderer && previousRenderer->isBR()) // <span><br/> <br/></span>
         return false;
@@ -419 +423 @@
         while (first && first->isFloatingOrOutOfFlowPositioned())
             first = first->nextSibling();
-        RenderObject* nextRenderer = textNode.renderer() ? textNode.renderer() : renderTreePosition.nextSiblingRenderer(textNode);
+        RenderObject* nextRenderer = textNode.renderer() ? textNode.renderer() :  renderTreePosition().nextSiblingRenderer(textNode);
         if (!first || nextRenderer == first) {
             // Whitespace at the start of a block just goes away. Don't even make a render object for this text.
@@ -460 +464 @@
 {
     auto* existingRenderer = text.renderer();
-    bool needsRenderer = textRendererIsNeeded(text, renderTreePosition());
+    bool needsRenderer = textRendererIsNeeded(text);
 
     if (existingRenderer && textUpdate && textUpdate->inheritedDisplayContentsStyle) {
@@ -512 +516 @@
 }
 
+void RenderTreeUpdater::storePreviousRenderer(Node& node)
+{
+    auto* renderer = node.renderer();
+    if (!renderer)
+        return;
+    ASSERT(parent().previousChildRenderer != renderer);
+    parent().previousChildRenderer = renderer;
+}
+
 void RenderTreeUpdater::tearDownRenderers(Element& root)
 {

trunk/Source/WebCore/style/RenderTreeUpdater.h

@@ -65 +65 @@
     void updateBeforeDescendants(Element&, const Style::ElementUpdates*);
     void updateAfterDescendants(Element&, const Style::ElementUpdates*);
+    bool textRendererIsNeeded(const Text& textNode);
+    void storePreviousRenderer(Node&);
 
     struct Parent {
@@ -70 +72 @@
         const Style::ElementUpdates* updates { nullptr };
         std::optional<RenderTreePosition> renderTreePosition;
+        RenderObject* previousChildRenderer { nullptr };
 
         Parent(ContainerNode& root);