[RenderTreeBuilder] Move RenderBlock::removeLeftoverAnonymousBlock to RenderTreeBuilder
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Feb 2018 15:40:52 +0000 (15:40 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Feb 2018 15:40:52 +0000 (15:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182510
<rdar://problem/37250037>

Reviewed by Antti Koivisto.

Do not reinvent subtree reparenting.

Covered by existing tests.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::removeLeftoverAnonymousBlock): Deleted.
* rendering/RenderBlock.h:
* rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::moveAllChildrenToInternal):
* rendering/RenderBoxModelObject.h:
* rendering/RenderButton.h:
* rendering/RenderElement.cpp:
(WebCore::RenderElement::detachRendererInternal):
(WebCore::RenderElement::attachRendererInternal):
(WebCore::RenderElement::insertChildInternal):
(WebCore::RenderElement::takeChildInternal):
* rendering/RenderElement.h:
* rendering/RenderRuby.h:
* rendering/RenderRubyRun.h:
* rendering/RenderTextControl.h:
* rendering/updating/RenderTreeBuilderBlock.cpp:
(WebCore::RenderTreeBuilder::Block::insertChildIgnoringContinuation):
(WebCore::RenderTreeBuilder::Block::childBecameNonInline):
(WebCore::RenderTreeBuilder::Block::removeLeftoverAnonymousBlock):
* rendering/updating/RenderTreeBuilderBlock.h:

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

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderBlock.h
Source/WebCore/rendering/RenderBoxModelObject.cpp
Source/WebCore/rendering/RenderBoxModelObject.h
Source/WebCore/rendering/RenderButton.h
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/rendering/RenderRuby.h
Source/WebCore/rendering/RenderRubyRun.h
Source/WebCore/rendering/RenderTextControl.h
Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp
Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h

index be020e8..703c91e 100644 (file)
@@ -1,3 +1,37 @@
+2018-02-07  Zalan Bujtas  <zalan@apple.com>
+
+        [RenderTreeBuilder] Move RenderBlock::removeLeftoverAnonymousBlock to RenderTreeBuilder
+        https://bugs.webkit.org/show_bug.cgi?id=182510
+        <rdar://problem/37250037>
+
+        Reviewed by Antti Koivisto.
+
+        Do not reinvent subtree reparenting.
+
+        Covered by existing tests.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::removeLeftoverAnonymousBlock): Deleted.
+        * rendering/RenderBlock.h:
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::moveAllChildrenToInternal):
+        * rendering/RenderBoxModelObject.h:
+        * rendering/RenderButton.h:
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::detachRendererInternal):
+        (WebCore::RenderElement::attachRendererInternal):
+        (WebCore::RenderElement::insertChildInternal):
+        (WebCore::RenderElement::takeChildInternal):
+        * rendering/RenderElement.h:
+        * rendering/RenderRuby.h:
+        * rendering/RenderRubyRun.h:
+        * rendering/RenderTextControl.h:
+        * rendering/updating/RenderTreeBuilderBlock.cpp:
+        (WebCore::RenderTreeBuilder::Block::insertChildIgnoringContinuation):
+        (WebCore::RenderTreeBuilder::Block::childBecameNonInline):
+        (WebCore::RenderTreeBuilder::Block::removeLeftoverAnonymousBlock):
+        * rendering/updating/RenderTreeBuilderBlock.h:
+
 2018-02-06  Don Olmstead  <don.olmstead@sony.com>
 
         Remove WebCore/ForwardingHeaders directory
index bdff483..267f4b6 100644 (file)
@@ -482,58 +482,6 @@ void RenderBlock::deleteLines()
         cache->deferRecomputeIsIgnored(element());
 }
 
-void RenderBlock::removeLeftoverAnonymousBlock(RenderBlock* child)
-{
-    ASSERT(child->isAnonymousBlock());
-    ASSERT(!child->childrenInline());
-    
-    if (child->continuation())
-        return;
-    
-    RenderObject* firstAnChild = child->firstChild();
-    RenderObject* lastAnChild = child->lastChild();
-    if (firstAnChild) {
-        RenderObject* o = firstAnChild;
-        while (o) {
-            o->setParent(this);
-            o = o->nextSibling();
-        }
-        firstAnChild->setPreviousSibling(child->previousSibling());
-        lastAnChild->setNextSibling(child->nextSibling());
-        if (child->previousSibling())
-            child->previousSibling()->setNextSibling(firstAnChild);
-        if (child->nextSibling())
-            child->nextSibling()->setPreviousSibling(lastAnChild);
-            
-        if (child == firstChild())
-            setFirstChild(firstAnChild);
-        if (child == lastChild())
-            setLastChild(lastAnChild);
-    } else {
-        if (child == firstChild())
-            setFirstChild(child->nextSibling());
-        if (child == lastChild())
-            setLastChild(child->previousSibling());
-
-        if (child->previousSibling())
-            child->previousSibling()->setNextSibling(child->nextSibling());
-        if (child->nextSibling())
-            child->nextSibling()->setPreviousSibling(child->previousSibling());
-    }
-
-    child->setFirstChild(nullptr);
-    child->m_next = nullptr;
-
-    // Remove all the information in the flow thread associated with the leftover anonymous block.
-    child->resetFragmentedFlowStateOnRemoval();
-
-    child->setParent(nullptr);
-    child->setPreviousSibling(nullptr);
-    child->setNextSibling(nullptr);
-
-    child->destroy();
-}
-
 static bool canDropAnonymousBlock(const RenderBlock& anonymousBlock)
 {
     if (anonymousBlock.beingDestroyed() || anonymousBlock.continuation())
index 3c8f139..93fc78f 100644 (file)
@@ -396,7 +396,6 @@ public:
     void adjustBorderBoxRectForPainting(LayoutRect&) override;
     LayoutRect paintRectToClipOutFromBorder(const LayoutRect&) override;
     void addChildIgnoringContinuation(RenderTreeBuilder&, RenderPtr<RenderObject> newChild, RenderObject* beforeChild) override;
-    virtual void removeLeftoverAnonymousBlock(RenderBlock* child);
     bool isInlineBlockOrInlineTable() const final { return isInline() && isReplaced(); }
 
 protected:
index e4f5886..c198244 100644 (file)
@@ -2742,4 +2742,10 @@ void RenderBoxModelObject::moveChildrenTo(RenderBoxModelObject* toBoxModelObject
     }
 }
 
+void RenderBoxModelObject::moveAllChildrenToInternal(RenderElement& newParent)
+{
+    while (firstChild())
+        newParent.attachRendererInternal(detachRendererInternal(*firstChild()), this);
+}
+
 } // namespace WebCore
index 27f33d5..74c43a2 100644 (file)
@@ -290,6 +290,7 @@ public:
     {
         moveChildrenTo(toBoxModelObject, firstChild(), nullptr, beforeChild, normalizeAfterInsertion);
     }
+    void moveAllChildrenToInternal(RenderElement& newParent);
     // Move all of the kids from |startChild| up to but excluding |endChild|. 0 can be passed as the |endChild| to denote
     // that all the kids from |startChild| onwards should be moved.
     void moveChildrenTo(RenderBoxModelObject* toBoxModelObject, RenderObject* startChild, RenderObject* endChild, NormalizeAfterInsertion normalizeAfterInsertion)
index 2e48d51..f48aa1f 100644 (file)
@@ -42,7 +42,6 @@ public:
     bool canBeSelectionLeaf() const override;
 
     RenderPtr<RenderObject> takeChild(RenderTreeBuilder&, RenderObject&) override;
-    void removeLeftoverAnonymousBlock(RenderBlock*) override { }
     bool createsAnonymousWrapper() const override { return true; }
 
     void updateFromElement() override;
index de43679..3cb609e 100644 (file)
@@ -499,6 +499,51 @@ void RenderElement::destroyLeftoverChildren()
     }
 }
 
+RenderObject* RenderElement::attachRendererInternal(RenderPtr<RenderObject> child, RenderObject* beforeChild)
+{
+    child->setParent(this);
+
+    if (m_firstChild == beforeChild)
+        m_firstChild = child.get();
+
+    if (beforeChild) {
+        auto* previousSibling = beforeChild->previousSibling();
+        if (previousSibling)
+            previousSibling->setNextSibling(child.get());
+        child->setPreviousSibling(previousSibling);
+        child->setNextSibling(beforeChild);
+        beforeChild->setPreviousSibling(child.get());
+        return child.release();
+    }
+    if (m_lastChild)
+        m_lastChild->setNextSibling(child.get());
+    child->setPreviousSibling(m_lastChild);
+    m_lastChild = child.get();
+    return child.release();
+}
+
+RenderPtr<RenderObject> RenderElement::detachRendererInternal(RenderObject& renderer)
+{
+    auto* parent = renderer.parent();
+    ASSERT(parent);
+    auto* nextSibling = renderer.nextSibling();
+
+    if (renderer.previousSibling())
+        renderer.previousSibling()->setNextSibling(nextSibling);
+    if (nextSibling)
+        nextSibling->setPreviousSibling(renderer.previousSibling());
+
+    if (parent->firstChild() == &renderer)
+        parent->m_firstChild = nextSibling;
+    if (parent->lastChild() == &renderer)
+        parent->m_lastChild = renderer.previousSibling();
+
+    renderer.setPreviousSibling(nullptr);
+    renderer.setNextSibling(nullptr);
+    renderer.setParent(nullptr);
+    return RenderPtr<RenderObject>(&renderer);
+}
+
 void RenderElement::insertChildInternal(RenderPtr<RenderObject> newChildPtr, RenderObject* beforeChild)
 {
     RELEASE_ASSERT_WITH_MESSAGE(!view().frameView().layoutContext().layoutState(), "Layout must not mutate render tree");
@@ -514,26 +559,7 @@ void RenderElement::insertChildInternal(RenderPtr<RenderObject> newChildPtr, Ren
     ASSERT(!is<RenderText>(beforeChild) || !downcast<RenderText>(*beforeChild).inlineWrapperForDisplayContents());
 
     // Take the ownership.
-    auto* newChild = newChildPtr.release();
-
-    newChild->setParent(this);
-
-    if (m_firstChild == beforeChild)
-        m_firstChild = newChild;
-
-    if (beforeChild) {
-        RenderObject* previousSibling = beforeChild->previousSibling();
-        if (previousSibling)
-            previousSibling->setNextSibling(newChild);
-        newChild->setPreviousSibling(previousSibling);
-        newChild->setNextSibling(beforeChild);
-        beforeChild->setPreviousSibling(newChild);
-    } else {
-        if (lastChild())
-            lastChild()->setNextSibling(newChild);
-        newChild->setPreviousSibling(lastChild());
-        m_lastChild = newChild;
-    }
+    auto* newChild = attachRendererInternal(WTFMove(newChildPtr), beforeChild);
 
     newChild->initializeFragmentedFlowStateOnInsertion();
     if (!renderTreeBeingDestroyed()) {
@@ -599,34 +625,19 @@ RenderPtr<RenderObject> RenderElement::takeChildInternal(RenderObject& oldChild)
     // WARNING: There should be no code running between willBeRemovedFromTree and the actual removal below.
     // This is needed to avoid race conditions where willBeRemovedFromTree would dirty the tree's structure
     // and the code running here would force an untimely rebuilding, leaving |oldChild| dangling.
-    
-    RenderObject* nextSibling = oldChild.nextSibling();
-
-    if (oldChild.previousSibling())
-        oldChild.previousSibling()->setNextSibling(nextSibling);
-    if (nextSibling)
-        nextSibling->setPreviousSibling(oldChild.previousSibling());
-
-    if (m_firstChild == &oldChild)
-        m_firstChild = nextSibling;
-    if (m_lastChild == &oldChild)
-        m_lastChild = oldChild.previousSibling();
-
-    oldChild.setPreviousSibling(nullptr);
-    oldChild.setNextSibling(nullptr);
-    oldChild.setParent(nullptr);
+    auto childToTake = detachRendererInternal(oldChild);
 
     // rendererRemovedFromTree walks the whole subtree. We can improve performance
     // by skipping this step when destroying the entire tree.
-    if (!renderTreeBeingDestroyed() && is<RenderElement>(oldChild))
-        RenderCounter::rendererRemovedFromTree(downcast<RenderElement>(oldChild));
+    if (!renderTreeBeingDestroyed() && is<RenderElement>(*childToTake))
+        RenderCounter::rendererRemovedFromTree(downcast<RenderElement>(*childToTake));
 
     if (!renderTreeBeingDestroyed()) {
         if (AXObjectCache* cache = document().existingAXObjectCache())
             cache->childrenChanged(this);
     }
 
-    return RenderPtr<RenderObject>(&oldChild);
+    return childToTake;
 }
 
 RenderBlock* RenderElement::containingBlockForFixedPosition() const
index 4466d01..3269ef2 100644 (file)
@@ -229,6 +229,8 @@ public:
     void setIsFirstLetter() { m_isFirstLetter = true; }
 
     void destroyLeftoverChildren();
+    RenderObject* attachRendererInternal(RenderPtr<RenderObject> child, RenderObject* beforeChild);
+    RenderPtr<RenderObject> detachRendererInternal(RenderObject&);
 
 protected:
     enum BaseTypeFlag {
index cdec13a..6fa63c3 100644 (file)
@@ -85,7 +85,6 @@ private:
     bool isRubyBlock() const final { return true; }
     const char* renderName() const override { return "RenderRuby (block)"; }
     bool createsAnonymousWrapper() const override { return true; }
-    void removeLeftoverAnonymousBlock(RenderBlock*) override { ASSERT_NOT_REACHED(); }
 };
 
 
index 71d98ea..043050c 100644 (file)
@@ -78,7 +78,6 @@ private:
     bool isRubyRun() const override { return true; }
     const char* renderName() const override { return "RenderRubyRun (anonymous)"; }
     bool createsAnonymousWrapper() const override { return true; }
-    void removeLeftoverAnonymousBlock(RenderBlock*) override { }
 
 private:
     UChar m_lastCharacter;
index a92e5fc..f8565dd 100644 (file)
@@ -74,7 +74,6 @@ private:
     bool isTextControl() const final { return true; }
     void computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const override;
     void computePreferredLogicalWidths() override;
-    void removeLeftoverAnonymousBlock(RenderBlock*) override { }
     bool avoidsFloats() const override { return true; }
     bool canHaveGeneratedChildren() const override { return false; }
     
index 13b7cbe..76cacf0 100644 (file)
 #include "config.h"
 #include "RenderTreeBuilderBlock.h"
 
+#include "RenderButton.h"
 #include "RenderChildIterator.h"
 #include "RenderFullScreen.h"
+#include "RenderRuby.h"
+#include "RenderRubyRun.h"
+#include "RenderTextControl.h"
 
 namespace WebCore {
 
@@ -193,7 +197,7 @@ void RenderTreeBuilder::Block::insertChildIgnoringContinuation(RenderBlock& pare
     parent.RenderBox::addChild(m_builder, WTFMove(child), beforeChild);
 
     if (madeBoxesNonInline && is<RenderBlock>(parent.parent()) && parent.isAnonymousBlock())
-        downcast<RenderBlock>(*parent.parent()).removeLeftoverAnonymousBlock(&parent);
+        removeLeftoverAnonymousBlock(parent);
     // parent object may be dead here
 }
 
@@ -201,8 +205,27 @@ void RenderTreeBuilder::Block::childBecameNonInline(RenderBlock& parent, RenderE
 {
     m_builder.makeChildrenNonInline(parent);
     if (parent.isAnonymousBlock() && is<RenderBlock>(parent.parent()))
-        downcast<RenderBlock>(*parent.parent()).removeLeftoverAnonymousBlock(&parent);
+        removeLeftoverAnonymousBlock(parent);
     // parent may be dead here
 }
 
+void RenderTreeBuilder::Block::removeLeftoverAnonymousBlock(RenderBlock& anonymousBlock)
+{
+    ASSERT(anonymousBlock.isAnonymousBlock());
+    ASSERT(!anonymousBlock.childrenInline());
+    ASSERT(anonymousBlock.parent());
+
+    if (anonymousBlock.continuation())
+        return;
+
+    auto* parent = anonymousBlock.parent();
+    if (is<RenderButton>(*parent) || is<RenderTextControl>(*parent) || is<RenderRubyAsBlock>(*parent) || is<RenderRubyRun>(*parent))
+        return;
+
+    // FIXME: This should really just be a moveAllChilrenTo (see webkit.org/b/182495)
+    anonymousBlock.moveAllChildrenToInternal(*parent);
+    auto toBeDestroyed = parent->takeChildInternal(anonymousBlock);
+    // anonymousBlock is dead here.
+}
+
 }
index 084b359..fc4ad70 100644 (file)
@@ -40,6 +40,7 @@ public:
 
 private:
     void insertChildToContinuation(RenderBlock& parent, RenderPtr<RenderObject> child, RenderObject* beforeChild);
+    void removeLeftoverAnonymousBlock(RenderBlock& anonymousBlock);
 
     RenderTreeBuilder& m_builder;
 };