Bug 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats
authorDaniel Holbert <dholbert@cs.stanford.edu>
Mon, 31 Oct 2016 08:58:17 -0700
changeset 320319 86fbc1fb2818ec68c7c917557639c6c0a049c75b
parent 320318 b48820f4c87aeaa2186c401c1bafe6661d6b7e31
child 320320 d1135160f859f64f5a5b0ca02bf70f220e8167ce
push id30896
push userphilringnalda@gmail.com
push dateTue, 01 Nov 2016 01:36:59 +0000
treeherdermozilla-central@0899c2b63e21 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1269045
milestone52.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 1269045 part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats This patch makes the following specific changes: (1) Adds an early-return to both versions of the IsOrderLEQ function, to treat placeholder children as LEQ everything (including each other). This will tend to sort them to the beginning of the child list, which is unimportant but fine. More importantly, though: this means our "order"-sorting code won't reorder placeholders *with respect to each other* (since our sort algorithm is stable). So their painting order won't be affected by the "order" property, which is required by the spec. (2) Drops some nsPlaceholderFrame::GetRealFrameFor() calls -- they're unnecessary, since any placeholder frames will have prompted us to return earlier. One caveat to (2): this patch does leave a few "nsPlaceholderFrame::GetRealFrameFor()" calls in place, *for the moment*. These remaining calls are for handling placeholders that are wrapped, i.e. inside of anonymous flex items. These calls are still needed to avoid assertion-failures (i.e. to get a consistent ordering) at this point, but they'll be removed in a later patch in this same bug, when we stop wrapping placeholders in anonymous flex items. MozReview-Commit-ID: 1R6NW30Kxgv
layout/generic/nsFlexContainerFrame.cpp
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -1076,36 +1076,38 @@ GetFirstNonAnonBoxDescendant(nsIFrame* a
 bool
 IsOrderLEQWithDOMFallback(nsIFrame* aFrame1,
                           nsIFrame* aFrame2)
 {
   MOZ_ASSERT(aFrame1->IsFlexItem() && aFrame2->IsFlexItem(),
              "this method only intended for comparing flex items");
   MOZ_ASSERT(aFrame1->GetParent() == aFrame2->GetParent(),
              "this method only intended for comparing siblings");
-  bool isInLegacyBox = nsFlexContainerFrame::IsLegacyBox(aFrame1->GetParent());
-
   if (aFrame1 == aFrame2) {
     // Anything is trivially LEQ itself, so we return "true" here... but it's
     // probably bad if we end up actually needing this, so let's assert.
     NS_ERROR("Why are we checking if a frame is LEQ itself?");
     return true;
   }
 
-  // If we've got a placeholder frame, use its out-of-flow frame's 'order' val.
-  {
-    nsIFrame* aRealFrame1 = nsPlaceholderFrame::GetRealFrameFor(aFrame1);
-    nsIFrame* aRealFrame2 = nsPlaceholderFrame::GetRealFrameFor(aFrame2);
-
-    int32_t order1 = GetOrderOrBoxOrdinalGroup(aRealFrame1, isInLegacyBox);
-    int32_t order2 = GetOrderOrBoxOrdinalGroup(aRealFrame2, isInLegacyBox);
-
-    if (order1 != order2) {
-      return order1 < order2;
-    }
+  if (aFrame1->GetType() == nsGkAtoms::placeholderFrame ||
+      aFrame2->GetType() == nsGkAtoms::placeholderFrame) {
+    // Treat placeholders (for abspos/fixedpos frames) as LEQ everything.  This
+    // ensures we don't reorder them w.r.t. one another, which is sufficient to
+    // prevent them from noticeably participating in "order" reordering.
+    return true;
+  }
+
+  bool isInLegacyBox = nsFlexContainerFrame::IsLegacyBox(aFrame1->GetParent());
+
+  int32_t order1 = GetOrderOrBoxOrdinalGroup(aFrame1, isInLegacyBox);
+  int32_t order2 = GetOrderOrBoxOrdinalGroup(aFrame2, isInLegacyBox);
+
+  if (order1 != order2) {
+    return order1 < order2;
   }
 
   // The "order" values are equal, so we need to fall back on DOM comparison.
   // For that, we need to dig through any anonymous box wrapper frames to find
   // the actual frame that corresponds to our child content.
   aFrame1 = GetFirstNonAnonBoxDescendant(aFrame1);
   aFrame2 = GetFirstNonAnonBoxDescendant(aFrame2);
   MOZ_ASSERT(aFrame1 && aFrame2,
@@ -1160,24 +1162,29 @@ IsOrderLEQWithDOMFallback(nsIFrame* aFra
 bool
 IsOrderLEQ(nsIFrame* aFrame1,
            nsIFrame* aFrame2)
 {
   MOZ_ASSERT(aFrame1->IsFlexItem() && aFrame2->IsFlexItem(),
              "this method only intended for comparing flex items");
   MOZ_ASSERT(aFrame1->GetParent() == aFrame2->GetParent(),
              "this method only intended for comparing siblings");
+
+  if (aFrame1->GetType() == nsGkAtoms::placeholderFrame ||
+      aFrame2->GetType() == nsGkAtoms::placeholderFrame) {
+    // Treat placeholders (for abspos/fixedpos frames) as LEQ everything.  This
+    // ensures we don't reorder them w.r.t. one another, which is sufficient to
+    // prevent them from noticeably participating in "order" reordering.
+    return true;
+  }
+
   bool isInLegacyBox = nsFlexContainerFrame::IsLegacyBox(aFrame1->GetParent());
 
-  // If we've got a placeholder frame, use its out-of-flow frame's 'order' val.
-  nsIFrame* aRealFrame1 = nsPlaceholderFrame::GetRealFrameFor(aFrame1);
-  nsIFrame* aRealFrame2 = nsPlaceholderFrame::GetRealFrameFor(aFrame2);
-
-  int32_t order1 = GetOrderOrBoxOrdinalGroup(aRealFrame1, isInLegacyBox);
-  int32_t order2 = GetOrderOrBoxOrdinalGroup(aRealFrame2, isInLegacyBox);
+  int32_t order1 = GetOrderOrBoxOrdinalGroup(aFrame1, isInLegacyBox);
+  int32_t order2 = GetOrderOrBoxOrdinalGroup(aFrame2, isInLegacyBox);
 
   return order1 <= order2;
 }
 
 bool
 nsFlexContainerFrame::IsHorizontal()
 {
   const FlexboxAxisTracker axisTracker(this, GetWritingMode());