From f47466119ddc438af7c1d6a091821155fe677468 Mon Sep 17 00:00:00 2001 From: Alan Bujtas Date: Mon, 19 Feb 2018 11:41:49 +0000 Subject: [PATCH] Merge r228224 - [RenderTreeBuilder] Move RenderBlock::removeLeftoverAnonymousBlock to RenderTreeBuilder https://bugs.webkit.org/show_bug.cgi?id=182510 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: --- Source/WebCore/ChangeLog | 34 +++++++ Source/WebCore/rendering/RenderBlock.cpp | 52 ----------- Source/WebCore/rendering/RenderBlock.h | 1 - .../rendering/RenderBoxModelObject.cpp | 6 ++ .../WebCore/rendering/RenderBoxModelObject.h | 1 + Source/WebCore/rendering/RenderButton.h | 1 - Source/WebCore/rendering/RenderElement.cpp | 89 +++++++++++-------- Source/WebCore/rendering/RenderElement.h | 2 + Source/WebCore/rendering/RenderRuby.h | 1 - Source/WebCore/rendering/RenderRubyRun.h | 1 - Source/WebCore/rendering/RenderTextControl.h | 1 - .../updating/RenderTreeBuilderBlock.cpp | 27 +++++- .../updating/RenderTreeBuilderBlock.h | 1 + 13 files changed, 119 insertions(+), 98 deletions(-) diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index a8d12fb18d2f..cc704df1f5ea 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,37 @@ +2018-02-07 Zalan Bujtas + + [RenderTreeBuilder] Move RenderBlock::removeLeftoverAnonymousBlock to RenderTreeBuilder + https://bugs.webkit.org/show_bug.cgi?id=182510 + + + 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 Remove WebCore/ForwardingHeaders directory diff --git a/Source/WebCore/rendering/RenderBlock.cpp b/Source/WebCore/rendering/RenderBlock.cpp index bdff483a3bb7..267f4b66f990 100644 --- a/Source/WebCore/rendering/RenderBlock.cpp +++ b/Source/WebCore/rendering/RenderBlock.cpp @@ -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()) diff --git a/Source/WebCore/rendering/RenderBlock.h b/Source/WebCore/rendering/RenderBlock.h index 3c8f139b9928..93fc78f1f20b 100644 --- a/Source/WebCore/rendering/RenderBlock.h +++ b/Source/WebCore/rendering/RenderBlock.h @@ -396,7 +396,6 @@ class RenderBlock : public RenderBox { void adjustBorderBoxRectForPainting(LayoutRect&) override; LayoutRect paintRectToClipOutFromBorder(const LayoutRect&) override; void addChildIgnoringContinuation(RenderTreeBuilder&, RenderPtr newChild, RenderObject* beforeChild) override; - virtual void removeLeftoverAnonymousBlock(RenderBlock* child); bool isInlineBlockOrInlineTable() const final { return isInline() && isReplaced(); } protected: diff --git a/Source/WebCore/rendering/RenderBoxModelObject.cpp b/Source/WebCore/rendering/RenderBoxModelObject.cpp index e4f588605d5f..c19824484b1a 100644 --- a/Source/WebCore/rendering/RenderBoxModelObject.cpp +++ b/Source/WebCore/rendering/RenderBoxModelObject.cpp @@ -2742,4 +2742,10 @@ void RenderBoxModelObject::moveChildrenTo(RenderBoxModelObject* toBoxModelObject } } +void RenderBoxModelObject::moveAllChildrenToInternal(RenderElement& newParent) +{ + while (firstChild()) + newParent.attachRendererInternal(detachRendererInternal(*firstChild()), this); +} + } // namespace WebCore diff --git a/Source/WebCore/rendering/RenderBoxModelObject.h b/Source/WebCore/rendering/RenderBoxModelObject.h index 27f33d5ed542..74c43a265a16 100644 --- a/Source/WebCore/rendering/RenderBoxModelObject.h +++ b/Source/WebCore/rendering/RenderBoxModelObject.h @@ -290,6 +290,7 @@ class RenderBoxModelObject : public RenderLayerModelObject { { 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) diff --git a/Source/WebCore/rendering/RenderButton.h b/Source/WebCore/rendering/RenderButton.h index 2e48d512baf2..f48aa1f71855 100644 --- a/Source/WebCore/rendering/RenderButton.h +++ b/Source/WebCore/rendering/RenderButton.h @@ -42,7 +42,6 @@ class RenderButton final : public RenderFlexibleBox { bool canBeSelectionLeaf() const override; RenderPtr takeChild(RenderTreeBuilder&, RenderObject&) override; - void removeLeftoverAnonymousBlock(RenderBlock*) override { } bool createsAnonymousWrapper() const override { return true; } void updateFromElement() override; diff --git a/Source/WebCore/rendering/RenderElement.cpp b/Source/WebCore/rendering/RenderElement.cpp index de43679dda20..3cb609e7bec9 100644 --- a/Source/WebCore/rendering/RenderElement.cpp +++ b/Source/WebCore/rendering/RenderElement.cpp @@ -499,6 +499,51 @@ void RenderElement::destroyLeftoverChildren() } } +RenderObject* RenderElement::attachRendererInternal(RenderPtr 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 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(&renderer); +} + void RenderElement::insertChildInternal(RenderPtr 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 newChildPtr, Ren ASSERT(!is(beforeChild) || !downcast(*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 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(oldChild)) - RenderCounter::rendererRemovedFromTree(downcast(oldChild)); + if (!renderTreeBeingDestroyed() && is(*childToTake)) + RenderCounter::rendererRemovedFromTree(downcast(*childToTake)); if (!renderTreeBeingDestroyed()) { if (AXObjectCache* cache = document().existingAXObjectCache()) cache->childrenChanged(this); } - return RenderPtr(&oldChild); + return childToTake; } RenderBlock* RenderElement::containingBlockForFixedPosition() const diff --git a/Source/WebCore/rendering/RenderElement.h b/Source/WebCore/rendering/RenderElement.h index 4466d01b92a2..3269ef2a5f2f 100644 --- a/Source/WebCore/rendering/RenderElement.h +++ b/Source/WebCore/rendering/RenderElement.h @@ -229,6 +229,8 @@ class RenderElement : public RenderObject { void setIsFirstLetter() { m_isFirstLetter = true; } void destroyLeftoverChildren(); + RenderObject* attachRendererInternal(RenderPtr child, RenderObject* beforeChild); + RenderPtr detachRendererInternal(RenderObject&); protected: enum BaseTypeFlag { diff --git a/Source/WebCore/rendering/RenderRuby.h b/Source/WebCore/rendering/RenderRuby.h index cdec13a558e0..6fa63c34281b 100644 --- a/Source/WebCore/rendering/RenderRuby.h +++ b/Source/WebCore/rendering/RenderRuby.h @@ -85,7 +85,6 @@ class RenderRubyAsBlock final : public RenderBlockFlow { 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(); } }; diff --git a/Source/WebCore/rendering/RenderRubyRun.h b/Source/WebCore/rendering/RenderRubyRun.h index 71d98ea3927d..043050cf56e6 100644 --- a/Source/WebCore/rendering/RenderRubyRun.h +++ b/Source/WebCore/rendering/RenderRubyRun.h @@ -78,7 +78,6 @@ class RenderRubyRun final : public RenderBlockFlow { 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; diff --git a/Source/WebCore/rendering/RenderTextControl.h b/Source/WebCore/rendering/RenderTextControl.h index a92e5fc77a0b..f8565dd9d026 100644 --- a/Source/WebCore/rendering/RenderTextControl.h +++ b/Source/WebCore/rendering/RenderTextControl.h @@ -74,7 +74,6 @@ class RenderTextControl : public RenderBlockFlow { 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; } diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp b/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp index 13b7cbe504c7..76cacf017bfd 100644 --- a/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp +++ b/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.cpp @@ -26,8 +26,12 @@ #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(parent.parent()) && parent.isAnonymousBlock()) - downcast(*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(parent.parent())) - downcast(*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(*parent) || is(*parent) || is(*parent) || is(*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. +} + } diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h b/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h index 084b35997a39..fc4ad70df6c9 100644 --- a/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h +++ b/Source/WebCore/rendering/updating/RenderTreeBuilderBlock.h @@ -40,6 +40,7 @@ class RenderTreeBuilder::Block { private: void insertChildToContinuation(RenderBlock& parent, RenderPtr child, RenderObject* beforeChild); + void removeLeftoverAnonymousBlock(RenderBlock& anonymousBlock); RenderTreeBuilder& m_builder; };