Bug 1361766: Move MathML content state changes outside of reflow. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 02 May 2017 22:42:26 +0200
changeset 573340 903bf049154f95919f362fe922a927710aeca0ac
parent 573249 47612d95b17794664b14584b295764bad724d270
child 627265 cb7fe90b655a2502aa77be8ff04661b6bfd7a0d9
push id57352
push userbmo:emilio+bugs@crisal.io
push dateFri, 05 May 2017 15:32:07 +0000
reviewersxidorn
bugs1361766
milestone55.0a1
Bug 1361766: Move MathML content state changes outside of reflow. r?xidorn This also adds assertions to ensure attributes and state don't change during layout or frame construction. MozReview-Commit-ID: BANcpxnRsYS
layout/base/GeckoRestyleManager.cpp
layout/base/RestyleManager.cpp
layout/base/ServoRestyleManager.cpp
layout/mathml/nsMathMLContainerFrame.cpp
layout/mathml/nsMathMLContainerFrame.h
layout/mathml/nsMathMLmunderoverFrame.cpp
layout/mathml/nsMathMLmunderoverFrame.h
layout/reftests/mathml/reftest.list
--- a/layout/base/GeckoRestyleManager.cpp
+++ b/layout/base/GeckoRestyleManager.cpp
@@ -242,16 +242,18 @@ GeckoRestyleManager::AttributeWillChange
 // passing the notification to the frame).
 void
 GeckoRestyleManager::AttributeChanged(Element* aElement,
                                       int32_t aNameSpaceID,
                                       nsIAtom* aAttribute,
                                       int32_t aModType,
                                       const nsAttrValue* aOldValue)
 {
+  MOZ_ASSERT(!mInStyleRefresh);
+
   // Hold onto the PresShell to prevent ourselves from being destroyed.
   // XXXbz how, exactly, would this attribute change cause us to be
   // destroyed from inside this function?
   nsCOMPtr<nsIPresShell> shell = PresContext()->GetPresShell();
   mozilla::Unused << shell; // Unused within this function
 
   // Get the frame associated with the content which is the highest in the frame tree
   nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -326,16 +326,17 @@ RestyleManager::ContentRemoved(nsINode* 
  * This is called from both Restyle managers.
  */
 void
 RestyleManager::ContentStateChangedInternal(Element* aElement,
                                             EventStates aStateMask,
                                             nsChangeHint* aOutChangeHint,
                                             nsRestyleHint* aOutRestyleHint)
 {
+  MOZ_ASSERT(!mInStyleRefresh);
   MOZ_ASSERT(aOutChangeHint);
   MOZ_ASSERT(aOutRestyleHint);
 
   StyleSetHandle styleSet = PresContext()->StyleSet();
   NS_ASSERTION(styleSet, "couldn't get style set");
 
   *aOutChangeHint = nsChangeHint(0);
   // Any change to a content state that affects which frames we construct
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -484,16 +484,18 @@ ServoRestyleManager::AttributeWillChange
   }
 }
 
 void
 ServoRestyleManager::AttributeChanged(Element* aElement, int32_t aNameSpaceID,
                                       nsIAtom* aAttribute, int32_t aModType,
                                       const nsAttrValue* aOldValue)
 {
+  MOZ_ASSERT(!mInStyleRefresh);
+
 #ifdef DEBUG
   ServoElementSnapshot* snapshot = Servo_Element_GetSnapshot(aElement);
   MOZ_ASSERT_IF(snapshot, snapshot->HasAttrs());
 #endif
 
   nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
   if (primaryFrame) {
     primaryFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);
--- a/layout/mathml/nsMathMLContainerFrame.cpp
+++ b/layout/mathml/nsMathMLContainerFrame.cpp
@@ -11,17 +11,16 @@
 #include "nsPresContext.h"
 #include "nsIPresShell.h"
 #include "nsStyleContext.h"
 #include "nsNameSpaceManager.h"
 #include "nsRenderingContext.h"
 #include "nsIDOMMutationEvent.h"
 #include "nsGkAtoms.h"
 #include "nsDisplayList.h"
-#include "nsIReflowCallback.h"
 #include "mozilla/Likely.h"
 #include "nsIScriptError.h"
 #include "nsContentUtils.h"
 #include "nsMathMLElement.h"
 
 using namespace mozilla;
 using namespace mozilla::gfx;
 
@@ -1313,48 +1312,16 @@ nsMathMLContainerFrame::PositionRowChild
     nscoord dx = aOffsetX + child.X();
     nscoord dy = aBaseline - child.Ascent();
     FinishReflowChild(child.Frame(), PresContext(), child.GetReflowOutput(),
                       nullptr, dx, dy, 0);
     ++child;
   }
 }
 
-class ForceReflow : public nsIReflowCallback {
-public:
-  virtual bool ReflowFinished() override {
-    return true;
-  }
-  virtual void ReflowCallbackCanceled() override {}
-};
-
-// We only need one of these so we just make it a static global, no need
-// to dynamically allocate/destroy it.
-static ForceReflow gForceReflow;
-
-void
-nsMathMLContainerFrame::SetIncrementScriptLevel(int32_t aChildIndex, bool aIncrement)
-{
-  nsIFrame* child = PrincipalChildList().FrameAt(aChildIndex);
-  if (!child)
-    return;
-  nsIContent* content = child->GetContent();
-  if (!content->IsMathMLElement())
-    return;
-  nsMathMLElement* element = static_cast<nsMathMLElement*>(content);
-
-  if (element->GetIncrementScriptLevel() == aIncrement)
-    return;
-
-  // XXXroc this does a ContentStatesChanged, is it safe to call here? If
-  // not we should do it in a post-reflow callback.
-  element->SetIncrementScriptLevel(aIncrement, true);
-  PresContext()->PresShell()->PostReflowCallback(&gForceReflow);
-}
-
 // helpers to fix the inter-spacing when <math> is the only parent
 // e.g., it fixes <math> <mi>f</mi> <mo>q</mo> <mi>f</mi> <mo>I</mo> </math>
 
 static nscoord
 GetInterFrameSpacingFor(int32_t         aScriptLevel,
                         nsIFrame*       aParentFrame,
                         nsIFrame*       aChildFrame)
 {
--- a/layout/mathml/nsMathMLContainerFrame.h
+++ b/layout/mathml/nsMathMLContainerFrame.h
@@ -23,17 +23,18 @@
  * to position children in various customized ways.
  */
 
 // Options for the preferred size at which to stretch our stretchy children 
 #define STRETCH_CONSIDER_ACTUAL_SIZE    0x00000001 // just use our current size
 #define STRETCH_CONSIDER_EMBELLISHMENTS 0x00000002 // size calculations include embellishments
 
 class nsMathMLContainerFrame : public nsContainerFrame,
-                               public nsMathMLFrame {
+                               public nsMathMLFrame
+{
   friend class nsMathMLmfencedFrame;
 public:
   nsMathMLContainerFrame(nsStyleContext* aContext)
     : nsContainerFrame(aContext, mozilla::LayoutFrameType::None)
     , mIntrinsicWidth(NS_INTRINSIC_WIDTH_UNKNOWN)
     , mBlockStartAscent(0)
   {}
 
@@ -55,28 +56,16 @@ public:
                                     int32_t         aLastIndex,
                                     uint32_t        aFlagsValues,
                                     uint32_t        aFlagsToUpdate) override
   {
     PropagatePresentationDataFromChildAt(this, aFirstIndex, aLastIndex,
       aFlagsValues, aFlagsToUpdate);
     return NS_OK;
   }
-  
-  // helper to set the "increment script level" flag on the element belonging
-  // to a child frame given by aChildIndex.
-  // When this flag is set, the style system will increment the scriptlevel
-  // for the child element. This is needed for situations where the style system
-  // cannot itself determine the scriptlevel (mfrac, munder, mover, munderover).
-  // This should be called during reflow. We set the flag and if it changed,
-  // we request appropriate restyling and also queue a post-reflow callback
-  // to ensure that restyle and reflow happens immediately after the current
-  // reflow.
-  void
-  SetIncrementScriptLevel(int32_t aChildIndex, bool aIncrement);
 
   // --------------------------------------------------------------------------
   // Overloaded nsContainerFrame methods -- see documentation in nsIFrame.h
 
   virtual bool IsFrameOfType(uint32_t aFlags) const override
   {
     return !(aFlags & nsIFrame::eLineParticipant) &&
       nsContainerFrame::IsFrameOfType(aFlags &
--- a/layout/mathml/nsMathMLmunderoverFrame.cpp
+++ b/layout/mathml/nsMathMLmunderoverFrame.cpp
@@ -2,16 +2,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsMathMLmunderoverFrame.h"
 #include "nsPresContext.h"
 #include "nsRenderingContext.h"
 #include "nsMathMLmmultiscriptsFrame.h"
+#include "nsMathMLElement.h"
 #include <algorithm>
 #include "gfxMathTable.h"
 
 //
 // <munderover> -- attach an underscript-overscript pair to a base - implementation
 // <mover> -- attach an overscript to a base - implementation
 // <munder> -- attach an underscript to a base - implementation
 //
@@ -66,16 +67,25 @@ nsMathMLmunderoverFrame::InheritAutomati
   // let the base class get the default from our parent
   nsMathMLContainerFrame::InheritAutomaticData(aParent);
 
   mPresentationData.flags |= NS_MATHML_STRETCH_ALL_CHILDREN_HORIZONTALLY;
 
   return NS_OK;
 }
 
+void
+nsMathMLmunderoverFrame::DestroyFrom(nsIFrame* aDestroyRoot)
+{
+  if (!mPostReflowIncrementScriptLevelCommands.IsEmpty()) {
+    PresContext()->PresShell()->CancelReflowCallback(this);
+  }
+  nsMathMLContainerFrame::DestroyFrom(aDestroyRoot);
+}
+
 uint8_t
 nsMathMLmunderoverFrame::ScriptIncrement(nsIFrame* aFrame)
 {
   nsIFrame* child = mFrames.FirstChild();
   if (!aFrame || aFrame == child) {
     return 0;
   }
   child = child->GetNextSibling();
@@ -87,16 +97,71 @@ nsMathMLmunderoverFrame::ScriptIncrement
   }
   if (child && aFrame == child->GetNextSibling()) {
     // must be a over frame of munderover
     return mIncrementOver ? 1 : 0;
   }
   return 0;  // frame not found
 }
 
+void
+nsMathMLmunderoverFrame::SetIncrementScriptLevel(uint32_t aChildIndex,
+                                                 bool aIncrement)
+{
+  nsIFrame* child = PrincipalChildList().FrameAt(aChildIndex);
+  if (!child || !child->GetContent()->IsMathMLElement()) {
+    return;
+  }
+
+  auto element = static_cast<nsMathMLElement*>(child->GetContent());
+  if (element->GetIncrementScriptLevel() == aIncrement) {
+    return;
+  }
+
+  if (mPostReflowIncrementScriptLevelCommands.IsEmpty()) {
+    PresContext()->PresShell()->PostReflowCallback(this);
+  }
+
+  mPostReflowIncrementScriptLevelCommands.AppendElement(
+      SetIncrementScriptLevelCommand { aChildIndex, aIncrement });
+}
+
+bool
+nsMathMLmunderoverFrame::ReflowFinished()
+{
+  SetPendingPostReflowIncrementScriptLevel();
+  return true;
+}
+
+void
+nsMathMLmunderoverFrame::ReflowCallbackCanceled()
+{
+  // Do nothing, at this point our work will just be useless.
+  mPostReflowIncrementScriptLevelCommands.Clear();
+}
+
+void
+nsMathMLmunderoverFrame::SetPendingPostReflowIncrementScriptLevel()
+{
+  MOZ_ASSERT(!mPostReflowIncrementScriptLevelCommands.IsEmpty());
+
+  nsTArray<SetIncrementScriptLevelCommand> commands;
+  commands.SwapElements(mPostReflowIncrementScriptLevelCommands);
+
+  for (const auto& command : commands) {
+    nsIFrame* child = PrincipalChildList().FrameAt(command.mChildIndex);
+    if (!child || !child->GetContent()->IsMathMLElement()) {
+      continue;
+    }
+
+    auto element = static_cast<nsMathMLElement*>(child->GetContent());
+    element->SetIncrementScriptLevel(command.mDoIncrement, true);
+  }
+}
+
 NS_IMETHODIMP
 nsMathMLmunderoverFrame::TransmitAutomaticData()
 {
   // At this stage, all our children are in sync and we can fully
   // resolve our own mEmbellishData struct
   //---------------------------------------------------------------------
 
   /* 
@@ -158,17 +223,17 @@ XXX The winner is the outermost setting 
   nsAutoString value;
   if (mContent->IsAnyOfMathMLElements(nsGkAtoms::munder_,
                                       nsGkAtoms::munderover_)) {
     GetEmbellishDataFrom(underscriptFrame, embellishData);
     if (NS_MATHML_EMBELLISH_IS_ACCENT(embellishData.flags)) {
       mEmbellishData.flags |= NS_MATHML_EMBELLISH_ACCENTUNDER;
     } else {
       mEmbellishData.flags &= ~NS_MATHML_EMBELLISH_ACCENTUNDER;
-    }    
+    }
 
     // if we have an accentunder attribute, it overrides what the underscript said
     if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accentunder_, value)) {
       if (value.EqualsLiteral("true")) {
         mEmbellishData.flags |= NS_MATHML_EMBELLISH_ACCENTUNDER;
       } else if (value.EqualsLiteral("false")) {
         mEmbellishData.flags &= ~NS_MATHML_EMBELLISH_ACCENTUNDER;
       }
@@ -227,17 +292,18 @@ XXX The winner is the outermost setting 
   */
   if (mContent->IsAnyOfMathMLElements(nsGkAtoms::mover_,
                                       nsGkAtoms::munderover_)) {
     uint32_t compress = NS_MATHML_EMBELLISH_IS_ACCENTOVER(mEmbellishData.flags)
       ? NS_MATHML_COMPRESSED : 0;
     mIncrementOver =
       !NS_MATHML_EMBELLISH_IS_ACCENTOVER(mEmbellishData.flags) ||
       subsupDisplay;
-    SetIncrementScriptLevel(mContent->IsMathMLElement(nsGkAtoms::mover_) ? 1 : 2, mIncrementOver);
+    SetIncrementScriptLevel(
+        mContent->IsMathMLElement(nsGkAtoms::mover_) ? 1 : 2, mIncrementOver);
     if (mIncrementOver) {
       PropagateFrameFlagFor(overscriptFrame,
                             NS_FRAME_MATHML_SCRIPT_DESCENDANT);
     }
     PropagatePresentationDataFor(overscriptFrame, compress, compress);
   }
   /*
      The TeXBook treats 'under' like a subscript, so p.141 or Rule 13a 
--- a/layout/mathml/nsMathMLmunderoverFrame.h
+++ b/layout/mathml/nsMathMLmunderoverFrame.h
@@ -2,56 +2,88 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef nsMathMLmunderoverFrame_h___
 #define nsMathMLmunderoverFrame_h___
 
 #include "mozilla/Attributes.h"
+#include "nsIReflowCallback.h"
 #include "nsMathMLContainerFrame.h"
 
 //
 // <munderover> -- attach an underscript-overscript pair to a base
 //
 
-class nsMathMLmunderoverFrame : public nsMathMLContainerFrame {
+class nsMathMLmunderoverFrame final
+  : public nsMathMLContainerFrame
+  , public nsIReflowCallback
+{
+
 public:
   NS_DECL_FRAMEARENA_HELPERS
 
   friend nsIFrame* NS_NewMathMLmunderoverFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
 
-  virtual nsresult
-  Place(DrawTarget*          aDrawTarget,
-        bool                 aPlaceOrigin,
-        ReflowOutput& aDesiredSize) override;
+  nsresult Place(DrawTarget* aDrawTarget,
+                 bool aPlaceOrigin,
+                 ReflowOutput& aDesiredSize) override;
 
-  NS_IMETHOD
-  InheritAutomaticData(nsIFrame* aParent) override;
+  NS_IMETHOD InheritAutomaticData(nsIFrame* aParent) override;
 
-  NS_IMETHOD
-  TransmitAutomaticData() override;
+  NS_IMETHOD TransmitAutomaticData() override;
+
+  NS_IMETHOD UpdatePresentationData(uint32_t aFlagsValues,
+                                    uint32_t aFlagsToUpdate) override;
 
-  NS_IMETHOD
-  UpdatePresentationData(uint32_t        aFlagsValues,
-                         uint32_t        aFlagsToUpdate) override;
+  void DestroyFrom(nsIFrame* aRoot) override;
+
+  nsresult AttributeChanged(int32_t aNameSpaceID,
+                            nsIAtom* aAttribute,
+                            int32_t aModType) override;
 
-  virtual nsresult
-  AttributeChanged(int32_t         aNameSpaceID,
-                   nsIAtom*        aAttribute,
-                   int32_t         aModType) override;
+  uint8_t ScriptIncrement(nsIFrame* aFrame) override;
 
-  uint8_t
-  ScriptIncrement(nsIFrame* aFrame) override;
+  // nsIReflowCallback.
+  bool ReflowFinished() override;
+  void ReflowCallbackCanceled() override;
 
 protected:
-  explicit nsMathMLmunderoverFrame(nsStyleContext* aContext) : nsMathMLContainerFrame(aContext),
-                                                               mIncrementUnder(false),
-                                                               mIncrementOver(false) {}
+  explicit nsMathMLmunderoverFrame(nsStyleContext* aContext)
+    : nsMathMLContainerFrame(aContext)
+    , mIncrementUnder(false)
+    , mIncrementOver(false)
+  {}
+
   virtual ~nsMathMLmunderoverFrame();
 
 private:
+  // Helper to set the "increment script level" flag on the element belonging
+  // to a child frame given by aChildIndex.
+  //
+  // When this flag is set, the style system will increment the scriptlevel for
+  // the child element. This is needed for situations where the style system
+  // cannot itself determine the scriptlevel (mfrac, munder, mover, munderover).
+  //
+  // This should be called during reflow.
+  //
+  // We set the flag and if it changed, we request appropriate restyling and
+  // also queue a post-reflow callback to ensure that restyle and reflow happens
+  // immediately after the current reflow.
+  void SetIncrementScriptLevel(uint32_t aChildIndex, bool aIncrement);
+  void SetPendingPostReflowIncrementScriptLevel();
+
   bool mIncrementUnder;
   bool mIncrementOver;
+
+  struct SetIncrementScriptLevelCommand
+  {
+    uint32_t mChildIndex;
+    bool mDoIncrement;
+  };
+
+  nsTArray<SetIncrementScriptLevelCommand>
+    mPostReflowIncrementScriptLevelCommands;
 };
 
 
 #endif /* nsMathMLmunderoverFrame_h___ */
--- a/layout/reftests/mathml/reftest.list
+++ b/layout/reftests/mathml/reftest.list
@@ -104,19 +104,19 @@ fails-if(!stylo) == stretchy-mover-2a.ht
 != embellished-op-2-2.html embellished-op-2-2-ref.html
 != embellished-op-2-3.html embellished-op-2-3-ref.html
 != embellished-op-2-4.html embellished-op-2-4-ref.html
 != embellished-op-3-1.html embellished-op-3-1-ref.html
 != embellished-op-3-2.html embellished-op-3-2-ref.html
 != embellished-op-3-3.html embellished-op-3-3-ref.html
 != embellished-op-3-4.html embellished-op-3-4-ref.html
 != embellished-op-3-5.html embellished-op-3-5-ref.html
-fails-if(stylo) == embellished-op-4-1.html embellished-op-4-1-ref.html
-fails-if(stylo) == embellished-op-4-2.html embellished-op-4-2-ref.html
-fails-if(stylo) == embellished-op-4-3.html embellished-op-4-3-ref.html
+== embellished-op-4-1.html embellished-op-4-1-ref.html
+== embellished-op-4-2.html embellished-op-4-2-ref.html
+== embellished-op-4-3.html embellished-op-4-3-ref.html
 == embellished-op-5-1.html embellished-op-5-ref.html
 == embellished-op-5-2.html embellished-op-5-ref.html
 fails-if(gtkWidget&&!stylo) random-if(winWidget) == semantics-1.xhtml semantics-1-ref.xhtml # bug 1309429, bug 1328771
 == semantics-2.html semantics-2-ref.html
 == semantics-3.html semantics-3-ref.html
 == semantics-4.html semantics-4-ref.html
 != mathcolor-1.xml mathcolor-1-ref.xml
 != mathcolor-2.xml mathcolor-2-ref.xml
@@ -131,17 +131,17 @@ fails-if(gtkWidget&&!stylo) random-if(wi
 == mstyle-3.xhtml mstyle-3-ref.xhtml
 == mstyle-4.xhtml mstyle-4-ref.xhtml
 == mstyle-5.xhtml mstyle-5-ref.xhtml # Bug 787215
 == scale-stretchy-1.xhtml scale-stretchy-1-ref.xhtml
 != scale-stretchy-2.xhtml scale-stretchy-2-ref.xhtml
 fails-if(skiaContent&&OSX>=1010) == scale-stretchy-3.xhtml scale-stretchy-3-ref.xhtml
 != scale-stretchy-4.xhtml scale-stretchy-4-ref.xhtml
 != scale-stretchy-5.xhtml scale-stretchy-5-ref.xhtml
-fails-if(stylo) != stretchy-1.html stretchy-1-ref.html
+!= stretchy-1.html stretchy-1-ref.html
 == mspace-1.html mspace-1-ref.html
 == mpadded-1.html mpadded-1-ref.html
 == mpadded-2.html mpadded-2-ref.html
 == mpadded-3.html mpadded-3-ref.html
 == mpadded-4.html mpadded-4-ref.html
 == mpadded-5.html mpadded-5-ref.html
 == mpadded-1-2.html mpadded-1-2-ref.html
 == mpadded-6.html mpadded-6-ref.html
@@ -335,17 +335,17 @@ random-if(gtkWidget) == rowlines-3-2.htm
 != op-dict-3.html op-dict-3-ref.html
 == op-dict-4.html op-dict-4-ref.html
 != op-dict-5.html op-dict-5-ref.html
 == op-dict-6.html op-dict-6-ref.html
 != op-dict-7.html op-dict-7-ref.html
 == op-dict-8.html op-dict-8-ref.html
 != op-dict-9.html op-dict-9-ref.html
 == op-dict-10.html op-dict-10-ref.html
-fails-if(stylo) != op-dict-11.html op-dict-11-ref.html
+!= op-dict-11.html op-dict-11-ref.html
 == op-dict-12.html op-dict-12-ref.html
 != op-dict-13.html op-dict-13-ref.html
 == mfrac-A-1.html mfrac-A-1-ref.html
 == mfrac-A-2.html mfrac-A-2-ref.html
 == mfrac-A-3.html mfrac-A-3-ref.html
 == mfrac-A-4.html mfrac-A-4-ref.html
 == mfrac-A-5.html mfrac-A-5-ref.html
 == mfrac-A-6.html mfrac-A-6-ref.html