Bug 1226697 part 1 - [css-grid] Fix off-by-one error when counting lines in reverse. r=dholbert
authorMats Palmgren <mats@mozilla.com>
Thu, 17 Dec 2015 04:12:09 +0100
changeset 276738 7639bf1c7f6bc56e8a0c2381e04eda9b1d40621d
parent 276737 e42fd112a6d405dee42ebe7d262a11bf167431d2
child 276739 fb0bf39d0e68f2aa14957f8e90d0a959e0b1b112
push id29806
push usercbook@mozilla.com
push dateThu, 17 Dec 2015 10:59:52 +0000
treeherdermozilla-central@0711218a018d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1226697
milestone46.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 1226697 part 1 - [css-grid] Fix off-by-one error when counting lines in reverse. r=dholbert
layout/generic/nsGridContainerFrame.cpp
--- a/layout/generic/nsGridContainerFrame.cpp
+++ b/layout/generic/nsGridContainerFrame.cpp
@@ -677,22 +677,17 @@ private:
 static
 bool IsMinContent(const nsStyleCoord& aCoord)
 {
   return aCoord.GetUnit() == eStyleUnit_Enumerated &&
          aCoord.GetIntValue() == NS_STYLE_GRID_TRACK_BREADTH_MIN_CONTENT;
 }
 
 /**
- * Search for the aNth occurrence of aName in aNameList (forward), starting at
- * the zero-based aFromIndex, and return the 1-based index (line number).
- * Also take into account there is an unconditional match at aImplicitLine
- * unless it's zero.
- * Return zero if aNth occurrences can't be found.  In that case, aNth has
- * been decremented with the number of occurrences that were found (if any).
+ * @see FindNamedLine, this function searches forward.
  */
 static uint32_t
 FindLine(const nsString& aName, int32_t* aNth,
          uint32_t aFromIndex, uint32_t aImplicitLine,
          const nsTArray<nsTArray<nsString>>& aNameList)
 {
   MOZ_ASSERT(aNth && *aNth > 0);
   int32_t nth = *aNth;
@@ -715,46 +710,65 @@ FindLine(const nsString& aName, int32_t*
     }
   }
   MOZ_ASSERT(nth > 0, "should have returned a valid line above already");
   *aNth = nth;
   return 0;
 }
 
 /**
- * @see FindLine, this function does the same but searches in reverse.
+ * @see FindNamedLine, this function searches in reverse.
  */
 static uint32_t
 RFindLine(const nsString& aName, int32_t* aNth,
           uint32_t aFromIndex, uint32_t aImplicitLine,
           const nsTArray<nsTArray<nsString>>& aNameList)
 {
   MOZ_ASSERT(aNth && *aNth > 0);
+  if (MOZ_UNLIKELY(aFromIndex == 0)) {
+    return 0; // There are no named lines beyond the start of the explicit grid.
+  }
+  --aFromIndex; // (shift aFromIndex so we can treat it as inclusive)
   int32_t nth = *aNth;
   const uint32_t len = aNameList.Length();
   // The implicit line may be beyond the length of aNameList so we match this
   // line first if it's within the len..aFromIndex range.
   if (aImplicitLine > len && aImplicitLine < aFromIndex) {
     if (--nth == 0) {
       return aImplicitLine;
     }
   }
-  uint32_t i = aFromIndex == 0 ? len : std::min(aFromIndex, len);
-  for (; i; --i) {
+  for (uint32_t i = std::min(aFromIndex, len); i; --i) {
     if (i == aImplicitLine || aNameList[i - 1].Contains(aName)) {
       if (--nth == 0) {
         return i;
       }
     }
   }
   MOZ_ASSERT(nth > 0, "should have returned a valid line above already");
   *aNth = nth;
   return 0;
 }
 
+/**
+ * Find the aNth occurrence of aName, searching forward if aNth is positive,
+ * and in reverse if aNth is negative (aNth == 0 is invalid), starting from
+ * aFromIndex (not inclusive), and return a 1-based line number.
+ * Also take into account there is an unconditional match at aImplicitLine
+ * unless it's zero.
+ * Return zero if aNth occurrences can't be found.  In that case, aNth has
+ * been decremented with the number of occurrences that were found (if any).
+ *
+ * E.g. to search for "A 2" forward from the start of the grid: aName is "A"
+ * aNth is 2 and aFromIndex is zero.  To search for "A -2", aNth is -2 and
+ * aFromIndex is ExplicitGridEnd + 1 (which is the line "before" the last
+ * line when we're searching in reverse).  For "span A 2", aNth is 2 when
+ * used on a grid-[row|column]-end property and -2 for a *-start property,
+ * and aFromIndex is the line (which we should skip) on the opposite property.
+ */
 static uint32_t
 FindNamedLine(const nsString& aName, int32_t* aNth,
               uint32_t aFromIndex, uint32_t aImplicitLine,
               const nsTArray<nsTArray<nsString>>& aNameList)
 {
   MOZ_ASSERT(aNth && *aNth != 0);
   if (*aNth > 0) {
     return ::FindLine(aName, aNth, aFromIndex, aImplicitLine, aNameList);
@@ -1349,17 +1363,17 @@ nsGridContainerFrame::ResolveLineRangeHe
         // span <integer> / auto
         return LinePair(kAutoLine, aStart.mInteger);
       }
       // span <custom-ident> / span *
       // span <custom-ident> / auto
       return LinePair(kAutoLine, 1); // XXX subgrid explicit size instead of 1?
     }
 
-    uint32_t from = aEnd.mInteger < 0 ? aExplicitGridEnd : 0;
+    uint32_t from = aEnd.mInteger < 0 ? aExplicitGridEnd + 1: 0;
     auto end = ResolveLine(aEnd, aEnd.mInteger, from, aLineNameList, aAreaStart,
                            aAreaEnd, aExplicitGridEnd, eLineRangeSideEnd,
                            aStyle);
     int32_t span = aStart.mInteger == 0 ? 1 : aStart.mInteger;
     if (end <= 1) {
       // The end is at or before the first explicit line, thus all lines before
       // it match <custom-ident> since they're implicit.
       int32_t start = std::max(end - span, nsStyleGridLine::kMinLine);
@@ -1383,17 +1397,17 @@ nsGridContainerFrame::ResolveLineRangeHe
         MOZ_ASSERT(aEnd.mInteger != 0);
         return LinePair(start, aEnd.mInteger);
       }
       // http://dev.w3.org/csswg/css-grid/#grid-placement-errors
       // auto / span <custom-ident>
       return LinePair(start, 1); // XXX subgrid explicit size instead of 1?
     }
   } else {
-    uint32_t from = aStart.mInteger < 0 ? aExplicitGridEnd : 0;
+    uint32_t from = aStart.mInteger < 0 ? aExplicitGridEnd + 1: 0;
     start = ResolveLine(aStart, aStart.mInteger, from, aLineNameList,
                         aAreaStart, aAreaEnd, aExplicitGridEnd,
                         eLineRangeSideStart, aStyle);
     if (aEnd.IsAuto()) {
       // A "definite line / auto" should resolve the auto to 'span 1'.
       // The error handling in ResolveLineRange will make that happen and also
       // clamp the end line correctly if we return "start / start".
       return LinePair(start, start);
@@ -1412,17 +1426,17 @@ nsGridContainerFrame::ResolveLineRangeHe
       if (start >= int32_t(aExplicitGridEnd)) {
         // The start is at or after the last explicit line, thus all lines
         // after it match <custom-ident> since they're implicit.
         return LinePair(start, std::min(start + nth, nsStyleGridLine::kMaxLine));
       }
       from = start;
     }
   } else {
-    from = aEnd.mInteger < 0 ? aExplicitGridEnd : 0;
+    from = aEnd.mInteger < 0 ? aExplicitGridEnd + 1: 0;
   }
   auto end = ResolveLine(aEnd, nth, from, aLineNameList, aAreaStart,
                          aAreaEnd, aExplicitGridEnd, eLineRangeSideEnd, aStyle);
   if (start == int32_t(kAutoLine)) {
     // auto / definite line
     start = std::max(nsStyleGridLine::kMinLine, end - 1);
   }
   return LinePair(start, end);
@@ -1488,30 +1502,32 @@ nsGridContainerFrame::ResolveAbsPosLineR
   int32_t aGridStart,
   int32_t aGridEnd,
   const nsStylePosition* aStyle)
 {
   if (aStart.IsAuto()) {
     if (aEnd.IsAuto()) {
       return LineRange(kAutoLine, kAutoLine);
     }
-    int32_t end = ResolveLine(aEnd, aEnd.mInteger, 0, aLineNameList, aAreaStart,
-                              aAreaEnd, aExplicitGridEnd, eLineRangeSideEnd,
-                              aStyle);
+    uint32_t from = aEnd.mInteger < 0 ? aExplicitGridEnd + 1: 0;
+    int32_t end =
+      ResolveLine(aEnd, aEnd.mInteger, from, aLineNameList, aAreaStart,
+                  aAreaEnd, aExplicitGridEnd, eLineRangeSideEnd, aStyle);
     if (aEnd.mHasSpan) {
       ++end;
     }
     // A line outside the existing grid is treated as 'auto' for abs.pos (10.1).
     end = AutoIfOutside(end, aGridStart, aGridEnd);
     return LineRange(kAutoLine, end);
   }
 
   if (aEnd.IsAuto()) {
+    uint32_t from = aStart.mInteger < 0 ? aExplicitGridEnd + 1: 0;
     int32_t start =
-      ResolveLine(aStart, aStart.mInteger, 0, aLineNameList, aAreaStart,
+      ResolveLine(aStart, aStart.mInteger, from, aLineNameList, aAreaStart,
                   aAreaEnd, aExplicitGridEnd, eLineRangeSideStart, aStyle);
     if (aStart.mHasSpan) {
       start = std::max(aGridEnd - start, aGridStart);
     }
     start = AutoIfOutside(start, aGridStart, aGridEnd);
     return LineRange(start, kAutoLine);
   }