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 500720 9217bce3d56119840999271a3627f8a4b55b9d9d
parent 500719 a14911472fe733a76c7703a29e35440f8f1c75cd
child 500721 a7172b2e62e0cdcf7c492c06b21f08be13d093b4
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbradwerth
bugs1500608
milestone64.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 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;
           }