Bug 1500608: Don't skip the flex-item early-freeze for devtools after all. r=bradwerth
authorDaniel Holbert <dholbert@cs.stanford.edu>
Fri, 19 Oct 2018 22:05:46 +0000
changeset 490556 9217bce3d56119840999271a3627f8a4b55b9d9d
parent 490555 a14911472fe733a76c7703a29e35440f8f1c75cd
child 490557 a7172b2e62e0cdcf7c492c06b21f08be13d093b4
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersbradwerth
bugs1500608
milestone64.0a1
Bug 1500608: Don't skip the flex-item early-freeze for devtools after all. r=bradwerth This patch basically reverts the functional part of changeset 52bd865d757c. I'd optimistically hoped we could skip this early-freeze in order to compute & report a bit more "potential flexing" information via devtools. Bbut it turns out that breaks assertions & produces bogus information for flex items whose base size vs. min/max-clamped "hypothetical" sizes are very different. (Specifically: it produces nonsense for flex items whose base sizes, if unclamped, would reverse the directionality of flexing.) Differential Revision: https://phabricator.services.mozilla.com/D9304
dom/flex/test/chrome/test_flex_items.html
layout/generic/nsFlexContainerFrame.cpp
--- a/dom/flex/test/chrome/test_flex_items.html
+++ b/dom/flex/test/chrome/test_flex_items.html
@@ -10,16 +10,23 @@
     background-color: grey;
     font: 14px sans-serif;
     height: 50px;
   }
   #flex-sanity {
     /* This just needs to be large enough so that no shrinking is required. */
     width: 1600px;
   }
+  .clamped-huge-item {
+    flex: 1 1 1650px; /* This needs to be bigger than #flex-sanity, to test
+                         the scenario it's aiming to test (early clamping of a
+                         flex item which, if left unclamped, would've reversed
+                         the direction of flexing & made everything shrink). */
+    max-width: 10px;
+  }
 
   .base        { align-self: baseline; }
   .lastbase    { align-self: last baseline; }
 
   .offset      { margin-top: 10px;
                  margin-bottom: 3px; }
 
   .lime        { background: lime;   }
@@ -139,16 +146,19 @@ function testFlexSanity() {
     { mainBaseSize:  10,
       mainMaxSize:   5,
       mainDeltaSize: 0 },
     { mainBaseSize: 10,
       mainMinSize:  15,
       mainDeltaSize: 0 },
     { mainBaseSize: 50,
       mainMaxSize: 10 },
+    { mainBaseSize: 1650,
+      mainMaxSize: 10,
+      mainDeltaSize: 0 },
     { mainDeltaSize: 0 },
     { /* final item is anonymous flex item */ },
   ];
 
   let items = line.getItems();
   is(items.length, expectedValues.length,
      "Line should have expected number of items.");
   is(items.length, container.children.length + 1,
@@ -236,16 +246,17 @@ function runTests() {
     </div>
     <!-- Inflexible item that is trivially clamped to smaller max-width: -->
     <div style="flex: 0 0 10px; max-width: 5px"></div>
     <!-- Inflexible item that is trivially clamped to larger min-width: -->
     <div style="flex: 0 0 10px; min-width: 15px"></div>
     <!-- Item that wants to grow but is trivially clamped to max-width
          below base size: -->
     <div style="flex: 1 1 50px; max-width: 10px"></div>
+    <div class="clamped-huge-item"></div>
     <div style="display:contents">
       <div class="white">replaced</div>
     </div>
     anon item for text
   </div>
 
   <!-- Second flex container to be tested, with items that grow by specific
        predictable amounts. This ends up triggering 4 passes of the main
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -2529,23 +2529,17 @@ FlexLine::FreezeItemsEarly(bool aIsUsing
   uint32_t numUnfrozenItemsToBeSeen = mNumItems - mNumFrozenItems;
   for (FlexItem* item = mItems.getFirst();
        numUnfrozenItemsToBeSeen > 0; item = item->getNext()) {
     MOZ_ASSERT(item, "numUnfrozenItemsToBeSeen says items remain to be seen");
 
     if (!item->IsFrozen()) {
       numUnfrozenItemsToBeSeen--;
       bool shouldFreeze = (0.0f == item->GetFlexFactor(aIsUsingFlexGrow));
-      // NOTE: We skip the "could flex but base size out of range"
-      // early-freezing if flex devtools are active, so that we can let the
-      // first run of the main flex layout loop compute how much this item
-      // wants to flex. (This skipping shouldn't impact results, because
-      // any affected items will just immediately be caught & frozen as min/max
-      // violations in that first loop, and that'll trigger another loop.)
-      if (!shouldFreeze && !aLineInfo) {
+      if (!shouldFreeze) {
         if (aIsUsingFlexGrow) {
           if (item->GetFlexBaseSize() > item->GetMainSize()) {
             shouldFreeze = true;
           }
         } else { // using flex-shrink
           if (item->GetFlexBaseSize() < item->GetMainSize()) {
             shouldFreeze = true;
           }