Bug 1336708 - Don't reuse cached flex-item reflow measurements if the item's computed height has changed. r=dholbert, a=lizzard
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 06 Feb 2017 13:06:57 +0100
changeset 378554 13018e92b17958dcde86fb4780a543c33129d056
parent 378553 4f962e1b4f853341fea61b971c5611fb20d91091
child 378555 a26a6726bf710f3568d65ecb864efa478674fe58
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert, lizzard
bugs1336708
milestone53.0a2
Bug 1336708 - Don't reuse cached flex-item reflow measurements if the item's computed height has changed. r=dholbert, a=lizzard For some stretched items, we override the computed height of the input for measuring reflows, which may change the ascent and height result. Just use that as a key for the reflow result cache too. MozReview-Commit-ID: 9NyObfVucnC
layout/generic/nsFlexContainerFrame.cpp
layout/generic/nsFlexContainerFrame.h
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -1767,56 +1767,74 @@ nsFlexContainerFrame::
   if (isMainMinSizeAuto) {
     aFlexItem.UpdateMainMinSize(resolvedMinSize);
   }
 }
 
 /**
  * A cached result for a measuring reflow.
  *
- * Right now we only need to cache the available size, the height and the
- * ascent. This can be extended later if needed.
+ * Right now we only need to cache the available size and the computed height
+ * for checking that the reflow input is valid, and the height and the ascent
+ * to be used. This can be extended later if needed.
  *
  * The assumption here is that a given flex item measurement won't change until
- * either the available size changes, or the flex container intrinsic size is
- * marked as dirty (due to a style or DOM change).
+ * either the available size or computed height changes, or the flex container
+ * intrinsic size is marked as dirty (due to a style or DOM change).
+ *
+ * In particular the computed height may change between measuring reflows due to
+ * how the mIsFlexContainerMeasuringReflow flag affects size computation (see
+ * bug 1336708).
  *
  * Caching it prevents us from doing exponential reflows in cases of deeply
  * nested flex and scroll frames.
  *
  * We store them in the frame property table for simplicity.
  */
-struct nsFlexContainerFrame::CachedMeasuringReflowResult
+class nsFlexContainerFrame::CachedMeasuringReflowResult
 {
-  LogicalSize mAvailableSize;
+  // Members that are part of the cache key:
+  const LogicalSize mAvailableSize;
+  const nscoord mComputedHeight;
+
+  // Members that are part of the cache value:
   const nscoord mHeight;
   const nscoord mAscent;
 
-  CachedMeasuringReflowResult(const LogicalSize& aAvailableSize,
-                              nscoord aHeight,
-                              nscoord aAscent)
-    : mAvailableSize(aAvailableSize)
-    , mHeight(aHeight)
-    , mAscent(aAscent)
+public:
+  CachedMeasuringReflowResult(const ReflowInput& aReflowInput,
+                              const ReflowOutput& aDesiredSize)
+    : mAvailableSize(aReflowInput.AvailableSize())
+    , mComputedHeight(aReflowInput.ComputedHeight())
+    , mHeight(aDesiredSize.Height())
+    , mAscent(aDesiredSize.BlockStartAscent())
   {}
+
+  bool IsValidFor(const ReflowInput& aReflowInput) const {
+    return mAvailableSize == aReflowInput.AvailableSize() &&
+      mComputedHeight == aReflowInput.ComputedHeight();
+  }
+
+  nscoord Height() const { return mHeight; }
+
+  nscoord Ascent() const { return mAscent; }
 };
 
 NS_DECLARE_FRAME_PROPERTY_DELETABLE(CachedFlexMeasuringReflow,
                                     CachedMeasuringReflowResult);
 
 const CachedMeasuringReflowResult&
 nsFlexContainerFrame::MeasureAscentAndHeightForFlexItem(
   FlexItem& aItem,
   nsPresContext* aPresContext,
   ReflowInput& aChildReflowInput)
 {
-  const auto availableSize = aChildReflowInput.AvailableSize();
   const FrameProperties props = aItem.Frame()->Properties();
   if (const auto* cachedResult = props.Get(CachedFlexMeasuringReflow())) {
-    if (cachedResult->mAvailableSize == availableSize) {
+    if (cachedResult->IsValidFor(aChildReflowInput)) {
       return *cachedResult;
     }
   }
 
   ReflowOutput childDesiredSize(aChildReflowInput);
   nsReflowStatus childReflowStatus;
 
   const uint32_t flags = NS_FRAME_NO_MOVE_FRAME;
@@ -1833,19 +1851,17 @@ nsFlexContainerFrame::MeasureAscentAndHe
              "should be complete");
 
   // Tell the child we're done with its initial reflow.
   // (Necessary for e.g. GetBaseline() to work below w/out asserting)
   FinishReflowChild(aItem.Frame(), aPresContext,
                     childDesiredSize, &aChildReflowInput, 0, 0, flags);
 
   auto result =
-    new CachedMeasuringReflowResult(availableSize,
-                                    childDesiredSize.Height(),
-                                    childDesiredSize.BlockStartAscent());
+    new CachedMeasuringReflowResult(aChildReflowInput, childDesiredSize);
 
   props.Set(CachedFlexMeasuringReflow(), result);
   return *result;
 }
 
 /* virtual */ void
 nsFlexContainerFrame::MarkIntrinsicISizesDirty()
 {
@@ -1880,21 +1896,21 @@ nsFlexContainerFrame::
   if (aForceVerticalResizeForMeasuringReflow) {
     childRIForMeasuringHeight.SetVResize(true);
   }
 
   const CachedMeasuringReflowResult& reflowResult =
     MeasureAscentAndHeightForFlexItem(aFlexItem, aPresContext,
                                       childRIForMeasuringHeight);
 
-  aFlexItem.SetAscent(reflowResult.mAscent);
+  aFlexItem.SetAscent(reflowResult.Ascent());
 
   // Subtract border/padding in vertical axis, to get _just_
   // the effective computed value of the "height" property.
-  nscoord childDesiredHeight = reflowResult.mHeight -
+  nscoord childDesiredHeight = reflowResult.Height() -
     childRIForMeasuringHeight.ComputedPhysicalBorderPadding().TopBottom();
 
   return std::max(0, childDesiredHeight);
 }
 
 FlexItem::FlexItem(ReflowInput& aFlexItemReflowInput,
                    float aFlexGrow, float aFlexShrink, nscoord aFlexBaseSize,
                    nscoord aMainMinSize,  nscoord aMainMaxSize,
@@ -4055,33 +4071,33 @@ nsFlexContainerFrame::SizeItemInCrossAxi
 
   // Tentatively store the child's desired content-box cross-size.
   // Note that childDesiredSize is the border-box size, so we have to
   // subtract border & padding to get the content-box size.
   // (Note that at this point in the code, we know our cross axis is vertical,
   // so we don't bother with making aAxisTracker pick the cross-axis component
   // for us.)
   nscoord crossAxisBorderPadding = aItem.GetBorderPadding().TopBottom();
-  if (reflowResult.mHeight < crossAxisBorderPadding) {
+  if (reflowResult.Height() < crossAxisBorderPadding) {
     // Child's requested size isn't large enough for its border/padding!
     // This is OK for the trivial nsFrame::Reflow() impl, but other frame
     // classes should know better. So, if we get here, the child had better be
     // an instance of nsFrame (i.e. it should return null from GetType()).
     // XXXdholbert Once we've fixed bug 765861, we should upgrade this to an
     // assertion that trivially passes if bug 765861's flag has been flipped.
     NS_WARNING_ASSERTION(
       !aItem.Frame()->GetType(),
       "Child should at least request space for border/padding");
     aItem.SetCrossSize(0);
   } else {
     // (normal case)
-    aItem.SetCrossSize(reflowResult.mHeight - crossAxisBorderPadding);
+    aItem.SetCrossSize(reflowResult.Height() - crossAxisBorderPadding);
   }
 
-  aItem.SetAscent(reflowResult.mAscent);
+  aItem.SetAscent(reflowResult.Ascent());
 }
 
 void
 FlexLine::PositionItemsInCrossAxis(nscoord aLineStartPosition,
                                    const FlexboxAxisTracker& aAxisTracker)
 {
   SingleLineCrossAxisPositionTracker lineCrossAxisPosnTracker(aAxisTracker);
 
--- a/layout/generic/nsFlexContainerFrame.h
+++ b/layout/generic/nsFlexContainerFrame.h
@@ -51,17 +51,17 @@ public:
   friend nsContainerFrame* NS_NewFlexContainerFrame(nsIPresShell* aPresShell,
                                                     nsStyleContext* aContext);
 
   // Forward-decls of helper classes
   class FlexItem;
   class FlexLine;
   class FlexboxAxisTracker;
   struct StrutInfo;
-  struct CachedMeasuringReflowResult;
+  class CachedMeasuringReflowResult;
 
   // nsIFrame overrides
   void Init(nsIContent*       aContent,
             nsContainerFrame* aParent,
             nsIFrame*         aPrevInFlow) override;
 
   void BuildDisplayList(nsDisplayListBuilder*   aBuilder,
                         const nsRect&           aDirtyRect,