Revert of Move Layout Tree Construction code into Element::rebuildLayoutTree() (issue 2439973005 by nainar@chromium.org)

[nai...@chromium.org]

Reviewers: esprehn, Bugs Nash
CL: https://codereview.chromium.org/2439973005/

Message:
Created Revert of Move Layout Tree Construction code into
Element::rebuildLayoutTree()

Description:
Revert of Move Layout Tree Construction code into Element::rebuildLayoutTree()
(patchset #12 id:240001 of https://codereview.chromium.org/2398293003/ )

Reason for revert:
Suspected to cause crbug.com/658322.

Original issue's description:
> Move Layout Tree Construction code into Element::rebuildLayoutTree()
>
> This patch uses the two dirty bits (NeedsReattachLayoutTree and
> ChildNeedsReattachLayoutTree) on Node and adds the relevant
> getters/setters for them.
>
> It makes rebuildLayoutTree() public to be accessed in Document.cpp.
>
> It also moves reattachWhitespaceSiblingsIfNeeded() to rebuildLayoutTree() and
> uses dirtyBits to call either reattachLayoutTree() on node itself or call
> rebuildLayoutTree() on children nodes.
>
> Also added some comments explaining some design decisions. Will be removed
> upon completion of separation.
>
> BUG=595137
>
> Committed: https://crrev.com/09b7a8cf0647abca732b3718987a629d12a67fee
> Cr-Commit-Position: refs/heads/master@{#426353}

TBR=esp...@chromium.org,bugs...@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=595137

Affected files (+9, -80 lines):
M third_party/WebKit/Source/core/dom/ContainerNode.cpp
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/core/dom/Element.h
M third_party/WebKit/Source/core/dom/Element.cpp
M third_party/WebKit/Source/core/dom/Node.h
M third_party/WebKit/Source/core/dom/Node.cpp
M third_party/WebKit/Source/core/dom/Text.cpp
M third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
M third_party/WebKit/Source/core/html/HTMLFrameSetElement.cpp


Index: third_party/WebKit/Source/core/dom/ContainerNode.cpp
diff --git a/third_party/WebKit/Source/core/dom/ContainerNode.cpp b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
index 330a6027b22b378232695d59bc56da084a9aadbc..2bda5cdbf441a5e23692f3e6f0a5aa4eaed21e93 100644
--- a/third_party/WebKit/Source/core/dom/ContainerNode.cpp
+++ b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
@@ -767,7 +767,6 @@
}

clearChildNeedsStyleRecalc();
- clearChildNeedsReattachLayoutTree();
Node::attachLayoutTree(context);
}

@@ -1287,7 +1286,7 @@
} else if (child->isElementNode()) {
Element* element = toElement(child);
if (element->shouldCallRecalcStyle(change))
- element->recalcStyle(change);
+ element->recalcStyle(change, lastTextNode);
else if (element->supportsStyleSharing())
styleResolver.addToStyleSharingList(*element);
if (element->layoutObject())
Index: third_party/WebKit/Source/core/dom/Document.cpp
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index 14fc4bb837fa35232739c6d17403adce4e3c4531..11f5f6195d2fef8f83d6e1439ef6cbdc663640f5 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -1778,8 +1778,6 @@
continue;
DCHECK(!node.needsStyleRecalc());
DCHECK(!node.childNeedsStyleRecalc());
- DCHECK(!node.needsReattachLayoutTree());
- DCHECK(!node.childNeedsReattachLayoutTree());
DCHECK(!node.childNeedsDistributionRecalc());
DCHECK(!node.needsStyleInvalidation());
DCHECK(!node.childNeedsStyleInvalidation());
@@ -1912,7 +1910,6 @@
}

clearNeedsStyleRecalc();
- clearNeedsReattachLayoutTree();

StyleResolver& resolver = ensureStyleResolver();

@@ -1935,7 +1932,6 @@
// LayoutTreeConstruction.
m_nonAttachedStyle.clear();
clearChildNeedsStyleRecalc();
- clearChildNeedsReattachLayoutTree();

resolver.clearStyleSharingList();

@@ -1943,8 +1939,6 @@

DCHECK(!needsStyleRecalc());
DCHECK(!childNeedsStyleRecalc());
- DCHECK(!needsReattachLayoutTree());
- DCHECK(!childNeedsReattachLayoutTree());
DCHECK(inStyleRecalc());
DCHECK_EQ(styleResolver(), &resolver);
DCHECK(m_nonAttachedStyle.isEmpty());
Index: third_party/WebKit/Source/core/dom/Element.h
diff --git a/third_party/WebKit/Source/core/dom/Element.h b/third_party/WebKit/Source/core/dom/Element.h
index bf894d78d7eea157064b794058ce63c6e7e50d92..068800f1b0789d4a78b352b838efc1d2b9f8a88d 100644
--- a/third_party/WebKit/Source/core/dom/Element.h
+++ b/third_party/WebKit/Source/core/dom/Element.h
@@ -400,8 +400,7 @@

virtual LayoutObject* createLayoutObject(const ComputedStyle&);
virtual bool layoutObjectIsNeeded(const ComputedStyle&);
- void recalcStyle(StyleRecalcChange);
- StyleRecalcChange rebuildLayoutTree();
+ void recalcStyle(StyleRecalcChange, Text* nextTextSibling = nullptr);
void pseudoStateChanged(CSSSelector::PseudoType);
void setAnimationStyleChange(bool);
void clearAnimationStyleChange();
@@ -824,6 +823,8 @@
PassRefPtr<ComputedStyle> propagateInheritedProperties(StyleRecalcChange);

StyleRecalcChange recalcOwnStyle(StyleRecalcChange);
+ StyleRecalcChange rebuildLayoutTree();
+
inline void checkForEmptyStyleChange();

void updatePseudoElement(PseudoId, StyleRecalcChange);
Index: third_party/WebKit/Source/core/dom/Element.cpp
diff --git a/third_party/WebKit/Source/core/dom/Element.cpp b/third_party/WebKit/Source/core/dom/Element.cpp
index 4ccf579e028cb164d8c6d02b63b7a1736dfc647d..7a6688b1a524f0d35371390cd52e2ea488d15955 100644
--- a/third_party/WebKit/Source/core/dom/Element.cpp
+++ b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -1835,7 +1835,7 @@
return document().ensureStyleResolver().styleForElement(this);
}

-void Element::recalcStyle(StyleRecalcChange change) {
+void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
DCHECK(document().inStyleRecalc());
DCHECK(!document().lifecycle().inDetach());
DCHECK(!parentOrShadowHostNode()->needsStyleRecalc());
@@ -1858,7 +1858,6 @@
if (parentComputedStyle())
change = recalcOwnStyle(change);
clearNeedsStyleRecalc();
- clearNeedsReattachLayoutTree();
}

// If we reattached we don't need to recalc the style of our descendants
@@ -1890,11 +1889,13 @@
childNeedsStyleRecalc() ? Force : change);

clearChildNeedsStyleRecalc();
- clearChildNeedsReattachLayoutTree();
}

if (hasCustomStyleCallbacks())
didRecalcStyle(change);
+
+ if (change == Reattach)
+ reattachWhitespaceSiblingsIfNeeded(nextTextSibling);
}

PassRefPtr<ComputedStyle> Element::propagateInheritedProperties(
@@ -1943,7 +1944,6 @@

if (localChange == Reattach) {
document().addNonAttachedStyle(*this, std::move(newStyle));
- setNeedsReattachLayoutTree();
return rebuildLayoutTree();
}

@@ -1987,31 +1987,12 @@
}

StyleRecalcChange Element::rebuildLayoutTree() {
- DCHECK(inActiveDocument());
AttachContext reattachContext;
reattachContext.resolvedStyle = document().getNonAttachedStyle(*this);
bool layoutObjectWillChange = needsAttach() || layoutObject();
-
- // We are calling Element::rebuildLayoutTree() from inside
- // Element::recalcOwnStyle where we set the NeedsReattachLayoutTree
- // flag - so needsReattachLayoutTree() should always be true.
- DCHECK(parentNode());
- DCHECK(parentNode()->childNeedsReattachLayoutTree());
- DCHECK(needsReattachLayoutTree());
reattachLayoutTree(reattachContext);
- // Since needsReattachLayoutTree() is always true we go into
- // reattachLayoutTree() which reattaches all the descendant
- // sub-trees. At this point no child should need reattaching.
- DCHECK(!childNeedsReattachLayoutTree());
-
- if (layoutObjectWillChange || layoutObject()) {
- // nextTextSibling is passed on to recalcStyle from recalcDescendantStyles
- // we can either traverse the current subtree from this node onwards
- // or store it.
- // The choice is between increased time and increased memory complexity.
- reattachWhitespaceSiblingsIfNeeded(nextTextSibling());
+ if (layoutObjectWillChange || layoutObject())
return Reattach;
- }
return ReattachNoLayoutObject;
}

Index: third_party/WebKit/Source/core/dom/Node.h
diff --git a/third_party/WebKit/Source/core/dom/Node.h b/third_party/WebKit/Source/core/dom/Node.h
index 3770da8d8120b4b33c1c01aada97753cb17a88a4..6bfe36cbb01af071b8949d8083c07ab584ee41f8 100644
--- a/third_party/WebKit/Source/core/dom/Node.h
+++ b/third_party/WebKit/Source/core/dom/Node.h
@@ -208,7 +208,6 @@
Node* firstChild() const;
Node* lastChild() const;
Node* getRootNode(const GetRootNodeOptions&) const;
- Text* nextTextSibling() const;
Node& treeRoot() const;
Node& shadowIncludingRoot() const;
// closed-shadow-hidden is defined at
@@ -427,23 +426,6 @@

void setNeedsStyleRecalc(StyleChangeType, const StyleChangeReasonForTracing&);
void clearNeedsStyleRecalc();
-
- bool needsReattachLayoutTree() { return getFlag(NeedsReattachLayoutTree); }
- bool childNeedsReattachLayoutTree() {
- return getFlag(ChildNeedsReattachLayoutTree);
- }
-
- void setNeedsReattachLayoutTree();
- void setChildNeedsReattachLayoutTree() {
- setFlag(ChildNeedsReattachLayoutTree);
- }
-
- void clearNeedsReattachLayoutTree() { clearFlag(NeedsReattachLayoutTree); }
- void clearChildNeedsReattachLayoutTree() {
- clearFlag(ChildNeedsReattachLayoutTree);
- }
-
- void markAncestorsWithChildNeedsReattachLayoutTree();

bool needsDistributionRecalc() const;

Index: third_party/WebKit/Source/core/dom/Node.cpp
diff --git a/third_party/WebKit/Source/core/dom/Node.cpp b/third_party/WebKit/Source/core/dom/Node.cpp
index 85cbf75d0a25f6581df94ef12ecfcfd18a9d96a6..fdc20908685c44adee91624c3ce73ec23a9780ba 100644
--- a/third_party/WebKit/Source/core/dom/Node.cpp
+++ b/third_party/WebKit/Source/core/dom/Node.cpp
@@ -390,18 +390,6 @@
: &treeRoot();
}

-Text* Node::nextTextSibling() const {
- for (Node* sibling = nextSibling();
- sibling &&
- (!sibling->isElementNode() || !toElement(sibling)->layoutObject());
- sibling = sibling->nextSibling()) {
- if (sibling->isTextNode()) {
- return toText(sibling);
- }
- }
- return nullptr;
-}
-
Node* Node::insertBefore(Node* newChild,
Node* refChild,
ExceptionState& exceptionState) {
@@ -709,17 +697,6 @@
document().scheduleLayoutTreeUpdateIfNeeded();
}

-void Node::markAncestorsWithChildNeedsReattachLayoutTree() {
- for (ContainerNode* p = parentOrShadowHostNode();
- p && !p->childNeedsReattachLayoutTree(); p = p->parentOrShadowHostNode())
- p->setChildNeedsReattachLayoutTree();
-}
-
-void Node::setNeedsReattachLayoutTree() {
- setFlag(NeedsReattachLayoutTree);
- markAncestorsWithChildNeedsReattachLayoutTree();
-}
-
void Node::setNeedsStyleRecalc(StyleChangeType changeType,
const StyleChangeReasonForTracing& reason) {
DCHECK(changeType != NoStyleChange);
@@ -911,7 +888,6 @@
(layoutObject()->parent() || layoutObject()->isLayoutView())));

clearNeedsStyleRecalc();
- clearNeedsReattachLayoutTree();

if (AXObjectCache* cache = document().axObjectCache())
cache->updateCacheAfterNodeIsAttached(this);
Index: third_party/WebKit/Source/core/dom/Text.cpp
diff --git a/third_party/WebKit/Source/core/dom/Text.cpp b/third_party/WebKit/Source/core/dom/Text.cpp
index 4017f379c8cca22e0fe45e387d703b258ae3d24a..4280dffd70ba3ad33c9d62e06ca58f80f23848ce 100644
--- a/third_party/WebKit/Source/core/dom/Text.cpp
+++ b/third_party/WebKit/Source/core/dom/Text.cpp
@@ -400,7 +400,6 @@
reattachLayoutTree();
if (this->layoutObject())
reattachWhitespaceSiblingsIfNeeded(nextTextSibling);
- clearNeedsReattachLayoutTree();
}
}

Index: third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
diff --git a/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp b/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
index d632432b1a9ffa75bb34f22a24cc1f48b5d4c19e..d8ed185d0e233bfe29136164ea9ac7fd4522048d 100644
--- a/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
+++ b/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
@@ -129,11 +129,9 @@

// There's no style to update so just calling recalcStyle means we're updated.
clearNeedsStyleRecalc();
- clearNeedsReattachLayoutTree();

recalcDescendantStyles(change);
clearChildNeedsStyleRecalc();
- clearChildNeedsReattachLayoutTree();
}

void ShadowRoot::attachLayoutTree(const AttachContext& context) {
Index: third_party/WebKit/Source/core/html/HTMLFrameSetElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLFrameSetElement.cpp b/third_party/WebKit/Source/core/html/HTMLFrameSetElement.cpp
index 1250c5e61b78b1de35d0a9cc75413917f6025bf0..629aee64c45d29ffbaac275279676d33a9908000 100644
--- a/third_party/WebKit/Source/core/html/HTMLFrameSetElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLFrameSetElement.cpp
@@ -274,7 +274,6 @@
layoutObject()->setNeedsLayoutAndFullPaintInvalidation(
LayoutInvalidationReason::StyleChange);
clearNeedsStyleRecalc();
- clearNeedsReattachLayoutTree();
}
}

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2439973005/1


https://codereview.chromium.org/2439973005/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Try jobs failed on following builders:
mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/320550)

https://codereview.chromium.org/2439973005/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2439973005/200001


https://codereview.chromium.org/2439973005/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Committed patchset #2 (id:200001)

https://codereview.chromium.org/2439973005/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Patchset 2 (id:??) landed as
https://crrev.com/352b045421e6e2ceeba083eeebd27f64adf41194
Cr-Commit-Position: refs/heads/master@{#426991}

https://codereview.chromium.org/2439973005/