Bug 1396018: Remove REMOVE_DESTROY_FRAMES. r=bz
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Sep 2017 16:17:43 +0200
changeset 428856 6b0b2d0e08aae0baa13dc198ad14c4ea051c11df
parent 428855 911c5220cc3e74419f1163aa3caba121ee651167
child 428857 0da4bb3c477c4f27ea7f3dc7a389fa6b83147fe9
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1396018
milestone57.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 1396018: Remove REMOVE_DESTROY_FRAMES. r=bz Instead, explicitly capture the frame tree state on the only caller that passes this flag around. This is somewhat wasteful if we end up reconstructing an ancestor frame, since we'll capture state for it again. But we do that already in most of the cases (somewhat inconsistently). MozReview-Commit-ID: CUJsXaVoIpO Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -8685,19 +8685,16 @@ nsCSSFrameConstructor::ContentRemoved(ns
     }
     UnregisterDisplayContentsStyleFor(aChild, aContainer);
     return false;
   }
 
 #ifdef MOZ_XUL
   if (NotifyListBoxBody(presContext, aContainer, aChild, aOldNextSibling,
                         childFrame, CONTENT_REMOVED)) {
-    if (aFlags == REMOVE_DESTROY_FRAMES) {
-      CaptureStateForFramesOf(aChild, mTempFrameTreeState);
-    }
     return false;
   }
 #endif // MOZ_XUL
 
   // If we're removing the root, then make sure to remove things starting at
   // the viewport's child instead of the primary frame (which might even be
   // null if the root had an XBL binding or display:none, even though the
   // frames above it got created).  We do the adjustment after the childFrame
@@ -8729,20 +8726,16 @@ nsCSSFrameConstructor::ContentRemoved(ns
     // XXXsmaug This is super unefficient!
     nsIContent* bindingParent = aContainer->GetBindingParent();
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(bindingParent, aInsertionKind);
     LAYOUT_PHASE_TEMP_REENTER();
     return true;
   }
 
-  if (aFlags == REMOVE_DESTROY_FRAMES) {
-    CaptureStateForFramesOf(aChild, mTempFrameTreeState);
-  }
-
   if (childFrame) {
     InvalidateCanvasIfNeeded(mPresShell, aChild);
 
     // See whether we need to remove more than just childFrame
     LAYOUT_PHASE_TEMP_EXIT();
     if (MaybeRecreateContainerForFrameRemoval(childFrame, aInsertionKind)) {
       LAYOUT_PHASE_TEMP_REENTER();
       return true;
@@ -10053,17 +10046,19 @@ nsCSSFrameConstructor::RecreateFramesFor
     nsIContent* nextSibling = aContent->IsRootOfAnonymousSubtree() ?
       nullptr : aContent->GetNextSibling();
     bool didReconstruct =
       ContentRemoved(container, aContent, nextSibling, aInsertionKind,
                      REMOVE_FOR_RECONSTRUCTION);
 
     if (!didReconstruct) {
       if (aInsertionKind == InsertionKind::Async) {
-        // XXXmats doesn't frame state need to be restored in this case too?
+        // FIXME(emilio, bug 1397239): There's nothing removing the frame state
+        // for elements that go away before we come back to the frame
+        // constructor.
         RestyleManager()->PostRestyleEvent(aContent->AsElement(),
                                            nsRestyleHint(0),
                                            nsChangeHint_ReconstructFrame);
       } else {
         // Now, recreate the frames associated with this content object. If
         // ContentRemoved triggered reconstruction, then we don't need to do this
         // because the frames will already have been built.
         ContentRangeInserted(container, aContent, aContent->GetNextSibling(),
@@ -10078,21 +10073,23 @@ nsCSSFrameConstructor::RecreateFramesFor
 
 bool
 nsCSSFrameConstructor::DestroyFramesFor(Element* aElement)
 {
   MOZ_ASSERT(aElement && aElement->GetParentNode());
 
   nsIContent* nextSibling =
     aElement->IsRootOfAnonymousSubtree() ? nullptr : aElement->GetNextSibling();
+
+  CaptureStateForFramesOf(aElement, mTempFrameTreeState);
   return ContentRemoved(aElement->GetParent(),
                         aElement,
                         nextSibling,
                         InsertionKind::Async,
-                        REMOVE_DESTROY_FRAMES);
+                        REMOVE_FOR_RECONSTRUCTION);
 }
 
 //////////////////////////////////////////////////////////////////////
 
 // Block frame construction code
 
 already_AddRefed<nsStyleContext>
 nsCSSFrameConstructor::GetFirstLetterStyle(nsIContent* aContent,
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -307,37 +307,37 @@ public:
    * from ContentRemoved.
    */
   enum class InsertionKind
   {
     Sync,
     Async,
   };
 
-  // FIXME(emilio): How important is it to keep the frame tree state around for
-  // REMOVE_DESTROY_FRAMES?
-  //
-  // Seems like otherwise we could just remove it.
   enum RemoveFlags {
-    REMOVE_CONTENT, REMOVE_FOR_RECONSTRUCTION, REMOVE_DESTROY_FRAMES };
+    REMOVE_CONTENT,
+    REMOVE_FOR_RECONSTRUCTION,
+  };
+
   /**
    * Recreate or destroy frames for aChild in aContainer.
+   *
    * aFlags == REMOVE_CONTENT means aChild has been removed from the document.
    * aFlags == REMOVE_FOR_RECONSTRUCTION means the caller will reconstruct the
-   *   frames later.
+   * frames later.
+   *
    * In both the above cases, this method will in some cases try to reconstruct
-   * the frames (and true will be returned in that case), it's just that in the
-   * former case aChild isn't in the document so no frames will be created for
-   * it.  Ancestors may have been reframed though.
-   * aFlags == REMOVE_DESTROY_FRAMES is the same as REMOVE_FOR_RECONSTRUCTION
-   * except it will never try to reconstruct frames.  Instead, the caller is
-   * responsible for doing that, on the content returned in aDestroyedFramesFor.
-   * The layout frame state is guarranted to be captured for the removed frames
-   * only when aFlags == REMOVE_DESTROY_FRAMES, otherwise it will only be
-   * captured if we reconstructed frames for an ancestor.
+   * frames on some ancestor of aChild.  This can happen regardless of the value
+   * of aFlags.
+   *
+   * The return value indicates whether this "reconstruct an ancestor" action
+   * took place.  If true is returned, that means that the frame subtree rooted
+   * at some ancestor of aChild's frame was destroyed and either has been
+   * reconstructed or will be reconstructed async, depending on the value of
+   * aInsertionKind.
    */
   bool ContentRemoved(nsIContent* aContainer,
                       nsIContent* aChild,
                       nsIContent* aOldNextSibling,
                       InsertionKind aInsertionKind,
                       RemoveFlags aFlags);
 
   void CharacterDataChanged(nsIContent* aContent,