Bug 1392964: Remove aDidReconstruct outparam from ContentRemoved. r?mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 23 Aug 2017 11:03:30 +0200
changeset 651306 d5d80e6a8d1459000ddd11ee6a9a45d5c2de8017
parent 651147 d61e90059e20942c80eec29e8d1eba1cdd4e9589
child 727661 d4513ae2b72d29657c884f8313b9a7f78fa8864d
push id75671
push userbmo:emilio+bugs@crisal.io
push dateWed, 23 Aug 2017 13:43:12 +0000
reviewersmats
bugs1392964
milestone57.0a1
Bug 1392964: Remove aDidReconstruct outparam from ContentRemoved. r?mats MozReview-Commit-ID: 4fSAQQAnPWF
layout/base/PresShell.cpp
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -2922,19 +2922,18 @@ PresShell::DestroyFramesFor(Element* aEl
   }
 
   nsAutoScriptBlocker scriptBlocker;
 
   // Mark ourselves as not safe to flush while we're doing frame destruction.
   ++mChangeNestCount;
 
   nsCSSFrameConstructor* fc = FrameConstructor();
-  bool didReconstruct;
   fc->BeginUpdate();
-  fc->DestroyFramesFor(aElement, &didReconstruct);
+  bool didReconstruct = fc->DestroyFramesFor(aElement);
   fc->EndUpdate();
 
   // XXXmats doesn't frame state need to be restored in this case?
   if (!didReconstruct) {
     PostRecreateFramesFor(aElement);
   }
 
   mPresContext->RestyleManager()->PostRestyleEvent(
@@ -4485,21 +4484,18 @@ PresShell::ContentRemoved(nsIDocument *a
   for (auto iter = sPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
     nsIPresShell::PointerCaptureInfo* data = iter.UserData();
     if (data && data->mPendingContent &&
         nsContentUtils::ContentIsDescendantOf(data->mPendingContent, aChild)) {
       nsIPresShell::ReleasePointerCapturingContent(iter.Key());
     }
   }
 
-  bool didReconstruct;
   mFrameConstructor->ContentRemoved(aMaybeContainer, aChild, oldNextSibling,
-                                    nsCSSFrameConstructor::REMOVE_CONTENT,
-                                    &didReconstruct);
-
+                                    nsCSSFrameConstructor::REMOVE_CONTENT);
 
   if (aChild->NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE) {
     MOZ_ASSERT(container == aDocument);
     NotifyFontSizeInflationEnabledIsDirty();
   }
 
   VERIFY_STYLE_TREE;
 }
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -8635,31 +8635,27 @@ nsCSSFrameConstructor::ContentRangeInser
   nsAccessibilityService* accService = nsIPresShell::AccService();
   if (accService) {
     accService->ContentRangeInserted(mPresShell, aContainer,
                                      aStartChild, aEndChild);
   }
 #endif
 }
 
-void
+bool
 nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
                                       nsIContent* aChild,
                                       nsIContent* aOldNextSibling,
-                                      RemoveFlags aFlags,
-                                      bool*       aDidReconstruct)
+                                      RemoveFlags aFlags)
 {
   MOZ_ASSERT(aChild);
-  MOZ_ASSERT(aDidReconstruct);
   AUTO_LAYOUT_PHASE_ENTRY_POINT(mPresShell->GetPresContext(), FrameC);
   NS_PRECONDITION(mUpdateCount != 0,
                   "Should be in an update while destroying frames");
 
-  *aDidReconstruct = false;
-
   // Only recreate sync if we're already in frame construction, that is,
   // recreate async for XBL DOM changes, and normal content removals.
   const InsertionKind insertionKind = (aFlags == REMOVE_FOR_RECONSTRUCTION)
                                         ? InsertionKind::Sync
                                         : InsertionKind::Async;
 
   nsPresContext* presContext = mPresShell->GetPresContext();
   MOZ_ASSERT(presContext, "Our presShell should have a valid presContext");
@@ -8702,46 +8698,47 @@ nsCSSFrameConstructor::ContentRemoved(ns
     MOZ_ASSERT(ancestor, "display: contents on the root?");
     while (!ancestor->GetPrimaryFrame()) {
       ancestor = ancestor->GetFlattenedTreeParent();
       MOZ_ASSERT(ancestor, "we can't have a display: contents subtree root!");
     }
 
     nsIFrame* ancestorFrame = ancestor->GetPrimaryFrame();
     if (ancestorFrame->GetProperty(nsIFrame::GenConProperty())) {
-      *aDidReconstruct = true;
       // XXXmats Can we recreate frames only for the ::after/::before content?
       // XXX Perhaps even only those that belong to the aChild sub-tree?
       LAYOUT_PHASE_TEMP_EXIT();
       RecreateFramesForContent(ancestor, insertionKind, aFlags);
       LAYOUT_PHASE_TEMP_REENTER();
-      return;
+      return true;
     }
 
     FlattenedChildIterator iter(aChild);
+    bool didReconstruct = false;
     for (nsIContent* c = iter.GetNextChild(); c; c = iter.GetNextChild()) {
       if (c->GetPrimaryFrame() || GetDisplayContentsStyleFor(c)) {
         LAYOUT_PHASE_TEMP_EXIT();
-        ContentRemoved(aChild, c, nullptr, aFlags, aDidReconstruct);
+        didReconstruct |= ContentRemoved(aChild, c, nullptr, aFlags);
         LAYOUT_PHASE_TEMP_REENTER();
-        if (aFlags != REMOVE_DESTROY_FRAMES && *aDidReconstruct) {
-          return;
+        if (aFlags != REMOVE_DESTROY_FRAMES && didReconstruct) {
+          return true;
         }
       }
     }
     UnregisterDisplayContentsStyleFor(aChild, aContainer);
+    return didReconstruct;
   }
 
 #ifdef MOZ_XUL
   if (NotifyListBoxBody(presContext, aContainer, aChild, aOldNextSibling,
                         childFrame, CONTENT_REMOVED)) {
     if (aFlags == REMOVE_DESTROY_FRAMES) {
       CaptureStateForFramesOf(aChild, mTempFrameTreeState);
     }
-    return;
+    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
   // check above, because we do want to clear any undisplayed content we might
@@ -8766,84 +8763,79 @@ nsCSSFrameConstructor::ContentRemoved(ns
   if (aContainer && aContainer->HasFlag(NODE_IS_IN_SHADOW_TREE) &&
       !aContainer->IsInNativeAnonymousSubtree() &&
       !aChild->IsInNativeAnonymousSubtree()) {
     // Recreate frames if content is removed from a ShadowRoot because it may
     // contain an insertion point which can change how the host is rendered.
     //
     // XXXsmaug This is super unefficient!
     nsIContent* bindingParent = aContainer->GetBindingParent();
-    *aDidReconstruct = true;
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(bindingParent, insertionKind, aFlags);
     LAYOUT_PHASE_TEMP_REENTER();
-    return;
+    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, insertionKind, aFlags)) {
-      *aDidReconstruct = true;
       LAYOUT_PHASE_TEMP_REENTER();
-      return;
+      return true;
     }
     LAYOUT_PHASE_TEMP_REENTER();
 
     // Get the childFrame's parent frame
     nsIFrame* parentFrame = childFrame->GetParent();
     LayoutFrameType parentType = parentFrame->Type();
 
     if (parentType == LayoutFrameType::FrameSet &&
         IsSpecialFramesetChild(aChild)) {
       // Just reframe the parent, since framesets are weird like that.
-      *aDidReconstruct = true;
       LAYOUT_PHASE_TEMP_EXIT();
       RecreateFramesForContent(parentFrame->GetContent(), insertionKind,
                                aFlags);
       LAYOUT_PHASE_TEMP_REENTER();
-      return;
+      return true;
     }
 
     // If we're a child of MathML, then we should reframe the MathML content.
     // If we're non-MathML, then we would be wrapped in a block so we need to
     // check our grandparent in that case.
     nsIFrame* possibleMathMLAncestor = parentType == LayoutFrameType::Block
                                          ? parentFrame->GetParent()
                                          : parentFrame;
     if (possibleMathMLAncestor->IsFrameOfType(nsIFrame::eMathML)) {
-      *aDidReconstruct = true;
       LAYOUT_PHASE_TEMP_EXIT();
       RecreateFramesForContent(parentFrame->GetContent(), insertionKind, aFlags);
       LAYOUT_PHASE_TEMP_REENTER();
-      return;
+      return true;
     }
 
     // Undo XUL wrapping if it's no longer needed.
     // (If we're in the XUL block-wrapping situation, parentFrame is the
     // wrapper frame.)
     nsIFrame* grandparentFrame = parentFrame->GetParent();
     if (grandparentFrame && grandparentFrame->IsXULBoxFrame() &&
         (grandparentFrame->GetStateBits() & NS_STATE_BOX_WRAPS_KIDS_IN_BLOCK) &&
         // check if this frame is the only one needing wrapping
         aChild == AnyKidsNeedBlockParent(parentFrame->PrincipalChildList().FirstChild()) &&
         !AnyKidsNeedBlockParent(childFrame->GetNextSibling())) {
-      *aDidReconstruct = true;
       LAYOUT_PHASE_TEMP_EXIT();
       RecreateFramesForContent(grandparentFrame->GetContent(), insertionKind,
                                aFlags);
       LAYOUT_PHASE_TEMP_REENTER();
-      return;
+      return true;
     }
 
 #ifdef ACCESSIBILITY
     if (nsAccessibilityService* accService = nsIPresShell::AccService()) {
       accService->ContentRemoved(mPresShell, aChild);
     }
 #endif
 
@@ -8876,17 +8868,17 @@ nsCSSFrameConstructor::ContentRemoved(ns
       RemoveLetterFrames(mPresShell, containingBlock);
 
       // Recover childFrame and parentFrame
       childFrame = aChild->GetPrimaryFrame();
       if (!childFrame || childFrame->GetContent() != aChild) {
         // XXXbz the GetContent() != aChild check is needed due to bug 135040.
         // Remove it once that's fixed.
         UnregisterDisplayNoneStyleFor(aChild, aContainer);
-        return;
+        return false;
       }
       parentFrame = childFrame->GetParent();
       parentType = parentFrame->Type();
 
 #ifdef NOISY_FIRST_LETTER
       printf("  ==> revised parentFrame=");
       nsFrame::ListTag(stdout, parentFrame);
       printf(" childFrame=");
@@ -8969,16 +8961,18 @@ nsCSSFrameConstructor::ContentRemoved(ns
 
 #ifdef DEBUG
     if (gReallyNoisyContentUpdates && parentFrame) {
       printf("nsCSSFrameConstructor::ContentRemoved: resulting frame model:\n");
       parentFrame->List(stdout, 0);
     }
 #endif
   }
+
+  return false;
 }
 
 /**
  * This method invalidates the canvas when frames are removed or added for a
  * node that might have its background propagated to the canvas, i.e., a
  * document root node or an HTML BODY which is a child of the root node.
  *
  * @param aFrame a frame for a content node about to be removed or a frame that
@@ -10091,20 +10085,21 @@ nsCSSFrameConstructor::RecreateFramesFor
 
     // Need the nsIContent parent, which might be null here, since we need to
     // pass it to ContentInserted and ContentRemoved.
     //
     // FIXME(emilio): Do we need a strong ref here?
     nsCOMPtr<nsIContent> container = aContent->GetParent();
 
     // Remove the frames associated with the content object.
-    bool didReconstruct;
     nsIContent* nextSibling = aContent->IsRootOfAnonymousSubtree() ?
       nullptr : aContent->GetNextSibling();
-    ContentRemoved(container, aContent, nextSibling, aFlags, &didReconstruct);
+    bool didReconstruct =
+      ContentRemoved(container, aContent, nextSibling, aFlags);
+
     if (!didReconstruct) {
       if (aInsertionKind == InsertionKind::Async) {
         // XXXmats doesn't frame state need to be restored in this case too?
         RestyleManager()->PostRestyleEvent(aContent->AsElement(),
                                            nsRestyleHint(0),
                                            nsChangeHint_ReconstructFrame);
       } else {
         // Now, recreate the frames associated with this content object. If
@@ -10115,29 +10110,27 @@ nsCSSFrameConstructor::RecreateFramesFor
                              false, // aAllowLazyConstruction
                              true,  // aForReconstruction
                              nullptr);
       }
     }
   }
 }
 
-void
-nsCSSFrameConstructor::DestroyFramesFor(nsIContent* aContent,
-                                        bool* aDidReconstruct)
-{
-  MOZ_ASSERT(aContent && aContent->GetParentNode());
+bool
+nsCSSFrameConstructor::DestroyFramesFor(Element* aElement)
+{
+  MOZ_ASSERT(aElement && aElement->GetParentNode());
 
   nsIContent* nextSibling =
-    aContent->IsRootOfAnonymousSubtree() ? nullptr : aContent->GetNextSibling();
-  ContentRemoved(aContent->GetParent(),
-                 aContent,
-                 nextSibling,
-                 REMOVE_DESTROY_FRAMES,
-                 aDidReconstruct);
+    aElement->IsRootOfAnonymousSubtree() ? nullptr : aElement->GetNextSibling();
+  return ContentRemoved(aElement->GetParent(),
+                        aElement,
+                        nextSibling,
+                        REMOVE_DESTROY_FRAMES);
 }
 
 //////////////////////////////////////////////////////////////////////
 
 // Block frame construction code
 
 already_AddRefed<nsStyleContext>
 nsCSSFrameConstructor::GetFirstLetterStyle(nsIContent* aContent,
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -313,31 +313,30 @@ public:
   enum RemoveFlags {
     REMOVE_CONTENT, REMOVE_FOR_RECONSTRUCTION, REMOVE_DESTROY_FRAMES };
   /**
    * 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.
    * In both the above cases, this method will in some cases try to reconstruct
-   * the frames (aDidReconstruct is then set to true), it's just that in the
+   * 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.
    */
-  void ContentRemoved(nsIContent* aContainer,
+  bool ContentRemoved(nsIContent* aContainer,
                       nsIContent* aChild,
                       nsIContent* aOldNextSibling,
-                      RemoveFlags aFlags,
-                      bool*       aDidReconstruct);
+                      RemoveFlags aFlags);
 
   void CharacterDataChanged(nsIContent* aContent,
                             CharacterDataChangeInfo* aInfo);
 
   // If aContent is a text node that has been optimized away due to being
   // whitespace next to a block boundary (or for some other reason), stop
   // doing that and create a frame for it if it should have one. This recreates
   // frames so be careful (although this should not change actual layout).
@@ -359,20 +358,21 @@ public:
   void NotifyCounterStylesAreDirty();
 
   // Gets called when the presshell is destroying itself and also
   // when we tear down our frame tree to reconstruct it
   void WillDestroyFrameTree();
 
   /**
    * Destroy the frames for aContent.  Note that this may destroy frames
-   * for an ancestor instead - aDidReconstruct contains whether a reconstruct
-   * was posted for any ancestor.
+   * for an ancestor instead.
+   *
+   * Returns whether a reconstruct was posted for any ancestor.
    */
-  void DestroyFramesFor(nsIContent* aContent, bool* aDidReconstruct);
+  bool DestroyFramesFor(mozilla::dom::Element* aElement);
 
   // Request to create a continuing frame.  This method never returns null.
   nsIFrame* CreateContinuingFrame(nsPresContext*    aPresContext,
                                   nsIFrame*         aFrame,
                                   nsContainerFrame* aParentFrame,
                                   bool              aIsFluid = true);
 
   // Copy over fixed frames from aParentFrame's prev-in-flow