Bug 1357461: Ensure not to increment the restyle generation if we haven't restyled after all. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 02 Jun 2017 21:27:41 +0200
changeset 410309 e0477439b222422a105c9e108878fadf1f36354b
parent 410308 54e9145b8892d636175b488b0eddc9a480fd8443
child 410310 6ddc4c39388edbae82fd505fb8a76c2a6c63b786
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1357461
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1357461: Ensure not to increment the restyle generation if we haven't restyled after all. r=heycam 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");