Bug 1188721 - Part 7: Stop swapping structs between style contexts when restyling. r?dbaron draft
authorCameron McCormack <cam@mcc.id.au>
Wed, 23 Mar 2016 17:35:58 +1100
changeset 343757 47a48195b5fcce2859abe7aaad535cc38e5a54e6
parent 343756 c220e2fd6bfc01d666ac060e6cd4092455da35f6
child 343758 5068d2788518412f5502d9b5d057ecc39d762d94
push id13680
push usercmccormack@mozilla.com
push dateWed, 23 Mar 2016 06:36:18 +0000
reviewersdbaron
bugs1188721
milestone48.0a1
Bug 1188721 - Part 7: Stop swapping structs between style contexts when restyling. r?dbaron MozReview-Commit-ID: CySThmmVyVK
layout/base/RestyleManager.cpp
layout/base/RestyleManager.h
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -3244,34 +3244,30 @@ ElementRestyler::Restyle(nsRestyleHint a
   // eRestyle_ForceDescendants flag down too.
   nsRestyleHint childRestyleHint =
     nsRestyleHint(aRestyleHint & (eRestyle_SomeDescendants |
                                   eRestyle_Subtree |
                                   eRestyle_ForceDescendants));
 
   RefPtr<nsStyleContext> oldContext = mFrame->StyleContext();
 
-  nsTArray<SwapInstruction> swaps;
-
   // TEMPORARY (until bug 918064):  Call RestyleSelf for each
   // continuation or block-in-inline sibling.
 
   // We must make a single decision on how to process this frame and
   // its descendants, yet RestyleSelf might return different RestyleResult
   // values for the different same-style continuations.  |result| is our
   // overall decision.
   RestyleResult result = RestyleResult(0);
-  uint32_t swappedStructs = 0;
 
   nsRestyleHint thisRestyleHint = aRestyleHint;
 
   bool haveMoreContinuations = false;
   for (nsIFrame* f = mFrame; f; ) {
-    RestyleResult thisResult =
-      RestyleSelf(f, thisRestyleHint, &swappedStructs, swaps);
+    RestyleResult thisResult = RestyleSelf(f, thisRestyleHint);
 
     if (thisResult != eRestyleResult_Stop) {
       // Calls to RestyleSelf for later same-style continuations must not
       // return eRestyleResult_Stop, so pass eRestyle_Force in to them.
       thisRestyleHint = nsRestyleHint(thisRestyleHint | eRestyle_Force);
 
       if (result == eRestyleResult_Stop) {
         // We received eRestyleResult_Stop for earlier same-style
@@ -3357,19 +3353,16 @@ ElementRestyler::Restyle(nsRestyleHint a
     return;
   }
 
   if (result == eRestyleResult_StopWithStyleChange &&
       !(mHintsHandled & nsChangeHint_ReconstructFrame)) {
     MOZ_ASSERT(mFrame->StyleContext() != oldContext,
                "eRestyleResult_StopWithStyleChange should only be returned "
                "if we got a new style context or we will reconstruct");
-    MOZ_ASSERT(swappedStructs == 0,
-               "should have ensured we didn't swap structs when "
-               "returning eRestyleResult_StopWithStyleChange");
 
     // We need to ensure that all of the frames that inherit their style
     // from oldContext are able to be moved across to newContext.
     // MoveStyleContextsForChildren will check for certain conditions
     // to ensure it is safe to move all of the relevant child style
     // contexts to newContext.  If these conditions fail, it will
     // return false, and we'll have to continue restyling.
     const bool canStop = MoveStyleContextsForChildren(oldContext);
@@ -3383,35 +3376,19 @@ ElementRestyler::Restyle(nsRestyleHint a
       }
 
       mRestyleTracker.AddRestyleRootsIfAwaitingRestyle(descendants);
       if (aRestyleHint & eRestyle_SomeDescendants) {
         ConditionallyRestyleChildren();
       }
       return;
     }
-
-    // Turns out we couldn't stop restyling here.  Process the struct
-    // swaps that RestyleSelf would've done had we not returned
-    // eRestyleResult_StopWithStyleChange.
-    for (SwapInstruction& swap : swaps) {
-      LOG_RESTYLE("swapping style structs between %p and %p",
-                  swap.mOldContext.get(), swap.mNewContext.get());
-      swap.mOldContext->SwapStyleData(swap.mNewContext, swap.mStructsToSwap);
-      swappedStructs |= swap.mStructsToSwap;
-    }
-    swaps.Clear();
-  }
-
-  if (!swappedStructs) {
-    // If we swapped any structs from the old context, then we need to keep
-    // it alive until after the RestyleChildren call so that we can fix up
-    // its descendants' cached structs.
-    oldContext = nullptr;
-  }
+  }
+
+  oldContext = nullptr;
 
   if (result == eRestyleResult_ContinueAndForceDescendants) {
     childRestyleHint =
       nsRestyleHint(childRestyleHint | eRestyle_ForceDescendants);
   }
 
   // No need to do this if we're planning to reframe already.
   // It's also important to check mHintsHandled since we use
@@ -3437,17 +3414,17 @@ ElementRestyler::Restyle(nsRestyleHint a
     //
     // Also, we don't want this style context to get any more uses by being
     // returned from nsStyleContext::FindChildWithRules, so we add the
     // NS_STYLE_INELIGIBLE_FOR_SHARING bit to it.
     oldContext->SetIneligibleForSharing();
 
     ContextToClear* toClear = mContextsToClear.AppendElement();
     toClear->mStyleContext = Move(oldContext);
-    toClear->mStructs = swappedStructs;
+    toClear->mStructs = 0;
   }
 
   mRestyleTracker.AddRestyleRootsIfAwaitingRestyle(descendants);
 }
 
 /**
  * Depending on the details of the frame we are restyling or its old style
  * context, we may or may not be able to stop restyling after this frame if
@@ -3764,20 +3741,17 @@ CommonInheritedStyleData(nsRuleNode* aRu
     n1 = n1->GetParent();
     n2 = n2->GetParent();
   }
 
   return true;
 }
 
 ElementRestyler::RestyleResult
-ElementRestyler::RestyleSelf(nsIFrame* aSelf,
-                             nsRestyleHint aRestyleHint,
-                             uint32_t* aSwappedStructs,
-                             nsTArray<SwapInstruction>& aSwaps)
+ElementRestyler::RestyleSelf(nsIFrame* aSelf, nsRestyleHint aRestyleHint)
 {
   MOZ_ASSERT(!(aRestyleHint & eRestyle_LaterSiblings),
              "eRestyle_LaterSiblings must not be part of aRestyleHint");
 
   // XXXldb get new context from prev-in-flow if possible, to avoid
   // duplication.  (Or should we just let |GetContext| handle that?)
   // Getting the hint would be nice too, but that's harder.
 
@@ -4138,54 +4112,16 @@ ElementRestyler::RestyleSelf(nsIFrame* a
       // which is important to maintain various invariants about
       // frame types matching their style contexts.
       // Note that this check even makes sense if we didn't call
       // CaptureChange because of copyFromContinuation being true,
       // since we'll have copied the existing context from the
       // previous continuation, so newContext == oldContext.
 
       if (result != eRestyleResult_Stop) {
-        if (copyFromContinuation) {
-          LOG_RESTYLE("not swapping style structs, since we copied from a "
-                      "continuation");
-        } else if (oldContext->IsShared() && newContext->IsShared()) {
-          LOG_RESTYLE("not swapping style structs, since both old and contexts "
-                      "are shared");
-        } else if (oldContext->IsShared()) {
-          LOG_RESTYLE("not swapping style structs, since the old context is "
-                      "shared");
-        } else if (newContext->IsShared()) {
-          LOG_RESTYLE("not swapping style structs, since the new context is "
-                      "shared");
-        } else {
-          if (result == eRestyleResult_StopWithStyleChange) {
-            LOG_RESTYLE("recording a style struct swap between %p and %p to "
-                        "do if eRestyleResult_StopWithStyleChange fails",
-                        oldContext.get(), newContext.get());
-            SwapInstruction* swap = aSwaps.AppendElement();
-            swap->mOldContext = oldContext;
-            swap->mNewContext = newContext;
-            swap->mStructsToSwap = equalStructs;
-          } else {
-            LOG_RESTYLE("swapping style structs between %p and %p",
-                        oldContext.get(), newContext.get());
-            oldContext->SwapStyleData(newContext, equalStructs);
-            *aSwappedStructs |= equalStructs;
-          }
-#ifdef RESTYLE_LOGGING
-          uint32_t structs = RestyleManager::StructsToLog() & equalStructs;
-          if (structs) {
-            LOG_RESTYLE_INDENT();
-            LOG_RESTYLE("old style context now has: %s",
-                        oldContext->GetCachedStyleDataAsString(structs).get());
-            LOG_RESTYLE("new style context now has: %s",
-                        newContext->GetCachedStyleDataAsString(structs).get());
-          }
-#endif
-        }
         LOG_RESTYLE("setting new style context");
         aSelf->SetStyleContext(newContext);
       }
     } else {
       LOG_RESTYLE("not setting new style context, since we'll reframe");
       // We need to keep the new parent alive, in case it had structs
       // swapped into it that our frame's style context still has cached.
       // This is a similar scenario to the one described in the
--- a/layout/base/RestyleManager.h
+++ b/layout/base/RestyleManager.h
@@ -711,30 +711,20 @@ private:
 
     // continue restyling children
     eRestyleResult_Continue,
 
     // continue restyling children with eRestyle_ForceDescendants set
     eRestyleResult_ContinueAndForceDescendants
   };
 
-  struct SwapInstruction
-  {
-    RefPtr<nsStyleContext> mOldContext;
-    RefPtr<nsStyleContext> mNewContext;
-    uint32_t mStructsToSwap;
-  };
-
   /**
    * First half of Restyle().
    */
-  RestyleResult RestyleSelf(nsIFrame* aSelf,
-                            nsRestyleHint aRestyleHint,
-                            uint32_t* aSwappedStructs,
-                            nsTArray<SwapInstruction>& aSwaps);
+  RestyleResult RestyleSelf(nsIFrame* aSelf, nsRestyleHint aRestyleHint);
 
   /**
    * Restyle the children of this frame (and, in turn, their children).
    *
    * Second half of Restyle().
    */
   void RestyleChildren(nsRestyleHint aChildRestyleHint);