Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis. r=mats
authorDaniel Holbert <dholbert@cs.stanford.edu>
Thu, 29 Mar 2018 11:46:27 -0700
changeset 410734 e974a8ab639efe84bbf97de0379e9e41b34deac7
parent 410707 4543c335c5d8629ffca8cc8f4c0fe46ca13e6f1b
child 410735 35da0017040ef09e5d5534857109f163e17068d7
push id33736
push usershindli@mozilla.com
push dateFri, 30 Mar 2018 09:56:41 +0000
treeherdermozilla-central@b7fa9d95150e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1436881
milestone61.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 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis. r=mats This patch should not affect behavior. Logic-wise: the idea behind this patch is to behave as if the 'usingFlexBasisForHeight' variable were always false, which in turn lets us remove an "if (!usingFlexBasisForHeight || ...)" check, because it trivially passes when that bool is false. Background on this special case & why we can remove it: ======================================================= In the original flexbox implementation, we had some special-case code to be sure we didn't end up swapping in e.g. "flex-basis:-moz-min-content" for "height" in these ComputeSize functions, because that was a scenario that previously would've been prevented at the parser level (height:-moz-min-content is rejected for now), and hence may not have ended up being handled robustly. However, nowadays it'll be handled just fine in these functions, and will produce the same result as our special-case exception tries to achieve. In particular, for a nsFrame that is a flex item in a flex container with a vertical main axis (the scenario that these special cases are catching): - If the (vertical) main axis is this nsFrame's inline axis (i.e. if this nsFrame has a vertical writing-mode), then Stylo actually converts enumerated flex-basis values like "-moz-min-content" to "auto" when producing the computed values that layout sees. So it's not actually possible for layout to see a computed "flex-basis" of -moz-min-content, in that scenario. - Otherwise, i.e. if the (vertical) main axis is this nsFrame's block axis, then these ComputeSize functions will now end up getting an enumerated "blockStyleCoord" (really pointing to flexBasis), but that'll still end up being treated like 'auto'. This happens by virtue of ComputeSize's calls to ComputeAutoSize (which initializes the tentative bsize value to NS_UNCONSTRAINEDSIZE) and to nsLayoutUtils::IsAutoBSize (which returns "true" for eStyleUnit_Enumerated values and then makes us leave the ComputeAutoSize result unperturbed).
layout/generic/nsFrame.cpp
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -5692,31 +5692,19 @@ nsFrame::ComputeSize(gfxContext*        
     usingFlexBasisForISize =
       (flexContainerIsRowOriented == inlineAxisSameAsParent);
 
     // NOTE: The logic here should match the similar chunk for determining
     // inlineStyleCoord and blockStyleCoord in
     // nsFrame::ComputeSizeWithIntrinsicDimensions().
     const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
     if (flexBasis->GetUnit() != eStyleUnit_Auto) {
-      // One caveat for when flex-basis is stomping on 'height': We don't
-      // support enumerated values (e.g. "max-content") for height yet (that's
-      // bug 567039). So, if our computed flex-basis is an enumerated value,
-      // we'll just behave as if it were "auto", which means "use the main-size
-      // property after all" (which is "height", in this case).  NOTE: Once we
-      // support intrinsic sizing keywords for "height", we should remove this
-      // check.
-      bool usingFlexBasisForHeight =
-        (usingFlexBasisForISize != aWM.IsVertical());
-      if (!usingFlexBasisForHeight ||
-          flexBasis->GetUnit() != eStyleUnit_Enumerated) {
-        // Override whichever coord we're overriding:
-        (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
-          flexBasis;
-      }
+      // Override whichever styleCoord is in flex container's main axis:
+      (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
+        flexBasis;
     }
   }
 
   // Compute inline-axis size
   if (inlineStyleCoord->GetUnit() != eStyleUnit_Auto) {
     result.ISize(aWM) =
       ComputeISizeValue(aRenderingContext, aCBSize.ISize(aWM),
                         boxSizingAdjust.ISize(aWM), boxSizingToMarginEdgeISize,
@@ -5955,31 +5943,19 @@ nsFrame::ComputeSizeWithIntrinsicDimensi
       // Flex items use their "flex-basis" property in place of their main-size
       // property (e.g. "width") for sizing purposes, *unless* they have
       // "flex-basis:auto", in which case they use their main-size property
       // after all.
       // NOTE: The logic here should match the similar chunk for determining
       // inlineStyleCoord and blockStyleCoord in nsFrame::ComputeSize().
       const nsStyleCoord* flexBasis = &(stylePos->mFlexBasis);
       if (flexBasis->GetUnit() != eStyleUnit_Auto) {
-        // One caveat for when flex-basis is stomping on 'height': We don't
-        // support enumerated values (e.g. "max-content") for height yet
-        // (that's bug 567039). So, if our computed flex-basis is an enumerated
-        // value, we'll just behave as if it were "auto", which means "use the
-        // main-size property after all" (which is "height", in this case).
-        // NOTE: Once we support intrinsic sizing keywords for "height", we
-        // should remove this check.
-        bool usingFlexBasisForHeight =
-          (usingFlexBasisForISize != aWM.IsVertical());
-        if (!usingFlexBasisForHeight ||
-            flexBasis->GetUnit() != eStyleUnit_Enumerated) {
-          // Override whichever coord we're overriding:
-          (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
-            flexBasis;
-        }
+        // Override whichever styleCoord is in flex container's main axis:
+        (usingFlexBasisForISize ? inlineStyleCoord : blockStyleCoord) =
+          flexBasis;
       }
     }
   }
 
   // Handle intrinsic sizes and their interaction with
   // {min-,max-,}{width,height} according to the rules in
   // http://www.w3.org/TR/CSS21/visudet.html#min-max-widths