Bug 1411754: Rename PresShell::DestroyFramesFor to DestroyFramesForAndRestyle. r?mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 25 Oct 2017 23:12:25 +0200
changeset 686961 8f7dc783365197d8b5294642577586189fa21842
parent 686864 a53d7991e5c8fbcd66bffd3dad60a6e6c6d93737
child 737531 3def0580bb699c1e225922cff14fead09ba92cbf
push id86359
push userbmo:emilio@crisal.io
push dateThu, 26 Oct 2017 17:03:05 +0000
reviewersmats
bugs1411754, 1397239
milestone58.0a1
Bug 1411754: Rename PresShell::DestroyFramesFor to DestroyFramesForAndRestyle. r?mats I'm drive-by removing the comment about the frame tree state because I looked into it, and the answer is: we properly restore it. The gotcha is that we retain it too much, indeed, we retain it enough that it can leak. See bug 1397239. MozReview-Commit-ID: LP6bXkduEZ4
dom/base/Element.cpp
dom/base/ShadowRoot.cpp
dom/xbl/nsXBLService.cpp
layout/base/PresShell.cpp
layout/base/PresShell.h
layout/base/nsIPresShell.h
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -1164,17 +1164,17 @@ Element::CreateShadowRoot(ErrorResult& a
                               docInfo, nullptr, true);
   if (aError.Failed()) {
     delete protoBinding;
     return nullptr;
   }
 
   if (nsIDocument* doc = GetComposedDoc()) {
     if (nsIPresShell* shell = doc->GetShell()) {
-      shell->DestroyFramesFor(this);
+      shell->DestroyFramesForAndRestyle(this);
       MOZ_ASSERT(!shell->FrameManager()->GetDisplayContentsStyleFor(this));
     }
   }
   MOZ_ASSERT(!GetPrimaryFrame());
 
   // Unlike for XBL, false is the default for inheriting style.
   protoBinding->SetInheritsStyle(false);
 
--- a/dom/base/ShadowRoot.cpp
+++ b/dom/base/ShadowRoot.cpp
@@ -262,18 +262,17 @@ ShadowRoot::DistributionChanged()
     return;
   }
 
   auto* shell = OwnerDoc()->GetShell();
   if (!shell) {
     return;
   }
 
-  // FIXME(emilio): Rename this to DestroyFramesForAndRestyle?
-  shell->DestroyFramesFor(host);
+  shell->DestroyFramesForAndRestyle(host);
 }
 
 const HTMLContentElement*
 ShadowRoot::DistributeSingleNode(nsIContent* aContent)
 {
   // Find the insertion point to which the content belongs.
   HTMLContentElement* foundInsertionPoint = nullptr;
   for (HTMLContentElement* insertionPoint : mInsertionPoints) {
@@ -508,17 +507,17 @@ ShadowRoot::AttributeChanged(nsIDocument
     return;
   }
 
   auto* shell = OwnerDoc()->GetShell();
   if (!shell) {
     return;
   }
 
-  shell->DestroyFramesFor(aElement);
+  shell->DestroyFramesForAndRestyle(aElement);
 }
 
 bool
 ShadowRoot::RedistributeElement(Element* aElement)
 {
   auto* oldInsertionPoint = RemoveDistributedNode(aElement);
   auto* newInsertionPoint = DistributeSingleNode(aElement);
 
--- a/dom/xbl/nsXBLService.cpp
+++ b/dom/xbl/nsXBLService.cpp
@@ -121,17 +121,17 @@ public:
     nsXBLService::GetInstance()->BindingReady(mBoundElement, mBindingURI, &ready);
     if (!ready)
       return;
 
     // Destroy the frames for mBoundElement. Do this after getting the binding,
     // since if the binding fetch fails then we don't want to destroy the
     // frames.
     if (nsIPresShell* shell = doc->GetShell()) {
-      shell->DestroyFramesFor(mBoundElement->AsElement());
+      shell->DestroyFramesForAndRestyle(mBoundElement->AsElement());
     }
     MOZ_ASSERT(!mBoundElement->GetPrimaryFrame());
   }
 
   nsXBLBindingRequest(nsIURI* aURI, nsIContent* aBoundElement)
     : mBindingURI(aURI),
       mBoundElement(aBoundElement)
   {
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -2917,17 +2917,17 @@ PresShell::CancelAllPendingReflows()
     GetPresContext()->RefreshDriver()->RemoveLayoutFlushObserver(this);
     mObservingLayoutFlushes = false;
   }
 
   ASSERT_REFLOW_SCHEDULED_STATE();
 }
 
 void
-PresShell::DestroyFramesFor(Element* aElement)
+PresShell::DestroyFramesForAndRestyle(Element* aElement)
 {
   MOZ_ASSERT(aElement);
   NS_ENSURE_TRUE_VOID(mPresContext);
   if (!mDidInitialize) {
     return;
   }
 
   nsAutoScriptBlocker scriptBlocker;
@@ -2935,31 +2935,25 @@ PresShell::DestroyFramesFor(Element* aEl
   // Mark ourselves as not safe to flush while we're doing frame destruction.
   ++mChangeNestCount;
 
   nsCSSFrameConstructor* fc = FrameConstructor();
   fc->BeginUpdate();
   bool didReconstruct = fc->DestroyFramesFor(aElement);
   fc->EndUpdate();
 
-  // XXXmats doesn't frame state need to be restored in this case?
-  if (!didReconstruct) {
-    PostRecreateFramesFor(aElement);
-  }
-
-  // NOTE(emilio): This is needed to force also a full subtree restyle for the
-  // content (in Stylo, where the existence of frames != the existence of
-  // styles).
-  //
-  // It's a bit out of place in a function called DestroyFramesFor,
-  // however the two only callers of this code really need this (given they
-  // shuffle the flattened tree around), and this avoids exposing additional
-  // APIs on the pres shell.
+  auto changeHint = didReconstruct
+    ? nsChangeHint(0)
+    : nsChangeHint_ReconstructFrame;
+
+  // NOTE(emilio): eRestyle_Subtree is needed to force also a full subtree
+  // restyle for the content (in Stylo, where the existence of frames != the
+  // existence of styles).
   mPresContext->RestyleManager()->PostRestyleEvent(
-    aElement, eRestyle_Subtree, nsChangeHint(0));
+    aElement, eRestyle_Subtree, changeHint);
 
   --mChangeNestCount;
 }
 
 void
 nsIPresShell::PostRecreateFramesFor(Element* aElement)
 {
   if (MOZ_UNLIKELY(!mDidInitialize)) {
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -126,17 +126,17 @@ public:
                                 nsFrameState aBitToAdd,
                                 ReflowRootHandling aRootHandling =
                                   eInferFromBitToAdd) override;
   virtual void FrameNeedsToContinueReflow(nsIFrame *aFrame) override;
   virtual void CancelAllPendingReflows() override;
   virtual bool IsSafeToFlush() const override;
   virtual void DoFlushPendingNotifications(mozilla::FlushType aType) override;
   virtual void DoFlushPendingNotifications(mozilla::ChangesToFlush aType) override;
-  virtual void DestroyFramesFor(mozilla::dom::Element* aElement) override;
+  virtual void DestroyFramesForAndRestyle(mozilla::dom::Element* aElement) 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;
--- a/layout/base/nsIPresShell.h
+++ b/layout/base/nsIPresShell.h
@@ -523,17 +523,17 @@ public:
   virtual void NotifyCounterStylesAreDirty() = 0;
 
   /**
    * Destroy the frames for aElement, and reconstruct them asynchronously if
    * needed.
    *
    * Note that this may destroy frames for an ancestor instead.
    */
-  virtual void DestroyFramesFor(mozilla::dom::Element* aElement) = 0;
+  virtual void DestroyFramesForAndRestyle(mozilla::dom::Element* aElement) = 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.