Bug 1357142: Kill PresShell::RecreateFramesFor. r=bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 17 Apr 2017 18:01:37 +0200
changeset 567713 3e77ea20e28def1c20d96d4cd432542a704daf3a
parent 563687 05c212a94183838f12feebb2c3fd483a6eec18c2
child 567714 be6b3d2ec0e1b4e42c0f80ac7eaee72dd0cc6c16
push id55671
push userbmo:emilio+bugs@crisal.io
push dateTue, 25 Apr 2017 12:32:43 +0000
reviewersbz
bugs1357142
milestone55.0a1
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>
dom/base/nsObjectLoadingContent.cpp
dom/xbl/nsBindingManager.cpp
dom/xbl/nsBindingManager.h
dom/xbl/nsXBLResourceLoader.cpp
editor/libeditor/HTMLAnonymousNodeEditor.cpp
layout/base/PresShell.cpp
layout/base/PresShell.h
layout/base/nsIPresShell.h
--- a/dom/base/nsObjectLoadingContent.cpp
+++ b/dom/base/nsObjectLoadingContent.cpp
@@ -2669,38 +2669,42 @@ nsObjectLoadingContent::NotifyStateChang
 
   nsIDocument* doc = thisContent->GetComposedDoc();
   if (!doc) {
     return; // Nothing to do
   }
 
   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
 nsObjectLoadingContent::GetTypeOfContent(const nsCString& aMIMEType)
 {
   nsCOMPtr<nsIContent> thisContent =
     do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
   NS_ASSERTION(thisContent, "must be a content");
--- a/dom/xbl/nsBindingManager.cpp
+++ b/dom/xbl/nsBindingManager.cpp
@@ -250,52 +250,53 @@ nsBindingManager::GetAnonymousNodesFor(n
 nsINodeList*
 nsBindingManager::GetAnonymousNodesFor(nsIContent* aContent)
 {
   nsXBLBinding* binding = GetBindingWithContent(aContent);
   return binding ? binding->GetAnonymousNodeList() : nullptr;
 }
 
 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;
   }
 
   // For now we can only handle removing a binding if it's the only one
   NS_ENSURE_FALSE(binding->GetBaseBinding(), NS_ERROR_FAILURE);
 
   // Hold strong ref in case removing the binding tries to close the
   // window or something.
   // 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
   // been removed and style may have changed due to the removal of the
   // anonymous children.
   // XXXbz this should be using the current doc (if any), not the owner doc.
   nsIPresShell *presShell = doc->GetShell();
   NS_ENSURE_TRUE(presShell, NS_ERROR_FAILURE);
 
-  return presShell->RecreateFramesFor(aContent);;
+  presShell->PostRecreateFramesFor(aElement);
+  return NS_OK;
 }
 
 nsresult
 nsBindingManager::LoadBindingDocument(nsIDocument* aBoundDoc,
                                       nsIURI* aURL,
                                       nsIPrincipal* aOriginPrincipal)
 {
   NS_PRECONDITION(aURL, "Must have a URI to load!");
--- a/dom/xbl/nsBindingManager.h
+++ b/dom/xbl/nsBindingManager.h
@@ -85,17 +85,17 @@ public:
    * Return the nodelist of "anonymous" kids for this node.  This might
    * actually include some of the nodes actual DOM kids, if there are
    * <children> tags directly as kids of <content>.  This will only end up
    * returning a non-null list for nodes which have a binding attached.
    */
   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);
 
   nsresult AddToAttachedQueue(nsXBLBinding* aBinding);
   void RemoveFromAttachedQueue(nsXBLBinding* aBinding);
   void ProcessAttachedQueue(uint32_t aSkipSize = 0)
   {
     if (mProcessingAttachedStack || mAttachedStack.Length() <= aSkipSize) {
--- a/dom/xbl/nsXBLResourceLoader.cpp
+++ b/dom/xbl/nsXBLResourceLoader.cpp
@@ -216,25 +216,26 @@ nsXBLResourceLoader::NotifyBoundElements
   if (!xblService)
     return;
 
   nsIURI* bindingURI = mBinding->BindingURI();
 
   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);
 
     if (ready) {
       // 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);
 
         // If |content| is (in addition to having binding |mBinding|)
         // also a descendant of another element with binding |mBinding|,
         // then we might have just constructed it due to the
         // notification of its parent.  (We can know about both if the
@@ -243,21 +244,24 @@ nsXBLResourceLoader::NotifyBoundElements
         // has a primary frame and whether it's in the undisplayed map
         // before sending a ContentInserted notification, or bad things
         // will happen.
         nsIPresShell *shell = doc->GetShell();
         if (shell) {
           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());
             }
           }
         }
 
         // Flush again
         // XXXbz why is this needed?
         doc->FlushPendingNotifications(FlushType::ContentAndNotify);
       }
--- a/editor/libeditor/HTMLAnonymousNodeEditor.cpp
+++ b/editor/libeditor/HTMLAnonymousNodeEditor.cpp
@@ -228,26 +228,26 @@ HTMLEditor::CreateAnonymousElement(nsIAt
 
   ElementDeletionObserver* observer =
     new ElementDeletionObserver(newContent, parentContent);
   NS_ADDREF(observer); // NodeWillBeDestroyed releases.
   parentContent->AddMutationObserver(observer);
   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();
 }
 
 // Removes event listener and calls DeleteRefToAnonymousNode.
 void
 HTMLEditor::RemoveListenerAndDeleteRef(const nsAString& aEvent,
                                        nsIDOMEventListener* aListener,
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -2945,54 +2945,16 @@ PresShell::CreateFramesFor(nsIContent* a
   nsILayoutHistoryState* layoutState = fc->GetLastCapturedLayoutHistoryState();
   fc->BeginUpdate();
   fc->ContentInserted(aContent->GetParent(), aContent, layoutState, false);
   fc->EndUpdate();
 
   --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)
 {
   mPresContext->RestyleManager()->PostRestyleEvent(aElement, nsRestyleHint(0),
                                                    nsChangeHint_ReconstructFrame);
 }
 
 void
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -51,16 +51,20 @@ struct nsCallbackEventRequest;
 class ReflowCountMgr;
 #endif
 
 class nsPresShellEventCB;
 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
 // PresShell. The set contains nsIFrame* pointers.
 typedef nsTHashtable<nsPtrHashKey<nsIFrame>> VisibleFrames;
 
 // A hash table type for tracking visible regions, for use by the visibility
 // code in PresShell. The mapping is from view IDs to regions in the
@@ -128,21 +132,16 @@ public:
   virtual bool IsSafeToFlush() const override;
   virtual void DoFlushPendingNotifications(mozilla::FlushType aType) override;
   virtual void DoFlushPendingNotifications(mozilla::ChangesToFlush aType) override;
   virtual void DestroyFramesFor(nsIContent*  aContent,
                                 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.
    */
   virtual nsresult PostReflowCallback(nsIReflowCallback* aCallback) override;
   virtual void CancelReflowCallback(nsIReflowCallback* aCallback) override;
 
   virtual void ClearFrameRefs(nsIFrame* aFrame) override;
   virtual already_AddRefed<gfxContext> CreateReferenceRenderingContext() override;
   virtual nsresult GoToAnchor(const nsAString& aAnchorName, bool aScroll,
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -548,21 +548,16 @@ public:
                                 nsIContent** aDestroyedFramesFor) = 0;
   /**
    * Create new frames for aContent.  It will use the last captured layout
    * history state captured in the frame constructor to restore the state
    * in the new frame tree.
    */
   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);
 
   // ShadowRoot has APIs that can change styles so we only
   // want to restyle elements in the ShadowRoot and not the whole
   // document.
   virtual void RecordShadowStyleChange(mozilla::dom::ShadowRoot* aShadowRoot) = 0;