Bug 1624247 part 1: When caching flex item BSize measurement, cache the content-box size instead of border-box size. r=TYLin
authorDaniel Holbert <dholbert@cs.stanford.edu>
Wed, 25 Mar 2020 04:51:39 +0000
changeset 520340 782a1cdba0ae15750c7b6eae8957c0d1cb36a861
parent 520339 d93b4a6d9bd4eefe81f85c758226a34e4f68b188
child 520341 7d8eddd3ede1babf661c95a672d0188b4af221c0
push id37248
push userbtara@mozilla.com
push dateWed, 25 Mar 2020 16:40:49 +0000
treeherdermozilla-central@c5112a7573ac [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersTYLin
bugs1624247
milestone76.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 1624247 part 1: When caching flex item BSize measurement, cache the content-box size instead of border-box size. r=TYLin This patch should not impact behavior; it is purely a refactoring change to do the same arithmetic in a different spot. The content-box size is the size that we actually care about when we use this cached data, so we might as well just store that size directly, instead of storing the border-box size and making adjustments at usage time, which is what we do right now. Note that one of the usage sites had an informational NS_WARNING_ASSERTION, for cases where the BorderPadding subtraction might drop us into negative territory. I'm not particularly concerned with preserving that warning at this point (in part because it's not necessarily an error when it fails, since it's possible to make it fail with huge sizes; and in part because it's non-fatal, which means we're not likely to notice it if it did "legitimately" fail anyway). So I've just removed it for simplicity & consistency. Differential Revision: https://phabricator.services.mozilla.com/D67800
layout/generic/nsFlexContainerFrame.cpp
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -1631,17 +1631,17 @@ void nsFlexContainerFrame::ResolveAutoFl
  * We store the cached value in the flex item's frame property table, for
  * simplicity.
  *
  * Right now, we cache the following as a "key", from the item's ReflowInput:
  *   - its ComputedSize
  *   - its min/max block size (in case its ComputedBSize is unconstrained)
  *   - its AvailableBSize
  * ...and we cache the following as the "value", from the item's ReflowOutput:
- *   - its final BSize
+ *   - its final content-box BSize
  *   - its ascent
  *
  * The assumption here is that a given flex item measurement from our "value"
  * won't change unless one of the pieces of the "key" change, or the flex
  * item's intrinsic size is marked as dirty (due to a style or DOM change).
  * (The latter will cause the cached value to be discarded, in
  * nsFrame::MarkIntrinsicISizesDirty.)
  *
@@ -1688,19 +1688,25 @@ class nsFlexContainerFrame::CachedMeasur
   Key mKey;
 
   nscoord mBSize;
   nscoord mAscent;
 
  public:
   CachedMeasuringReflowResult(const ReflowInput& aReflowInput,
                               const ReflowOutput& aReflowOutput)
-      : mKey(aReflowInput),
-        mBSize(aReflowOutput.BSize(aReflowInput.GetWritingMode())),
-        mAscent(aReflowOutput.BlockStartAscent()) {}
+      : mKey(aReflowInput), mAscent(aReflowOutput.BlockStartAscent()) {
+    // To get content-box bsize, we have to subtract off border & padding
+    // (and floor at 0 in case the border/padding are too large):
+    WritingMode itemWM = aReflowInput.GetWritingMode();
+    nscoord borderBoxBSize = aReflowOutput.BSize(itemWM);
+    mBSize = borderBoxBSize -
+             aReflowInput.ComputedLogicalBorderPadding().BStartEnd(itemWM);
+    mBSize = std::max(0, mBSize);
+  }
 
   /**
    * Returns true if this cached flex item measurement is valid for (i.e. can
    * be expected to match the output of) a measuring reflow whose input
    * parameters are given via aReflowInput.
    */
   bool IsValidFor(const ReflowInput& aReflowInput) const {
     return mKey == Key(aReflowInput);
@@ -1811,24 +1817,17 @@ nscoord nsFlexContainerFrame::MeasureFle
     // Not 100% sure this is needed, but be conservative for now:
     childRIForMeasuringBSize.mFlags.mIsBResizeForPercentages = true;
   }
 
   const CachedMeasuringReflowResult& reflowResult =
       MeasureAscentAndBSizeForFlexItem(aFlexItem, childRIForMeasuringBSize);
 
   aFlexItem.SetAscent(reflowResult.Ascent());
-
-  // Subtract border/padding in block axis, to get _just_
-  // the effective computed value of the BSize property.
-  nscoord childDesiredBSize =
-      reflowResult.BSize() -
-      childRIForMeasuringBSize.ComputedLogicalBorderPadding().BStartEnd(wm);
-
-  return std::max(0, childDesiredBSize);
+  return reflowResult.BSize();
 }
 
 FlexItem::FlexItem(ReflowInput& aFlexItemReflowInput, float aFlexGrow,
                    float aFlexShrink, nscoord aFlexBaseSize,
                    nscoord aMainMinSize, nscoord aMainMaxSize,
                    nscoord aTentativeCrossSize, nscoord aCrossMinSize,
                    nscoord aCrossMaxSize,
                    const FlexboxAxisTracker& aAxisTracker)
@@ -4025,35 +4024,17 @@ void nsFlexContainerFrame::SizeItemInCro
   // Potentially reflow the item, and get the sizing info.
   const CachedMeasuringReflowResult& reflowResult =
       MeasureAscentAndBSizeForFlexItem(aItem, aChildReflowInput);
 
   // Save the sizing info that we learned from this reflow
   // -----------------------------------------------------
 
   // Tentatively store the child's desired content-box cross-size.
-  // Note that childReflowOutput is the border-box size, so we have to
-  // subtract border & padding to get the content-box size.
-  nscoord crossAxisBorderPadding = aItem.BorderPaddingSizeInCrossAxis();
-  if (reflowResult.BSize() < 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()->Type() == LayoutFrameType::None,
-        "Child should at least request space for border/padding");
-    aItem.SetCrossSize(0);
-  } else {
-    // (normal case)
-    aItem.SetCrossSize(reflowResult.BSize() - crossAxisBorderPadding);
-  }
-
+  aItem.SetCrossSize(reflowResult.BSize());
   aItem.SetAscent(reflowResult.Ascent());
 }
 
 void FlexLine::PositionItemsInCrossAxis(
     nscoord aLineStartPosition, const FlexboxAxisTracker& aAxisTracker) {
   SingleLineCrossAxisPositionTracker lineCrossAxisPosnTracker(aAxisTracker);
 
   for (FlexItem& item : Items()) {