Move Layout Tree Construction code into Element::rebuildLayoutTree()

third_party/WebKit/Source/core/dom/Element.cpp

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 047e339cba3e817a5c7eb056291cb3d2776f7584..94bfed791c949f6e41b01052b41b80d54860c74a 100644
--- a/third_party/WebKit/Source/core/dom/Element.cpp
+++ b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -1860,6 +1860,7 @@ void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
     if (parentComputedStyle())
       change = recalcOwnStyle(change);
     clearNeedsStyleRecalc();
+    clearNeedsReattachLayoutTree();
   }
 
   // If we reattached we don't need to recalc the style of our descendants
@@ -1891,13 +1892,11 @@ void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
                         childNeedsStyleRecalc() ? Force : change);
 
     clearChildNeedsStyleRecalc();
+    clearChildNeedsReattachLayoutTree();
   }
 
   if (hasCustomStyleCallbacks())
     didRecalcStyle(change);
-
-  if (change == Reattach)
-    reattachWhitespaceSiblingsIfNeeded(nextTextSibling);
 }
 
 PassRefPtr<ComputedStyle> Element::propagateInheritedProperties(
@@ -1946,6 +1945,7 @@ StyleRecalcChange Element::recalcOwnStyle(StyleRecalcChange change) {
 
   if (localChange == Reattach) {
     document().addNonAttachedStyle(*this, std::move(newStyle));
+    setNeedsReattachLayoutTree();
     return rebuildLayoutTree();
   }
 
@@ -1992,9 +1992,26 @@ StyleRecalcChange Element::rebuildLayoutTree() {
   AttachContext reattachContext;
   reattachContext.resolvedStyle = document().getNonAttachedStyle(*this);
   bool layoutObjectWillChange = needsAttach() || layoutObject();
-  reattachLayoutTree(reattachContext);
-  if (layoutObjectWillChange || layoutObject())
+
+  if (needsReattachLayoutTree())

[Bugs Nash, 2016/10/17 00:17:05]

what is the purpose of this check? it doesn't seem necessary at this point

[nainar, 2016/10/17 16:29:12]

Moved to a DCHECK and added an explainer comment here as well. 

+    reattachLayoutTree(reattachContext);
+
+  DCHECK(!childNeedsReattachLayoutTree());

[Bugs Nash, 2016/10/17 00:17:05]

add a comment explaining why we expect this DCHECK to be true (i.e.
reattachLayoutTree handles the children)

[nainar, 2016/10/17 16:29:12]

Done.

+
+  if (layoutObjectWillChange || layoutObject()) {
+    Text* nextTextSibling = nullptr;

[Bugs Nash, 2016/10/17 00:17:04]

the comment about the trade off between storage and calculation was good, I just
meant to remove the part about webkit.

I think this would be better separated into its own method nextTextSibling(),
possibly on Node, which can return null if the next sibling is not an Element or
doesn't have a layout object. In future once Text mirrors Element this could
even be called from within reattachWhitespaceSiblings itself instead of being
passed here.

[nainar, 2016/10/17 16:29:12]

Edited the comment. 

Moved code to function. 

+    for (Node* sibling = this->nextSibling();
+         sibling &&
+         !(sibling->isElementNode() && toElement(sibling)->layoutObject());
+         sibling = sibling->nextSibling()) {
+      if (sibling->isTextNode()) {
+        nextTextSibling = toText(sibling);
+        break;
+      }
+    }
+    reattachWhitespaceSiblingsIfNeeded(nextTextSibling);
     return Reattach;
+  }
   return ReattachNoLayoutObject;
 }