Call Element::rebuildLayoutTree from Document::updateStyle directly

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Peter Kelly (pmk@post.com)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2007 David Smith (catfish.man@gmail.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012, 2013 Apple Inc.
  * All rights reserved.
  *           (C) 2007 Eric Seidel (eric@webkit.org)
  *
@@ skipping 1866 matching lines @@
 }
 
 PassRefPtr<ComputedStyle> Element::originalStyleForLayoutObject() {
   DCHECK(document().inStyleRecalc());
   return document().ensureStyleResolver().styleForElement(this);
 }
 
 void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
   DCHECK(document().inStyleRecalc());
   DCHECK(!document().lifecycle().inDetach());
-  DCHECK(!parentOrShadowHostNode()->needsStyleRecalc());

[esprehn, 2017/02/16 05:49:25]

ditto, entering into here with a dirty style is a serious bug, it means you're
inherting from a dirty parent.

[nainar, 2017/02/16 06:47:54]

This isn't true anymore in case we need this information in
reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is
false. 

[esprehn, 2017/02/16 06:54:12]

Hmm? We should have cleared all the bits by calling attach() which then computes
the style in order. Can you give a short example?

[nainar, 2017/02/16 07:13:20]

Test case:

<div style="display:none">
  <input type="image" id="image" list="dl1">
</div>

   DCHECK(inActiveDocument());
 
   if (hasCustomStyleCallbacks())
     willRecalcStyle(change);
 
   if (change >= IndependentInherit || needsStyleRecalc()) {
     if (hasRareData()) {
       ElementRareData* data = elementRareData();
       if (change != IndependentInherit)
         data->clearComputedStyle();
 
       if (change >= IndependentInherit) {
         if (ElementAnimations* elementAnimations = data->elementAnimations())
           elementAnimations->setAnimationStyleChange(false);
       }
     }
-    if (parentComputedStyle())
+    if (parentComputedStyle()) {
       change = recalcOwnStyle(change, nextTextSibling);
-    clearNeedsStyleRecalc();
-    clearNeedsReattachLayoutTree();
+    } else {
+      // In case we don't perform recalcOwnStyle we will never clear the
+      // NeedsReattachLayoutTree flag which is set on the creation of each
+      // Node. Clear that here.
+      clearNeedsReattachLayoutTree();

[esprehn, 2017/02/16 05:49:25]

why would this be true? We should still follow the childNeeds bits in the next
phase and eventually get here and clear it? The entry won't be in the map
though.

How do we get here with the NeedsReattachLayoutTree bit set, but not the
ancestor bits set? I don't think you need this code.

[nainar, 2017/02/16 06:47:54]

Done.

+    }
   }
 
-  // If we reattached we don't need to recalc the style of our descendants
-  // anymore.
-  if ((change >= UpdatePseudoElements && change < Reattach) ||
-      childNeedsStyleRecalc()) {
+  // If we are going to reattach we don't need to recalc the style of
+  // our descendants anymore.
+  if (change < Reattach &&
+      (change >= UpdatePseudoElements || childNeedsStyleRecalc())) {
     SelectorFilterParentScope filterScope(*this);
     StyleSharingDepthScope sharingScope(*this);
 
     updatePseudoElement(PseudoIdBefore, change);
 
     if (change > UpdatePseudoElements || childNeedsStyleRecalc()) {
       for (ShadowRoot* root = youngestShadowRoot(); root;
            root = root->olderShadowRoot()) {
         if (root->shouldCallRecalcStyle(change))
           root->recalcStyle(change);
       }
       recalcDescendantStyles(change);
     }
 
     updatePseudoElement(PseudoIdAfter, change);
     updatePseudoElement(PseudoIdBackdrop, change);
 
     // If our children have changed then we need to force the first-letter
     // checks as we don't know if they effected the first letter or not.
     // This can be seen when a child transitions from floating to
     // non-floating we have to take it into account for the first letter.
     updatePseudoElement(PseudoIdFirstLetter,
                         childNeedsStyleRecalc() ? Force : change);
 
     clearChildNeedsStyleRecalc();
-    clearChildNeedsReattachLayoutTree();
   }
 
+  if (!needsReattachLayoutTree() && !childNeedsReattachLayoutTree())
+    clearNeedsStyleRecalc();
+
   if (hasCustomStyleCallbacks())
     didRecalcStyle();
 }
 
 PassRefPtr<ComputedStyle> Element::propagateInheritedProperties(
     StyleRecalcChange change) {
   if (change != IndependentInherit)
     return nullptr;
   if (isPseudoElement())
     return nullptr;
   if (needsStyleRecalc())
     return nullptr;
   if (hasAnimations())
     return nullptr;
   const ComputedStyle* parentStyle = parentComputedStyle();
   DCHECK(parentStyle);
   const ComputedStyle* style = computedStyle();
   if (!style || style->animations() || style->transitions())
     return nullptr;
   RefPtr<ComputedStyle> newStyle = ComputedStyle::clone(*style);
   newStyle->propagateIndependentInheritedProperties(*parentStyle);
   INCREMENT_STYLE_STATS_COUNTER(document().styleEngine(),
                                 independentInheritedStylesPropagated, 1);
   return newStyle;
 }
 
 StyleRecalcChange Element::recalcOwnStyle(StyleRecalcChange change,
                                           Text* nextTextSibling) {
   DCHECK(document().inStyleRecalc());
-  DCHECK(!parentOrShadowHostNode()->needsStyleRecalc());

[esprehn, 2017/02/16 05:49:25]

Add this back, entering into recalcOwnStyle with a dirty style on the parent is
a bug.

[nainar, 2017/02/16 06:47:54]

This isn't true anymore in case we need this information in
reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is
false. 

[esprehn, 2017/02/16 06:54:12]

This should be true, we're only leaving the style bits behind on the attach
roots, and we should never traverse down into recalcOwnStyle on anything under
an attach root. Can you give a short markup example?

[nainar, 2017/02/16 07:13:20]

Same test case as above.

I see what you mean here though. Debugging this. 

   DCHECK(change >= IndependentInherit || needsStyleRecalc());
   DCHECK(parentComputedStyle());
 
   RefPtr<ComputedStyle> oldStyle = mutableComputedStyle();
 
   // When propagating inherited changes, we don't need to do a full style recalc
   // if the only changed properties are independent. In this case, we can simply
   // set these directly on the ComputedStyle object.
   RefPtr<ComputedStyle> newStyle = propagateInheritedProperties(change);
   if (!newStyle)
     newStyle = styleForLayoutObject();
   DCHECK(newStyle);
 
   StyleRecalcChange localChange =
       ComputedStyle::stylePropagationDiff(oldStyle.get(), newStyle.get());
   if (localChange == NoChange) {
     INCREMENT_STYLE_STATS_COUNTER(document().styleEngine(), stylesUnchanged, 1);
   } else {
     INCREMENT_STYLE_STATS_COUNTER(document().styleEngine(), stylesChanged, 1);
   }
 
   if (localChange == Reattach) {
     StyleReattachData styleReattachData;
     styleReattachData.computedStyle = std::move(newStyle);
     styleReattachData.nextTextSibling = nextTextSibling;
     document().addStyleReattachData(*this, styleReattachData);
     setNeedsReattachLayoutTree();
-    return rebuildLayoutTree();
+    return Reattach;
   }
 
   DCHECK(oldStyle);
 
   if (localChange != NoChange)
     updateCallbackSelectors(oldStyle.get(), newStyle.get());
 
   if (LayoutObject* layoutObject = this->layoutObject()) {
     if (localChange != NoChange ||
         pseudoStyleCacheIsInvalid(oldStyle.get(), newStyle.get())) {
@@ skipping 20 matching lines @@
         return Inherit;
       newStyle->copyChildDependentFlagsFrom(*oldStyle);
     }
     if (oldStyle->hasPseudoElementStyle() || newStyle->hasPseudoElementStyle())
       return UpdatePseudoElements;
   }
 
   return localChange;
 }
 
-StyleRecalcChange Element::rebuildLayoutTree() {
+void Element::rebuildLayoutTree() {
   DCHECK(inActiveDocument());
+  DCHECK(parentNode());
+
   StyleReattachData styleReattachData = document().getStyleReattachData(*this);
   AttachContext reattachContext;
   reattachContext.resolvedStyle = styleReattachData.computedStyle.get();
   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.
+  if (needsReattachLayoutTree()) {
+    reattachLayoutTree(reattachContext);
+  } else if (childNeedsReattachLayoutTree()) {
+    DCHECK(!needsReattachLayoutTree());
+    SelectorFilterParentScope filterScope(*this);
+    StyleSharingDepthScope sharingScope(*this);
+    reattachPseudoElementLayoutTree(PseudoIdBefore);
+    rebuildShadowRootLayoutTree();
+    rebuildChildrenLayoutTrees();
+    reattachPseudoElementLayoutTree(PseudoIdAfter);
+    reattachPseudoElementLayoutTree(PseudoIdBackdrop);
+    reattachPseudoElementLayoutTree(PseudoIdFirstLetter);
+    DCHECK(!needsStyleRecalc());

[esprehn, 2017/02/16 05:49:25]

remove, you check it below

[nainar, 2017/02/16 06:47:54]

Done.

+  }
+  DCHECK(!needsStyleRecalc());
+  DCHECK(!childNeedsStyleRecalc());
+  DCHECK(!needsReattachLayoutTree());
   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(styleReattachData.nextTextSibling);
-    return Reattach;
+  } else {
+    clearNeedsStyleRecalc();

[esprehn, 2017/02/16 05:49:25]

You DCHECK(!needsStyleRecalc()); right above this, so this code can be removed

[nainar, 2017/02/16 06:47:54]

Done.

   }
-  return ReattachNoLayoutObject;
+}
+
+void Element::rebuildShadowRootLayoutTree() {
+  for (ShadowRoot* root = youngestShadowRoot(); root;
+       root = root->olderShadowRoot()) {
+    if (root->needsReattachLayoutTree() || root->childNeedsReattachLayoutTree())
+      root->rebuildLayoutTree();
+  }
+}
+
+void Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) {
+  if (PseudoElement* element = pseudoElement(pseudoId)) {
+    if (element->needsReattachLayoutTree() ||
+        element->childNeedsReattachLayoutTree())
+      element->rebuildLayoutTree();
+  } else {
+    createPseudoElementIfNeeded(pseudoId);
+  }
 }
 
 void Element::updateCallbackSelectors(const ComputedStyle* oldStyle,
                                       const ComputedStyle* newStyle) {
   Vector<String> emptyVector;
   const Vector<String>& oldCallbackSelectors =
       oldStyle ? oldStyle->callbackSelectors() : emptyVector;
   const Vector<String>& newCallbackSelectors =
       newStyle ? newStyle->callbackSelectors() : emptyVector;
   if (oldCallbackSelectors.isEmpty() && newCallbackSelectors.isEmpty())
@@ skipping 1129 matching lines @@
 Locale& Element::locale() const {
   return document().getCachedLocale(computeInheritedLanguage());
 }
 
 void Element::cancelFocusAppearanceUpdate() {
   if (document().focusedElement() == this)
     document().cancelFocusAppearanceUpdate();
 }
 
 void Element::updatePseudoElement(PseudoId pseudoId, StyleRecalcChange change) {
-  DCHECK(!needsStyleRecalc());
   PseudoElement* element = pseudoElement(pseudoId);
 
   if (element && (change == UpdatePseudoElements ||
                   element->shouldCallRecalcStyle(change))) {
     if (pseudoId == PseudoIdFirstLetter && updateFirstLetter(element))
       return;
 
     // Need to clear the cached style if the PseudoElement wants a recalc so it
     // computes a new style.
     if (element->needsStyleRecalc())
@@ skipping 906 matching lines @@
 }
 
 DEFINE_TRACE_WRAPPERS(Element) {
   if (hasRareData()) {
     visitor->traceWrappers(elementRareData());
   }
   ContainerNode::traceWrappers(visitor);
 }
 
 }  // namespace blink