Bug 1357142: Kill PresShell::RecreateFramesFor. r=bz

It's not only inefficient, but also prone to buggyness. Since styles may not be
up-to-date when it happens.

Post a reconstruct instead, which ensures a style flush happens before running
frame construction.

MozReview-Commit-ID: DrakHsJv5fY
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

--HG--
extra : rebase_source : 11900af908654336cc2391ab9480542c5474e38f

dom/base/nsObjectLoadingContent.cpp

@@ -2674,28 +2674,32 @@ nsObjectLoadingContent::NotifyStateChanged(ObjectType aOldType,
 
   EventStates newState = ObjectState();
 
+  if (newState == aOldState && mType == aOldType) {
+    return; // Also done.
+  }
+
   if (newState != aOldState) {
     NS_ASSERTION(thisContent->IsInComposedDoc(), "Something is confused");
     // This will trigger frame construction
     EventStates changedBits = aOldState ^ newState;
-
     {
       nsAutoScriptBlocker scriptBlocker;
       doc->ContentStateChanged(thisContent, changedBits);
     }
-    if (aSync) {
-      NS_ASSERTION(InActiveDocument(thisContent), "Something is confused");
-      // Make sure that frames are actually constructed immediately.
-      doc->FlushPendingNotifications(FlushType::Frames);
-    }
   } else if (aOldType != mType) {
     // If our state changed, then we already recreated frames
     // Otherwise, need to do that here
     nsCOMPtr<nsIPresShell> shell = doc->GetShell();
     if (shell) {
-      shell->RecreateFramesFor(thisContent);
+      shell->PostRecreateFramesFor(thisContent->AsElement());
     }
   }
+
+  if (aSync) {
+    NS_ASSERTION(InActiveDocument(thisContent), "Something is confused");
+    // Make sure that frames are actually constructed immediately.
+    doc->FlushPendingNotifications(FlushType::Frames);
+  }
 }
 
 nsObjectLoadingContent::ObjectType

dom/xbl/nsBindingManager.cpp

@@ -255,11 +255,11 @@ nsBindingManager::GetAnonymousNodesFor(nsIContent* aContent)
 }
 
 nsresult
-nsBindingManager::ClearBinding(nsIContent* aContent)
+nsBindingManager::ClearBinding(Element* aElement)
 {
   // Hold a ref to the binding so it won't die when we remove it from our table
   RefPtr<nsXBLBinding> binding =
-    aContent ? aContent->GetXBLBinding() : nullptr;
+    aElement ? aElement->GetXBLBinding() : nullptr;
 
   if (!binding) {
     return NS_OK;
@@ -273,14 +273,14 @@ nsBindingManager::ClearBinding(nsIContent* aContent)
   // XXXbz should that be ownerdoc?  Wouldn't we need a ref to the
   // currentdoc too?  What's the one that should be passed to
   // ChangeDocument?
-  nsCOMPtr<nsIDocument> doc = aContent->OwnerDoc();
+  nsCOMPtr<nsIDocument> doc = aElement->OwnerDoc();
 
   // Finally remove the binding...
   // XXXbz this doesn't remove the implementation!  Should fix!  Until
   // then we need the explicit UnhookEventHandlers here.
   binding->UnhookEventHandlers();
   binding->ChangeDocument(doc, nullptr);
-  aContent->SetXBLBinding(nullptr, this);
+  aElement->SetXBLBinding(nullptr, this);
   binding->MarkForDeath();
 
   // ...and recreate its frames. We need to do this since the frames may have
@@ -290,7 +290,8 @@ nsBindingManager::ClearBinding(nsIContent* aContent)
   nsIPresShell *presShell = doc->GetShell();
   NS_ENSURE_TRUE(presShell, NS_ERROR_FAILURE);
 
-  return presShell->RecreateFramesFor(aContent);;
+  presShell->PostRecreateFramesFor(aElement);
+  return NS_OK;
 }
 
 nsresult

dom/xbl/nsBindingManager.h

@@ -90,7 +90,7 @@ public:
   nsresult GetAnonymousNodesFor(nsIContent* aContent, nsIDOMNodeList** aResult);
   nsINodeList* GetAnonymousNodesFor(nsIContent* aContent);
 
-  nsresult ClearBinding(nsIContent* aContent);
+  nsresult ClearBinding(mozilla::dom::Element* aElement);
   nsresult LoadBindingDocument(nsIDocument* aBoundDoc, nsIURI* aURL,
                                nsIPrincipal* aOriginPrincipal);
 

dom/xbl/nsXBLResourceLoader.cpp

@@ -221,7 +221,8 @@ nsXBLResourceLoader::NotifyBoundElements()
   uint32_t eltCount = mBoundElements.Count();
   for (uint32_t j = 0; j < eltCount; j++) {
     nsCOMPtr<nsIContent> content = mBoundElements.ObjectAt(j);
-    
+    MOZ_ASSERT(content->IsElement());
+
     bool ready = false;
     xblService->BindingReady(content, bindingURI, &ready);
 
@@ -229,7 +230,7 @@ nsXBLResourceLoader::NotifyBoundElements()
       // We need the document to flush out frame construction and
       // such, so we want to use the current document.
       nsIDocument* doc = content->GetUncomposedDoc();
-    
+
       if (doc) {
         // Flush first to make sure we can get the frame for content
         doc->FlushPendingNotifications(FlushType::Frames);
@@ -248,11 +249,14 @@ nsXBLResourceLoader::NotifyBoundElements()
           nsIFrame* childFrame = content->GetPrimaryFrame();
           if (!childFrame) {
             // Check to see if it's in the undisplayed content map.
+            //
+            // FIXME(emilio, bug 1359384): What about display: contents stuff?
+            // Looks like this would be inefficient in that case?
             nsStyleContext* sc =
               shell->FrameManager()->GetUndisplayedContent(content);
 
             if (!sc) {
-              shell->RecreateFramesFor(content);
+              shell->PostRecreateFramesFor(content->AsElement());
             }
           }
         }

editor/libeditor/HTMLAnonymousNodeEditor.cpp

@@ -233,16 +233,16 @@ HTMLEditor::CreateAnonymousElement(nsIAtom* aTag,
   newContent->AddMutationObserver(observer);
 
 #ifdef DEBUG
-  // Editor anonymous content gets passed to RecreateFramesFor... which can't
-  // _really_ deal with anonymous content (because it can't get the frame tree
-  // ordering right).  But for us the ordering doesn't matter so this is sort of
-  // ok.
+  // Editor anonymous content gets passed to PostRecreateFramesFor... which
+  // can't _really_ deal with anonymous content (because it can't get the frame
+  // tree ordering right).  But for us the ordering doesn't matter so this is
+  // sort of ok.
   newContent->SetProperty(nsGkAtoms::restylableAnonymousNode,
 			  reinterpret_cast<void*>(true));
 #endif // DEBUG
 
   // display the element
-  ps->RecreateFramesFor(newContent);
+  ps->PostRecreateFramesFor(newContent);
 
   return newContent.forget();
 }

layout/base/PresShell.cpp

@@ -2946,44 +2946,6 @@ PresShell::CreateFramesFor(nsIContent* aContent)
   --mChangeNestCount;
 }
 
-nsresult
-PresShell::RecreateFramesFor(nsIContent* aContent)
-{
-  NS_ENSURE_TRUE(mPresContext, NS_ERROR_FAILURE);
-  if (!mDidInitialize) {
-    // Nothing to do here.  In fact, if we proceed and aContent is the
-    // root we will crash.
-    return NS_OK;
-  }
-
-  // Don't call RecreateFramesForContent since that is not exported and we want
-  // to keep the number of entrypoints down.
-
-  NS_ASSERTION(mViewManager, "Should have view manager");
-
-  // Have to make sure that the content notifications are flushed before we
-  // start messing with the frame model; otherwise we can get content doubling.
-  mDocument->FlushPendingNotifications(FlushType::ContentAndNotify);
-
-  nsAutoScriptBlocker scriptBlocker;
-
-  nsStyleChangeList changeList(mPresContext->StyleSet()->BackendType());
-  changeList.AppendChange(nullptr, aContent, nsChangeHint_ReconstructFrame);
-
-  // We might have restyles pending when we're asked to recreate frames.
-  // Record that we're OK with stale styles being returned, to avoid assertions.
-  ServoStyleSet::AutoAllowStaleStyles guard(mStyleSet->GetAsServo());
-
-  // Mark ourselves as not safe to flush while we're doing frame construction.
-  ++mChangeNestCount;
-  RestyleManager* restyleManager = mPresContext->RestyleManager();
-  restyleManager->ProcessRestyledFrames(changeList);
-  restyleManager->FlushOverflowChangedTracker();
-  --mChangeNestCount;
-
-  return NS_OK;
-}
-
 void
 nsIPresShell::PostRecreateFramesFor(Element* aElement)
 {

layout/base/PresShell.h

@@ -56,6 +56,10 @@ class nsAutoCauseReflowNotifier;
 
 namespace mozilla {
 
+namespace dom {
+class Element;
+}  // namespace dom
+
 class EventDispatchingCallback;
 
 // A set type for tracking visible frames, for use by the visibility code in
@@ -132,11 +136,6 @@ public:
                                 nsIContent** aDestroyedFramesFor) override;
   virtual void CreateFramesFor(nsIContent* aContent) override;
 
-  /**
-   * Recreates the frames for a node
-   */
-  virtual nsresult RecreateFramesFor(nsIContent* aContent) override;
-
   /**
    * Post a callback that should be handled after reflow has finished.
    */

layout/base/nsIPresShell.h

@@ -549,11 +549,6 @@ public:
    */
   virtual void CreateFramesFor(nsIContent* aContent) = 0;
 
-  /**
-   * Recreates the frames for a node
-   */
-  virtual nsresult RecreateFramesFor(nsIContent* aContent) = 0;
-
   void PostRecreateFramesFor(mozilla::dom::Element* aElement);
   void RestyleForAnimation(mozilla::dom::Element* aElement,
                            nsRestyleHint aHint);