Bug 1357461: Ensure not to increment the restyle generation if we haven't restyled after all. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 02 Jun 2017 21:27:41 +0200
changeset 588669 8dca66e7727f5591e7c1335b9174f0b6b63c29ca
parent 588668 91cca93ea4156733f05ef61513a0e4f799896ba3
child 588670 724eb350a67d0b979fba06e859c8d65d829a052f
push id62106
push userbmo:emilio+bugs@crisal.io
push dateSat, 03 Jun 2017 16:59:53 +0000
bugs1357461
milestone55.0a1
Bug 1357461: Ensure not to increment the restyle generation if we haven't restyled after all. This can happen with content attribute or state changes that end up not generating any hint. In particular, in the media_queries_dynamic.html case, the iframe resize was toggling a few scrollbar attributes, which made us never pass the "didn't restyle" test, even though the test really passed. I'll probably need to add a workaround to assume we use viewport units, so probably won't pass for long. MozReview-Commit-ID: 2oEfic5yaOy
layout/base/ServoRestyleManager.cpp
layout/base/ServoRestyleManager.h
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -278,17 +278,17 @@ UpdateFramePseudoElementStyles(nsIFrame*
     UpdateBlockFramePseudoElements(static_cast<nsBlockFrame*>(aFrame),
                                    aStyleSet,
                                    aChangeList);
   }
 
   UpdateBackdropIfNeeded(aFrame, aStyleSet, aChangeList);
 }
 
-void
+bool
 ServoRestyleManager::ProcessPostTraversal(Element* aElement,
                                           nsStyleContext* aParentContext,
                                           ServoStyleSet* aStyleSet,
                                           nsStyleChangeList& aChangeList)
 {
   nsIFrame* styleFrame = nsLayoutUtils::GetStyleFrame(aElement);
 
   // Grab the change hint from Servo.
@@ -314,17 +314,17 @@ ServoRestyleManager::ProcessPostTraversa
 
   // If our change hint is reconstruct, we delegate to the frame constructor,
   // which consumes the new style and expects the old style to be on the frame.
   //
   // XXXbholley: We should teach the frame constructor how to clear the dirty
   // descendants bit to avoid the traversal here.
   if (changeHint & nsChangeHint_ReconstructFrame) {
     ClearRestyleStateFromSubtree(aElement);
-    return;
+    return true;
   }
 
   // TODO(emilio): We could avoid some refcount traffic here, specially in the
   // ServoComputedValues case, which uses atomic refcounting.
   //
   // Hold the old style context alive, because it could become a dangling
   // pointer during the replacement. In practice it's not a huge deal (on
   // GetNextContinuationWithSameStyle the pointer is not dereferenced, only
@@ -409,62 +409,70 @@ ServoRestyleManager::ProcessPostTraversa
     AddLayerChangesForAnimation(styleFrame, aElement, aChangeList);
   }
 
   const bool descendantsNeedFrames =
     aElement->HasFlag(NODE_DESCENDANTS_NEED_FRAMES);
   const bool traverseElementChildren =
     aElement->HasDirtyDescendantsForServo() || descendantsNeedFrames;
   const bool traverseTextChildren = recreateContext || descendantsNeedFrames;
+  bool recreatedAnyContext = recreateContext;
   if (traverseElementChildren || traverseTextChildren) {
     nsStyleContext* upToDateContext =
       recreateContext ? newContext : oldStyleContext;
 
     StyleChildrenIterator it(aElement);
     TextPostTraversalState textState(
         *upToDateContext, *aStyleSet, displayContentsNode && recreateContext);
     for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {
       if (traverseElementChildren && n->IsElement()) {
-        ProcessPostTraversal(n->AsElement(), upToDateContext,
-                             aStyleSet, aChangeList);
+        recreatedAnyContext |=
+          ProcessPostTraversal(n->AsElement(), upToDateContext,
+                               aStyleSet, aChangeList);
       } else if (traverseTextChildren && n->IsNodeOfType(nsINode::eTEXT)) {
-        ProcessPostTraversalForText(n, aChangeList, textState);
+        recreatedAnyContext |=
+          ProcessPostTraversalForText(n, aChangeList, textState);
       }
     }
   }
 
   aElement->UnsetHasDirtyDescendantsForServo();
   aElement->UnsetFlags(NODE_DESCENDANTS_NEED_FRAMES);
+  return recreatedAnyContext;
 }
 
-void
+bool
 ServoRestyleManager::ProcessPostTraversalForText(
     nsIContent* aTextNode,
     nsStyleChangeList& aChangeList,
     TextPostTraversalState& aPostTraversalState)
 {
   // Handle lazy frame construction.
   if (aTextNode->HasFlag(NODE_NEEDS_FRAME)) {
     aChangeList.AppendChange(nullptr, aTextNode, nsChangeHint_ReconstructFrame);
-    return;
+    return true;
   }
 
   // Handle restyle.
   nsIFrame* primaryFrame = aTextNode->GetPrimaryFrame();
-  if (primaryFrame) {
-    RefPtr<nsStyleContext> oldStyleContext = primaryFrame->StyleContext();
-    nsStyleContext& newContext = aPostTraversalState.ComputeStyle(aTextNode);
-    aPostTraversalState.ComputeHintIfNeeded(
-        aTextNode, primaryFrame, newContext, aChangeList);
+  if (!primaryFrame) {
+    return false;
+  }
 
-    for (nsIFrame* f = primaryFrame; f;
-         f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
-      f->SetStyleContext(&newContext);
-    }
+  RefPtr<nsStyleContext> oldStyleContext = primaryFrame->StyleContext();
+  nsStyleContext& newContext = aPostTraversalState.ComputeStyle(aTextNode);
+  aPostTraversalState.ComputeHintIfNeeded(
+      aTextNode, primaryFrame, newContext, aChangeList);
+
+  for (nsIFrame* f = primaryFrame; f;
+       f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
+    f->SetStyleContext(&newContext);
   }
+
+  return true;
 }
 
 void
 ServoRestyleManager::ClearSnapshots()
 {
   for (auto iter = mSnapshots.Iter(); !iter.Done(); iter.Next()) {
     iter.Key()->UnsetFlags(ELEMENT_HAS_SNAPSHOT | ELEMENT_HANDLED_SNAPSHOT);
     iter.Remove();
@@ -574,18 +582,20 @@ ServoRestyleManager::DoProcessPendingRes
     if (!animationOnly) {
       ClearSnapshots();
     }
 
     // Recreate style contexts, and queue up change hints (which also handle
     // lazy frame construction).
     nsStyleChangeList currentChanges(StyleBackendType::Servo);
     DocumentStyleRootIterator iter(doc);
+    bool anyStyleChanged = false;
     while (Element* root = iter.GetNextStyleRoot()) {
-      ProcessPostTraversal(root, nullptr, styleSet, currentChanges);
+      anyStyleChanged |=
+        ProcessPostTraversal(root, nullptr, styleSet, currentChanges);
     }
 
     // Process the change hints.
     //
     // Unfortunately, the frame constructor can generate new change hints while
     // processing existing ones. We redirect those into a secondary queue and
     // iterate until there's nothing left.
     ReentrantChangeList newChanges;
@@ -604,17 +614,28 @@ ServoRestyleManager::DoProcessPendingRes
         }
         currentChanges.AppendChange(change.mContent->GetPrimaryFrame(),
                                     change.mContent, change.mHint);
       }
       newChanges.Clear();
     }
     mReentrantChanges = nullptr;
 
-    IncrementRestyleGeneration();
+
+    if (anyStyleChanged) {
+      // Maybe no styles changed when:
+      //
+      //  * Only explicit change hints were posted in the first place.
+      //  * When an attribute or state change in the content happens not to need
+      //    a restyle after all.
+      //
+      // In any case, we don't need to increment the restyle generation in that
+      // case.
+      IncrementRestyleGeneration();
+    }
   }
 
   FlushOverflowChangedTracker();
 
   if (!animationOnly) {
     ClearSnapshots();
     styleSet->AssertTreeIsClean();
     mHaveNonAnimationRestyles = false;
--- a/layout/base/ServoRestyleManager.h
+++ b/layout/base/ServoRestyleManager.h
@@ -111,25 +111,31 @@ public:
 protected:
   ~ServoRestyleManager() override
   {
     MOZ_ASSERT(!mReentrantChanges);
   }
 
 private:
   /**
-   * Performs post-Servo-traversal processing on this element and its descendants.
+   * Performs post-Servo-traversal processing on this element and its
+   * descendants.
+   *
+   * Returns whether any style did actually change. There may be cases where we
+   * didn't need to change any style after all, for example, when a content
+   * attribute changes that happens not to have any effect on the style of that
+   * element or any descendant or sibling.
    */
-  void ProcessPostTraversal(Element* aElement,
+  bool ProcessPostTraversal(Element* aElement,
                             nsStyleContext* aParentContext,
                             ServoStyleSet* aStyleSet,
                             nsStyleChangeList& aChangeList);
 
   struct TextPostTraversalState;
-  void ProcessPostTraversalForText(nsIContent* aTextNode,
+  bool ProcessPostTraversalForText(nsIContent* aTextNode,
                                    nsStyleChangeList& aChangeList,
                                    TextPostTraversalState& aState);
 
   inline ServoStyleSet* StyleSet() const
   {
     MOZ_ASSERT(PresContext()->StyleSet()->IsServo(),
                "ServoRestyleManager should only be used with a Servo-flavored "
                "style backend");