1146051 - [css-grid] Allow the implicit grid to be extended before the first row/column of the explicit grid

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

We need to allow the implicit grid to be extended before the first
row/column of the explicit grid, so we need to redesign LineRange
to accommodate that.  At that same time we should also take 'auto'
representation into account, for both normal flow / abs.pos. items.
And enforce stricter invariants as appropriate for each type.

Here's the Chrome bug:
https://code.google.com/p/chromium/issues/detail?id=444011
And some discussion between Rego / Tab that confirms the spec intent.
https://lists.w3.org/Archives/Public/www-style/2014Dec/0242.html
(This is a spec change that occurred after I wrote the first version
of the line placement code.)

Comment 1

[Mats Palmgren (inactive)]

I think this implies that line numbers can be zero or negative,
but only as a result of resolving definite lines in 9.2.
The auto-placement algo. can't expand the grid to the top/left
IIUC (if any span is larger than the grid it expands it
bottom/right).

So I think what we should do is use the full integer range
while resolving definite lines.  Do *not* inflate the grid
or fill the cellmap during this step, but record the max
overflow to the top/left.

Next, before auto-placement, iterate the items with at least
one definite axis and add an offset to it (the abs() value of
the max top/left overflow from above) and inflate the implicit
grid / cellmap with this new grid area, this will make the
whole grid 1,1-based or 0,0-based as we see fit.

Now, do auto-placement as before without changes.

For abs.pos. placement, which occurs next, we can probably
resolve definite lines as usual and then add the offset.

If we offset the grid like described we might as well make
the lines zero-based rather than 1-based to simplify later
code that use the lines to index arrays etc.  I think we
should probably do the above 1-based in the first patch,
and then do the 1-based to zero-based conversion as
a separate patch on top.

We probably don't need to keep the line offset around
after placement, but perhaps it would be good to have it
at least in DEBUG builds.

Given that we will clamp line numbers (bug 1147423) we could
use a constant outside the range to represent 'auto'.
That's probably the easiest to work with, e.g.
LineRange(GRID_AUTO, span) or possibly make a subclass
AutoSpanRange(uint32_t aSpan) to hide that. Testing for 'auto'
can use IsAuto() as before, possibly with some nuanced methods
for abs.pos. IsAutoToAuto() / IsLineToAuto() / IsAutoToLine(),
for auto/auto, 1/auto and auto/1 respectively.

Thoughts?

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Change the representation of 'auto' from zero to kGridAuto - an arbitrary number outside the range we clamp definite lines to.

Mostly idempotent changes, except for a few added assertions.
I moved an assertion that we relaxed earlier for abs.pos.
into ContainingBlockFor() instead, which isn't used for abs.pos.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Remove dead code because ResolveLine() never returns kGridAuto.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Simplify some code because ResolveLine() never returns kGridAuto.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Translate the grid so that the top-/left-most implicit line becomes 1,1.

(a later patch will make the grid 0,0-based after resolving definite
lines but I figured it's easier to review that separately)

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Resolve definite lines such that they expand the implicit grid also to the top/left as needed.

This is the patch that allows ResolveLine to return negative lines.

It also updates the line resolution code to the latest spec for the
"not enough <custom-ident>"s case.  An earlier spec said we should
return the last found, whereas the new spec says "If not enough lines
with that name exist, all lines in the implicit grid are assumed to
have that name...".

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-grid] Make the grid zero-based after resolving definite lines.

After deliberating this for while, I think this is worth doing.
Later patches will be so much simpler without having to deal with
a 1,1-based grid.  So we'll use 1,1-based grid as before while
resolving definite lines and then use 0,0-based grid after that.
And since we're already translating all GridAreas to a 1,1-based
grid it's easy to make it 0,0-based instead.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-grid] More grid placement tests and new grid clamping tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24aed92edfd5

Hmm, I see that I broke some existing Grid reftests on OSX.
I'll look into that...

Comment 9

[Mats Palmgren (inactive)]

Actually, these are new tests that I added here.  It looks like some line-height
issue on that platform, so it's a test bug not a code bug.

Comment 10

[Mats Palmgren (inactive)]

Attachment: WIP part 8: use a RawLine typedef to indicate it's an untranslated, but clamped definite line.

Continuing the discussion from bug 1147423 comment 5.
Does this help as documentation?

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 3, v2 - [css-grid] Simplify some code because ResolveLine() never returns kGridAuto.

I realized that the "if (aStart.IsAuto())" block does an early return
when aEnd.IsAuto() is true, so the later "if (aEnd.IsAuto())" block
can be at the end of the else-branch instead.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #10)
> Created attachment 8595563 [details] [diff] [review]
> WIP part 8: use a RawLine typedef to indicate it's an untranslated, but
> clamped definite line.
> 
> Continuing the discussion from bug 1147423 comment 5.
> Does this help as documentation?

I'm not sure -- would the idea here be that *all* of bug 1147423's converted-toint32_t line indexes would now be RawLine? (including e.g. LineRange? [maybe as RawLineRange vs LineRange?]) If so, I think that seems clearer.

Or: would we now have 3 types of line variables -- uint32_t, int32_t, and RawLine (which is a typedef for int32_t)?  If so, that seems like it'd be more-confusing than just having the two options -- unsigned & signed.  But if this 3-option scenario is what you're going for, maybe it'd be clear enough with documentation explaining the significance of each.

Comment 13

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> I'm not sure -- would the idea here be that *all* of bug 1147423's
> converted-toint32_t line indexes would now be RawLine? (including e.g.
> LineRange? [maybe as RawLineRange vs LineRange?])

No, I think that leads to code duplication for very little gain
in clarity.  The extra types also adds a non-zero cognitive burden
so I think the net result isn't an improvement. (I think going down
the "separate types" path will also lead to RawGridArea and then
you'll pay a runtime cost in the end to copy the results back into
a GridArea.)

So how would you feel about something like this instead then:
struct LineRange { ...
    union {
      uint32_t mStart;
      int32_t mUntranslatedStart;
    };

and then use mUntranslatedStart up to the point where we translate
the grid and mStart after that?

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #13)
> The extra types also adds a non-zero cognitive burden
> so I think the net result isn't an improvement.
> The extra types also adds a non-zero cognitive burden

You're right, RawLineRange vs LineRange would probably make things too confusing.

On that topic -- my point in comment 12 was that there's already a cognitive burden (and type-mismatch-in-assignments confusion) from having 2 different line types (int32_t and uint32_t), and it only gets worse if we add RawLine as a third type.

I somewhat like the idea of introducing RawLine as an *alternative* to int32_t, to make things clearer; but I don't think I like introducing it as a *third type*.

I see two ways of addressing this:
 (1) Use RawLine as an alternative to int32_t
 (2) Just standardize on making line indices int32_t (and use assertions to enforce expected-range where appropriate) -- that's seeming more and more attractive to me -- but it sounded like you were against that in bug 1147423 comment 7.

> So how would you feel about something like this instead then:
> struct LineRange { ...
>     union {
>       uint32_t mStart;
>       int32_t mUntranslatedStart;
>     };
> and then use mUntranslatedStart up to the point where we translate
> the grid and mStart after that?

Maybe -- particularly if we had a bool flag to enforce that we're using the right union-member. (IIRC, C++ does not define what happens if you access the wrong member of a union, so if we e.g. set mUntranslatedStart and then read it off later via mStart, we'd be potentially in trouble (though compilers do what you expect I think).

This union solution seems complex, though -- I think I'd prefer either of the two options (1)/(2) above. (particularly (2).)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8595004 [details] [diff] [review]
part 1 - [css-grid] Change the representation of 'auto' from zero to kGridAuto - an arbitrary number outside the range we clamp definite lines to.

Review of attachment 8595004 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +602,5 @@
>      }
>      int32_t end = ResolveLine(aEnd, aEnd.mInteger, 0, aLineNameList, aAreaStart,
>                                aAreaEnd, aExplicitGridEnd, eLineRangeSideEnd,
>                                aStyle);
> +    MOZ_ASSERT(end != kGridAuto, "resolving non-auto line shouldn't result in auto");

(This is replacing an assertion about end != 0.)

I'm not sure this assertion makes sense -- can ResolveLine even return kGridAuto? Its return value is still documented as follows:
   * @return a definite line number, or zero in case aLine is a <custom-ident>
   * that can't be found.

So this assertion may really want to be (still) checking against 0. OR, ResolveLine may need to be updated to return kGridAuto instead of 0. (or its documentation may need an update.)

(There are 6 instances of this, in this patch -- where we call ResolveLine, and the old code checks its return value against 0, and the new code checks its return value against kGridAuto.)

@@ +1084,5 @@
>    const nsTArray<TrackSize>& aRowSizes) const
>  {
>    nscoord i, b, iSize, bSize;
> +  MOZ_ASSERT(aArea.mCols.Extent() > 0, "grid items cover at least one track");
> +  MOZ_ASSERT(aArea.mRows.Extent() > 0, "grid items cover at least one track");

It's possible aArea.mCols & aArea.mRows have mEnd == kGridAuto here, which might make these Extent() calls bogus... (depending on how you decide to address mEnd==kGridAuto in Extent()).

::: layout/generic/nsGridContainerFrame.h
@@ +59,5 @@
>  
>    NS_DECLARE_FRAME_PROPERTY(GridItemContainingBlockRect, DeleteValue<nsRect>)
>  
>  protected:
> +  static const int32_t kGridAuto;

How about "kGridAutoLine", or just "kAutoLine"? This would give better naming-consistency with kMaxLine, and it'd give more of a hint about what it's used for. ("kGridAuto" is a bit vague -- there are lots of grid-related "auto" things, in the Grid spec.)

@@ +108,5 @@
> +    uint32_t Extent() const
> +    {
> +      if (IsAuto()) {
> +        MOZ_ASSERT(mEnd >= 1 && mEnd <= nsStyleGridLine::kMaxLine,
> +                   "invalid span");

I think this wants "<", not "<=", in the "mEnd <= nsStyleGridLine::kMaxLine" comparison.

According to ResolveLineRange()'s "auto" case in attachment 8594997 [details] [diff] [review], spans for auto ranges are clamped to kMaxLine-1 -- they can't be kMaxLine.  (You have the comment "r.second is a span, clamp it to kMaxLine - 1 so that the returned range has a HypotheticalEnd <= kMaxLine")  So I think that means mEnd should max out at kMaxLine - 1 here.

@@ +111,5 @@
> +        MOZ_ASSERT(mEnd >= 1 && mEnd <= nsStyleGridLine::kMaxLine,
> +                   "invalid span");
> +        return mEnd;
> +      }
> +      return mEnd - mStart;

Given that mEnd can now be kGridAuto, I think we need to handle that here as well, right?

(We don't want to be returning mEnd - mStart if mEnd is kGridAuto.)

@@ +150,1 @@
>      int32_t mEnd;    // the end line, or the span length for 'auto'

The mEnd documentation is wrong/insufficient here.

I think it means to say:
  // The end line, or the span length if 'mStart' is kGridAuto,
  // or kGridAuto to indicate an auto span.
or something like that.

At least, ResolveAbsPosLineRange instantiates LineRanges like so:
  return LineRange(kGridAuto, kGridAuto);
...and:
  return LineRange(clamped(start, 1, int32_t(aGridEnd)), kGridAuto);

This updated documentation would cover both of those cases, I think.

Comment 16

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> This union solution seems complex, though -- I think I'd prefer either of
> the two options (1)/(2) above. (particularly (2).)

I don't think it'd be complex, and it makes it very clear when we're
using translated lines and when we're not.

Yeah, I'm not particularly fond of (2) because I think it loses the
fidelity and documentation value that using an unsigned type brings:
it conveys that we're working with translated line numbers at that
point and they are never negative.  The major part of the code will
translated line numbers.  I guess we could add a bunch of
MOZ_ASSERT(line >= 0) to solve that, but that seems like a poor
choice when we could've used an unsigned type in the first place.

I don't feel that strongly about it, but there are only a few places
that would use mUntranslatedStart so I think I'll code that up to
see what it looks like.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> (This is replacing an assertion about end != 0.)
> 
> I'm not sure this assertion makes sense -- can ResolveLine even return
> kGridAuto? Its return value is still documented as follows:
>    * @return a definite line number, or zero in case aLine is a
> <custom-ident>
>    * that can't be found.
[...]
> (There are 6 instances of this, in this patch -- where we call ResolveLine,
> and the old code checks its return value against 0, and the new code checks
> its return value against kGridAuto.)

(Ah, looks like the next patch addresses this; never mind about this review comment.)

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 5b - [css-grid] Clamp resolved definite lines for abs.pos. to the untranslated grid bounds.

I missed the abs.pos. clamping in part 5.  We need to change the literal
"1" in ResolveAbsPosLineRange and use the untranslated grid start instead,
since we're dealing with untranslated lines here which can be negative.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8595604 [details] [diff] [review]
part 3, v2 - [css-grid] Simplify some code because ResolveLine() never returns kGridAuto.

Review of attachment 8595604 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +521,5 @@
>                          aAreaEnd, aExplicitGridEnd, eLineRangeSideStart,
>                          aStyle);
> +    if (aEnd.IsAuto()) {
> +      // definite line / auto
> +      return LinePair(start, start); // ResolveLineRange will deal with it

Could you document and/or handle this case (towards the end of ResolveLineRangeHelper) a bit more clearly?

Currently, I think ResolveLineRange "deals with it" via its (r.second <= r.first) check, which has a comment referencing the #grid-placement-errors URL.  But I don't see where this scenario (definite line / auto) is described in http://dev.w3.org/csswg/css-grid/#grid-placement-errors .

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8595012 [details] [diff] [review]
part 4 - [css-grid] Translate the grid so that the top-/left-most implicit line becomes 1,1.

Review of attachment 8595012 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +813,5 @@
>  
>    // http://dev.w3.org/csswg/css-grid/#line-placement
>    // Resolve definite positions per spec chap 9.2.
> +  {
> +    nsAutoTArray<GridArea*, 100> areasToAdjust;

This (kinda huge by default) array seems a bit wasteful, particularly given that we populate it despite rarely needing it for rendering real content. (IIUC, we only need it if there are negative lines in the implicit grid, which seems unlikely for most content.)

Perhaps we should do away with areasToAdjust, and convert the second loop in this function (which currently runs over areasToAdjust) to just iterate across all of our grid items again, in the unlikely event that we need to adjust anything?  (Looks like we can restart the iterator by calling .reset() on it, to prep it for that second loop.)

That would make this uncommon case a tad slower (requiring 2 iterations), but it'd make the common case faster & more memory-efficient, which seems like a win overall.  (Or, do you suspect negative line numbers will be common enough that we should optimize for them here?)

@@ +842,5 @@
>      }
> +
> +    // Translate the whole grid so that the top-/left-most area is at 1,1.
> +    const uint32_t explicitGridOffsetCol = 1 - minCol;
> +    const uint32_t explicitGridOffsetRow = 1 - minRow;

This looked wrong to me at first -- a line like the following...
  uint32_t foo = 1 - something;
...superficially looks like it's asking for an underflow bug, though in this case it won't underflow because our 'something' is a *signed* value that's guaranteed to be 1 or less.

To make this look less suspicious, could you add a comment saying something like:
  // (Note that minCol & minRow are each <= 1, so the subtraction below must
  // produce something nonnegative.)

@@ +847,5 @@
> +    mGridColEnd += explicitGridOffsetCol;
> +    mGridRowEnd += explicitGridOffsetRow;
> +    mExplicitGridOffsetCol = explicitGridOffsetCol;
> +    mExplicitGridOffsetRow = explicitGridOffsetRow;
> +    for (GridArea* area : areasToAdjust) {

Most of the time, explicitGridOffset[Row|Col] will be zero, and this for-loop won't be needed.

Can we skip this loop entirely if the offsets are 0?  (And as noted above, I think we might just want to do away with areasToAdjust and instead loop across *all* the items here, in the unlikely event that we've got nontrivial offsets.)

@@ +976,5 @@
>        GridArea area(PlaceAbsPos(child, aStyle));
> +      if (area.mCols.mStart != kGridAuto) {
> +        area.mCols.mStart += explicitGridOffsetCol;
> +      }
> +      if (area.mCols.mEnd != kGridAuto) {

[etc]

This chunk of 12 lines seems like it'd be worth refactoring into a helper function, so you can collapse this to a one-liner:
  area->AddExplcitGridOffsets(explicitGridOffsetCol, explicitGridOffsetRow);

(I think you could use this same helper-function up higher where you adjust areas by the offset, too. The logic doesn't quite match, but I don't think it differs in a way that matters.)

Comment 21

[Mats Palmgren (inactive)]

> Perhaps we should do away with areasToAdjust ...

Yeah, that's a good point.  I wrote this before I decided to go for
a zero-based grid (part 6).  Which means we'll translate all items
anyway that aren't auto/auto but those are probably rare.  I agree
iterating a second time makes more sense.

If you don't mind I'll fix this as a separate patch at the end,
to avoid bit-rotting the rest.  It doesn't make much sense to
change this before the zero-based grid patch anyway.

>   area->AddExplcitGridOffsets( ...

I'll look into that too.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8595012 [details] [diff] [review]
part 4 - [css-grid] Translate the grid so that the top-/left-most implicit line becomes 1,1.

Sounds good -- so, r=me on part 4, with review notes (comment 20) addressed in a separate patch as you see fit.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8595014 [details] [diff] [review]
part 5 - [css-grid] Resolve definite lines such that they expand the implicit grid also to the top/left as needed.

FYI, the last hunk here is a "typo".  You can assume this patch
makes no change to Extent().

Comment 24

[Mats Palmgren (inactive)]

Attachment: part 1, v2 - [css-grid] Change the representation of 'auto' from zero to kGridAuto - an arbitrary number outside the range we clamp definite lines to.

(In reply to Daniel Holbert [:dholbert] from comment #15)
> I'm not sure this assertion makes sense -- can ResolveLine even return
> kGridAuto?

Sorry for the misunderstanding.  That's what I meant by "mostly
idempotent changes".  I should have pointed out that later
patches would remove these redundant assertions.

> > +  MOZ_ASSERT(aArea.mRows.Extent() > 0, "grid items cover at least one track");
> 
> It's possible aArea.mCols & aArea.mRows have mEnd == kGridAuto here

No, that shouldn't be possible.  Only abs.pos. can have mEnd==kGridAuto
and it doesn't call this method (which is why I moved these assertions
here).

> How about "kGridAutoLine", or just "kAutoLine"?

Sure, kAutoLine is a better name.  Thanks.

> > +    uint32_t Extent() const
> > +    {
> > +      if (IsAuto()) {
> > +        MOZ_ASSERT(mEnd >= 1 && mEnd <= nsStyleGridLine::kMaxLine,
> > +                   "invalid span");
> 
> I think this wants "<", not "<=", in the "mEnd <= nsStyleGridLine::kMaxLine"
> comparison.

Fixed.

> Given that mEnd can now be kGridAuto, I think we need to handle that here as
> well, right?

mEnd can only be kAutoLine for abs.pos. and Extent() is undefined for
abs.pos with 'auto'.  I added an "mEnd != kAutoLine" assert there.

> >      int32_t mEnd;    // the end line, or the span length for 'auto'
> 
> The mEnd documentation is wrong/insufficient here.

Good catch. I meant to update the documentation for LineRange in general
in a later patch (so I wouldn't have to tweak the documentation in each
separate patch) but it seems I forgot about that entirely.
Sorry about that. :-(

(I'll address this in a later patch, which I'll submit shortly.)

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 3, v3 - [css-grid] Simplify some code because ResolveLine() never returns kGridAuto.

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Currently, I think ResolveLineRange "deals with it" via its (r.second <=
> r.first) check

Yes, I added a code comment at the call site to explain it.

Comment 26

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-grid] Make LineRange::mStart/mEnd into a union with both a signed/unsigned members to make it clear when we're working with translated line numbers and when not.

I think this idea turned out well.  It makes the code clear when we're
dealing with untranslated lines and when not, and exactly where the
transition occurs.  There are still a few int32_t/uint32_t casts but
I think we would have a few of those even if we switched to int32_t
wholesale (for "mEnd < array.Length()" tests and such).
Using uint32_t also saves us a few "line >= 0" assertions.

I intentionally didn't change the transition statements ...:

if (area.mCols.mStart != kAutoLine) {
  area.mCols.mStart = area.mCols.mUntranslatedStart + offsetToColZero;
}

... behind convenience functions, for two reasons:
1. it's easier to understand what happens above if it's not hidden
   behind a convenience function
2. it might save a few lines of repetitive code, but not that much


I also realized that ResolveAutoPosition() is only called for
translated lines so I changed that to take an unsigned type.

This patch also updates the documentation of LineRange to describe
the final state of the code.

Comment 27

[Mats Palmgren (inactive)]

Attachment: (wdiff) part 8 - [css-grid] Iterate all items again instead of collecting the ones with a definite position in an array which wouldn't be much of win anyway.

This removes the 'areasToAdjust' array as discussed above.
(This is the wdiff version for easier review.)

Comment 28

[Mats Palmgren (inactive)]

Attachment: part 9 - [css-grid] More grid placement tests and new grid clamping tests.

Added a few tests for the "clamp abs.pos. to negative line" case that
I first missed and then addressed in part 5b.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8595014 [details] [diff] [review]
part 5 - [css-grid] Resolve definite lines such that they expand the implicit grid also to the top/left as needed.

Review of attachment 8595014 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +133,5 @@
>   * 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 was found (if any).

3 documentation nits:
 (1) s/that was/that were/

 (2) Maybe s/aNth/*aNth/ throughout this header-comment, given that aNth is a pointer now? ("aNth has been decremented" is semi-ambiguous, since both pointers and integers can be decremented.)

 (3) It looks like the provided *aNth must be positive, and it's guaranteed to still be positive when we return, yes? (Worth documenting both of these facts, IMO, because you've changed the type to be signed, which implies that this value could be negative as an input and/or output -- but it can't, IIUC.)

@@ +163,3 @@
>    }
> +  *aNth = nth;
> +  return 0;

Could we assert that nth is > 0 here, as a post-condition?

If nth were <= 0 here, I think that'd imply that we found (at least) the requested number of occurrences of the line-name, which means we should have succeeded. (which would be bad, because we're failing)

This applies to the end of RFindLine, too.

@@ +173,4 @@
>            uint32_t aFromIndex, uint32_t aImplicitLine,
>            const nsTArray<nsTArray<nsString>>& aNameList)
>  {
> +  MOZ_ASSERT(aNth && aNth > 0);

You're missing a '*' in the comparison to 0 here.

@@ +179,3 @@
>    // The implicit line may be beyond the length of aNameList so we match this
>    // line first if it's within the 0..aFromIndex range.
>    if (aImplicitLine > len && aImplicitLine < aFromIndex) {

While you're here, I think this comment means to say "len..aFromIndex"? (not "0..")

@@ +200,4 @@
>                uint32_t aFromIndex, uint32_t aImplicitLine,
>                const nsTArray<nsTArray<nsString>>& aNameList)
>  {
> +  MOZ_ASSERT(aNth && aNth != 0);

s/aNth/*aNth/ in the 0 comparison.

@@ +206,5 @@
>    }
> +  int32_t nth = -*aNth;
> +  int32_t line = ::RFindLine(aName, &nth, aFromIndex, aImplicitLine, aNameList);
> +  *aNth = -nth;
> +  return line;

Technically, we only need the "*aNth = -nth" assignment at the end here if 'line' is 0, right? (Otherwise nth won't have changed, and so *aNth won't be changing.)

So, it might be clearer to wrap that assignment in "if (line == 0)", but not a big deal.

@@ +453,4 @@
>        if (aLine.mHasSpan) {
>          // http://dev.w3.org/csswg/css-grid/#grid-placement-span-int
> +        // 'span <custom-ident> N'
> +        aFromIndex = aSide == eLineRangeSideStart ? 1 : aExplicitGridEnd;

Why are we reassigning |aFromIndex| here?

aFromIndex is documented as "the zero-based index to start counting from", and presumably the caller has passed in some meaningful value for it.  But the patch is reassigning it to one or the other extreme edge of the grid here. So then we'll end up counting from that edge, instead of from the caller's passed-in "aFromIndex", in the |line| assignment below, which seems wrong.

@@ +498,5 @@
>      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);

Could we broaden the check here from "end <= 1" to instead check if the *hypothetical start* is less than 1?
e.g.:
  if (end - span <= 1) {

Reasoning: if our span will *necessarily* push us past (below) the first explicit line, then I think this clause's code would be all correct. (And it lets us skip a potentially-expensive call to ResolveLine, & the string-comparisons that it involves.)

If you agree, you'd want to change the comment here, too (since we're no longer checking "the end is at or before").

Possible revised comment (possibly too wordy):
    // Our hypothetical start line is at or before the first explicit line;
    // thus, there aren't enough lines with our <custom-ident>*. So, we
    // assume all lines in the explicit grid match our <custom-ident>.
    // (*Or, we might exactly reach 1 with all lines actually matching our
    // <custom-ident>, in which case this assumption is still valid.)

I think this all applies to the symmetric check for "if (start >= int32_t(aExplicitGridEnd)) {" that you're adding further down, too. (That could be "start + span").

@@ +540,5 @@
> +    if (MOZ_UNLIKELY(start < 0)) {
> +      if (aEnd.mLineName.IsEmpty()) {
> +        return LinePair(start, start + nth);
> +      }
> +      // Fall through and start searching from the start of the grid (from=0).

RE using "from=0" for this start<0 w/ non-empty linename case:
Do we need to consider the scenario of "not enough lines that match our line-name, hence treat all lines in implicit grid as matching <custom-ident>, *including negative lines*"?

If so, using |from=0| seems wrong (due to my "including negative lines" emphasis above). But maybe that scenario is handled elsewhere & I lost track of it, & we don't need to worry about it here?

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8596699 [details] [diff] [review]
part 3, v3 - [css-grid] Simplify some code because ResolveLine() never returns kGridAuto.

Review of attachment 8596699 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +523,5 @@
> +    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 pass "start / start".
> +      return LinePair(start, start);

Thanks for clarifying the language here.

One nit: "pass" seems slightly wrong here. Maybe replace...
  if we pass "start / start".
...with:
  if we return "start / start" to it.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8596119 [details] [diff] [review]
part 5b - [css-grid] Clamp resolved definite lines for abs.pos. to the untranslated grid bounds.

Review of attachment 8596119 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +696,5 @@
>                                    const nsStylePosition* aStyle)
>  {
>    const nsStylePosition* itemStyle = aChild->StylePosition();
> +  int32_t gridColStart = 1 - mExplicitGridOffsetCol;
> +  int32_t gridRowStart = 1 - mExplicitGridOffsetRow;

These variables would benefit from an explanatory comment, to establish some context about how we're working with an untranslated grid.

(The caller has some code/context about un-translating the grid, but inside this function, it's not yet clear that we're dealing with a weird briefly-untranslated grid -- so these new variables are currently mysterious.)

r=me with that.

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8595018 [details] [diff] [review]
part 6 - [css-grid] Make the grid zero-based after resolving definite lines.

Review of attachment 8595018 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +264,5 @@
>  
>  static nscoord
>  GridLinePosition(uint32_t aLine, const nsTArray<TrackSize>& aTrackSizes)
>  {
> +  const uint32_t endIndex = aLine;

General comment -- so a lot of the changes in this file (like this change) are converting formerly-1-based line indices to be 0-based now.  Given that 1-based line indices still exist, it'd be nice to document which functions can expect that their line indices are all 0-based, so that this the basis is clearer & doesn't have to be inferred/guessed.

So -- could you add a one-liner comment above functions like this one (and FindAutoCol, and FindAutoRow, and PlaceAutoAutoInRow, and CellMap::Fill), saying e.g.:
  // Note: In this function, line numbers are all 0-based.

(It's probably obvious to you in which functions the line indices are 1-based vs. 0-based, depending on whether we've resolved our explicit grid yet, per comment 7.  However, looking at a particular function in isolation, it's non-obvious whether we're before or after that point. So, it'd be nice to be explicit about the 0|1-based-ness, given that both numbering schemes are used here.)

::: layout/generic/nsGridContainerFrame.h
@@ +125,1 @@
>        mStart = aStart;

Two nits about the LineRange changes (like this one), in general:
 (1) The documentation above the LineRange class-definition still says mStart & mEnd are 1-based. I think that documentation needs updating now, since at least in this case, it's 0-based. (I think it's always 0-based now? Or, if it varies, maybe we should have a debug-only flag to record what numbering scheme we're in, so we can sanity-check that the numbering scheme matches what we expect in various functions, like the functions that you're changing here?)

 (2) The LineRange() constructor has assertions about kMinLine and kMaxLine. I think these assertions might now mean something different from what they used to, given that LineRange is now 0-based. So, do those assertions need changing, to preserve what they were previously asserting?

@@ +470,2 @@
>    uint32_t mGridColEnd; // always >= mExplicitGridColEnd
>    uint32_t mGridRowEnd; // always >= mExplicitGridRowEnd

The "always >=" comments here are no longer true, right? (because these values are now 0-based, whereas the mExplicitGrid* versions are 1-based)

@@ +471,5 @@
>    uint32_t mGridRowEnd; // always >= mExplicitGridRowEnd
>  
>    /**
>     * Offsets from the start of the implicit grid to the start of the translated
>     * explicit grid.  They are zero if there are no implicit lines before 1,1.

(If you agree with my comment below, then this will want:
  s/translated/translated, 0-based/
and "They are zero" will need a tweak as well. Anyway, read on, & this'll make more sense:)

@@ +474,5 @@
>     * Offsets from the start of the implicit grid to the start of the translated
>     * explicit grid.  They are zero if there are no implicit lines before 1,1.
>     * e.g. "grid-column: span 3 / 1" makes mExplicitGridOffsetCol = 3 and the
> +   * corresponding GridArea::mCols will be 0 / 3 in the zero-based translated
> +   * grid.

I wonder if these should just be *actual offsets* between the two numbering schemes? (i.e. maybe they should be "1,1" (or -1,-1?) in the trivial case, instead of being "0,0".

I say that because it's a bit odd to have offsets = 0, when the numbering system is off by 1.  And it also looks like whenever you use these offsets, you *first* convert them into "offsetToColZero" first, and then use *that*. So it seems like it might be simpler to just directly store that.

If you agree, the name would need a tweak, too -- maybe: "mOffsetTo0BasedGridCol" & Row?

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 8596733 [details] [diff] [review]
(wdiff) part 8 - [css-grid] Iterate all items again instead of collecting the ones with a definite position in an array which wouldn't be much of win anyway.

r=me on part 8

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8595022 [details] [diff] [review]
part 7 - [css-grid] More grid placement tests and new grid clamping tests.

(I think you meant to obsolete the test-only "part 7" when you posted the test-only "part 9". Looks like they add some of the same files, so I think part 9 is the new version of that part 7. [and there's now a code patch that's also named part 7].  --> Marking the original test-only "part 7" as obsolete.)

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8596725 [details] [diff] [review]
part 7 - [css-grid] Make LineRange::mStart/mEnd into a union with both a signed/unsigned members to make it clear when we're working with translated line numbers and when not.

Review of attachment 8596725 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +1001,5 @@
>      for (nsFrameList::Enumerator e(children); !e.AtEnd(); e.Next()) {
>        nsIFrame* child = e.get();
>        GridArea area(PlaceAbsPos(child, aStyle));
>        if (area.mCols.mStart != kAutoLine) {
> +        area.mCols.mStart = area.mCols.mUntranslatedStart + offsetToColZero;

The "mStart != kAutoLine" & "mEnd != kAutoLine" comparisons here should all be checking mUntranslatedStart, I think?

(Technically we haven't set mStart/mEnd yet -- that's what we're about to do. And mStart/mEnd are by definition never supposed to be "auto", IIUC.)

::: layout/generic/nsGridContainerFrame.h
@@ +71,5 @@
>    explicit nsGridContainerFrame(nsStyleContext* aContext) : nsContainerFrame(aContext) {}
>  
>    /**
>     * A LineRange can be definite or auto - when it's definite it represents
> +   * a consecutive set of tracks between a starting line and an ending line.

Ah, looks like the LineRange documentation is getting an update here. That partly addresses my concern from comment 32, but the new documentation here doesn't say whether the lines are 0- or 1-based. Can you add a mention of that (maybe down near the mUntranslatedStart & mStart decls)?

@@ +73,5 @@
>    /**
>     * A LineRange can be definite or auto - when it's definite it represents
> +   * a consecutive set of tracks between a starting line and an ending line.
> +   * Before it's definite it can also represent an auto position with a span,
> +   * where mStart == kAutoLine and mEnd is the (non-zero positive) span.

I think you mean mUntranslatedStart and mUntranslatedEnd here? (that's what we'll be using for pre-definite LineRanges now, as of this patch, right?)

(Probably good to hint at the mUntranslatedStart / mStart distinction in this header comment, too -- pointing to the more detailed documentation next to the variable decls.)

@@ +75,5 @@
> +   * a consecutive set of tracks between a starting line and an ending line.
> +   * Before it's definite it can also represent an auto position with a span,
> +   * where mStart == kAutoLine and mEnd is the (non-zero positive) span.
> +   * For normal-flow items, the invariant mStart < mEnd holds when both
> +   * lines are definite.

This is confusing: "when both lines are definite".  Aren't mStart/mEnd *always* definite now? (and the "mUntranslated" vars are their pre-definite versions?) I think perhaps this wants to be talking about mUntranslatedStart / mUntranslatedEnd, instead of mStart/mEnd.

@@ +82,2 @@
>     * "attach this side to the grid container containing block edge".
> +   * Additionally, mStart <= mEnd holds when both are definite (non-kAutoLine),

(I think both mentions of mStart/mEnd here mean to say mUntranslatedStart/mUntranslatedEnd? Since mStart & mEnd should never be kAutoLine.)

@@ +98,2 @@
>            MOZ_ASSERT(mEnd == kAutoLine ||
> +                     (aEnd >= nsStyleGridLine::kMinLine &&

Nit: s/mEnd/aEnd" in the "mEnd == kAutoLine ||" comparison here, I think? (to match your conversions to aStart/aEnd in the rest of this section).

@@ +103,5 @@
>  #endif
>      }
>      bool IsAutoAuto() const { return mStart == kAutoLine && mEnd == kAutoLine; }
>      bool IsAuto() const { return mStart == kAutoLine; }
>      bool IsDefinite() const { return mStart != kAutoLine; }

Should these all be using mUntranslatedStart? (mStart is only used once we're definite, right?)

@@ +125,2 @@
>        mStart = aStart;
>        mEnd += aStart;

I think this should be operating on mUntranslatedStart/mUntranslatedEnd, too, right? (I think this is before PlaceGridItems has done the translation & initialized mStart/mEnd.)

@@ +159,5 @@
> +    };
> +    union {
> +      uint32_t mEnd;
> +      int32_t mUntranslatedEnd;
> +    };

As hinted in comments above, could you add a boolean flag here "mIsDefinite" or something -- which we set when we become definite & can start using mStart/mEnd -- to let us sanity-check that we're always using the expected union-member in a given function?

(So e.g. Extent() would now assert that mIsDefinite is true, since it uses mStart/mEnd; and same for other functions.)

@@ +215,5 @@
>     * @param aAreaEnd a pointer to GridNamedArea::mColumnEnd/mRowEnd
>     * @param aExplicitGridEnd the last line in the explicit grid
>     * @param aEdge indicates whether we are resolving a start or end line
>     * @param aStyle the StylePosition() for the grid container
> +   * @return a definite line number, clamped to the kMinLine..kMaxLine range

(I think this change might really belong in part 5 or 6? It doesn't seem related to making LineRange::mStart/mEnd a union.)

(Wherever it lives, though, this should say whether the returned line-number is 0-based or 1-based. The documentation you're replacing makes it clear that the returned line-number is 1-based; the new documentation doesn't specify, though.)

Comment 36

[Mats Palmgren (inactive)]

Attachment: part 5, v2 - [css-grid] Resolve definite lines such that they expand the implicit grid also to the top/left as needed.

(In reply to Daniel Holbert [:dholbert] from comment #29)
> 3 documentation nits:
>  (1) s/that was/that were/

Fixed.

>  (2) Maybe s/aNth/*aNth/ throughout this header-comment,

I think it's clear from context that *aNth is what is meant because
interpreting the pointer value itself as the number of occurences is
completely nonsensical.  Repeating *aNth in the documentation makes
it harder to read and if anyone is still unsure the code is right
there and it's utterly clear that *aNth is what is being used.

>  (3) It looks like the provided *aNth must be positive,

Yes, the first line asserts "*aNth > 0" so I think it's clear as is.

> > +  *aNth = nth;
> > +  return 0;
> 
> Could we assert that nth is > 0 here, as a post-condition?

Sure.

> > +  MOZ_ASSERT(aNth && aNth > 0);
> 
> You're missing a '*' in the comparison to 0 here.

Oops, thanks.

> While you're here, I think this comment means to say "len..aFromIndex"? (not
> "0..")

Thanks.

> > +  MOZ_ASSERT(aNth && aNth != 0);
> 
> s/aNth/*aNth/ in the 0 comparison.

Fixed.

> So, it might be clearer to wrap that assignment in "if (line == 0)", but not
> a big deal.

Meh.

> > +        // 'span <custom-ident> N'
> > +        aFromIndex = aSide == eLineRangeSideStart ? 1 : aExplicitGridEnd;
> 
> Why are we reassigning |aFromIndex| here?

This is in the "line == 0" branch, meaning we didn't find enough lines.
aNth is decremented with what we found at this point, and the above
prepares aFromIndex for what follows:
  line = int32_t(aFromIndex) + aNth;
so this implements the "all implicit lines have the name <custom-ident>"
by distancing 'line' from the explicit grid edge by the remaining aNth.

> So then we'll end up counting from that edge, instead of from the
> caller's passed-in "aFromIndex", in the |line| assignment below, which seems
> wrong.

The passed in aFromIndex only serves as the starting point for
finding names in the explicit grid, so it's irrelevant at this point.

> Could we broaden the check here from "end <= 1" to instead check if the
> *hypothetical start* is less than 1?
> e.g.:
>   if (end - span <= 1) {
> Reasoning: if our span will *necessarily* push us past (below) the first
> explicit line, then I think this clause's code would be all correct. (And it
> lets us skip a potentially-expensive call to ResolveLine, & the
> string-comparisons that it involves.)

No, if we overlap the explicit grid we need to count which lines actually
have the name we're looking for.  For example if we have the grid:
... (-1) (0) (1) (2 A) (3) (4) ...
for "grid-column: span A 2 / 3" we only want to count line 2 as an A,
not line 1, and then the implicit line (0) as an A to get the range 0..3.

> > + // Fall through and start searching from the start of the grid (from=0).
> 
> RE using "from=0" for this start<0 w/ non-empty linename case:
> Do we need to consider the scenario of "not enough lines that match our
> line-name, hence treat all lines in implicit grid as matching
> <custom-ident>, *including negative lines*"?

The relevant spec text for "<custom-ident> N" is:
http://dev.w3.org/csswg/css-grid/#grid-placement-int

"Contributes the Nth grid line to the grid item's placement.
 If a negative integer is given, it instead counts in reverse,
 starting from the end edge of the explicit grid."

I think "starting from the end edge" implies that we should start
from the explicit grid's start edge if N is positive.  The spec
continues:

"If a name is given as a <custom-ident>, only lines with that name
 are counted. *If* not enough lines with that name exist, all lines
 in the implicit grid are assumed to have that name for the purpose
 of finding this position."

My emphasis on the "If" above.  That implies a *condition* to me,
meaning that we should only consider implicit lines *after* we
have exhausted the explicit grid without finding N matching lines.

IOW, my interpretation is: search for N named lines in the explicit
grid and if not enough matching lines were found then *continue*
into the implicit grid *in the same direction* and treat every
implicit line as matching.

The spec text for "span <custom-ident> N" is similar and has the
same "If not enough lines with that name exist" text, so I think
the search for <custom-ident> here should be the same.

What you're suggesting is basically:
"always count all implicit lines as matching any <custom-ident>"

I already did consider this interpretation carefully but came to
the conclusion that it can't be the intent of the spec author,
because it's very far from the current spec text.  I'd be happy
to ask for clarification on www-style@ though if you want.

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #36)
> > Why are we reassigning |aFromIndex| here?
> 
> This is in the "line == 0" branch, meaning we didn't find enough lines.

(I'm just noticing that this disagrees with the comment in the code. The code
says:
    if (line == 0) {
      // No line matching <custom-ident> exists.
I think this comment needs s/No line/Not enough lines/?)

> > So then we'll end up counting from that edge, instead of from the
> > caller's passed-in "aFromIndex", in the |line| assignment below, which seems
> > wrong.
> 
> The passed in aFromIndex only serves as the starting point for
> finding names in the explicit grid, so it's irrelevant at this point.

OK -- so, this code is taking an existing parameter, with a documented usage,
& reusing it for a different purpose. That seems unnecessarily confusing.

I'd prefer we either:
 - use a new local variable here (instead of aFromIndex)
or:
 - add a comment making it clearer that we're co-opting aFromIndex for a new,
   different purpose/meaning (instead of e.g. tweaking it in a way that's
   consistent with its original meaning).

> I already did consider this interpretation carefully but came to
> the conclusion that it can't be the intent of the spec author,
[...]
> I'd be happy
> to ask for clarification on www-style@ though if you want.

Thanks -- I'll go ahead & send an email to ask for clarification, since I'm
currently the one who's confused about this. :) I agree that your
interpretation makes for more sensible behavior -- I think the ambiguity lies
what "the implicit grid" means (and whether it's a superset of the explicit
grid, or disjoint -- i.e. whether "all lines in the implicit grid" includes any
lines in the explicit grid).

Comment 38

[Mats Palmgren (inactive)]

Attachment: part 5, v3 - [css-grid] Resolve definite lines such that they expand the implicit grid also to the top/left as needed.

(In reply to Daniel Holbert [:dholbert] from comment #37)
> (In reply to Mats Palmgren (:mats) from comment #36)
> > > Why are we reassigning |aFromIndex| here?
> > 
> > This is in the "line == 0" branch, meaning we didn't find enough lines.
> 
> I'm just noticing that this disagrees with the comment in the code.

Yeah that comment is obsolete.

>  - use a new local variable here (instead of aFromIndex)

Fair enough.

> I agree that your interpretation makes for more sensible behavior

I didn't claim that. :-)  I'm just saying that what I've implemented
is what the spec says.

I think I could go either way here actually.  It kind of depends on
which considerations you deem more important for authors.

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #38)
> > I agree that your interpretation makes for more sensible behavior
> 
> I didn't claim that. :-)  I'm just saying that what I've implemented
> is what the spec says.

I still think the spec technically calls for something different, but I do like your interpretation better, so I think the spec should be changed/clarified.  I just emailed www-style to suggest a tweak (and explained my understanding of the current language & why it's weird):
 https://lists.w3.org/Archives/Public/www-style/2015Apr/0362.html

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8597451 [details] [diff] [review]
part 5, v3 - [css-grid] Resolve definite lines such that they expand the implicit grid also to the top/left as needed.

r=me

Comment 41

[Mats Palmgren (inactive)]

Attachment: part 6b, additional clamping in ResolveAutoPosition

To address bug 1147423 comment 11.

Comment 42

[Mats Palmgren (inactive)]

Attachment: part 6, v2 - [css-grid] Make the grid zero-based after resolving definite lines.

(In reply to Daniel Holbert [:dholbert] from comment #32)
> it'd be nice to document which functions
> can expect that their line indices are all 0-based

I've added "1-based" in the description of the few methods
that expects 1-based definite lines (in the next patch).

>  (1) The documentation above the LineRange class-definition still says
> mStart & mEnd are 1-based. I think that documentation needs updating now,
> since at least in this case, it's 0-based.

I've updated this comment in part 7.  (I'll attach a rollup patch
so you can see the final code.)

>  (2) The LineRange() constructor has assertions about kMinLine and kMaxLine.
> I think these assertions might now mean something different from what they
> used to, given that LineRange is now 0-based. So, do those assertions need
> changing, to preserve what they were previously asserting?

No, LineRanges always starts life with untranslated 1-based lines.

> >    uint32_t mGridColEnd; // always >= mExplicitGridColEnd
> >    uint32_t mGridRowEnd; // always >= mExplicitGridRowEnd
> 
> The "always >=" comments here are no longer true, right?

Yeah, I removed these comments since they are not very useful
and the preceding comment is sufficient.

> I wonder if these should just be *actual offsets* between the two numbering
> schemes? (i.e. maybe they should be "1,1" (or -1,-1?) in the trivial case,
> instead of being "0,0".

There will be more uses of these members in future patches so I think it's
more useful to keep them as is, i.e. as the offset from the first implicit
line to the first explicit line.

Comment 43

[Mats Palmgren (inactive)]

Attachment: part 7, v2 - [css-grid] Make LineRange::mStart/mEnd into a union with both a signed/unsigned members to make it clear when we're working with translated line numbers and when not.

(In reply to Daniel Holbert [:dholbert] from comment #35)
> doesn't say whether the lines are 0- or 1-based. Can you add a mention of
> that (maybe down near the mUntranslatedStart & mStart decls)?

I've updated the LineRange documentation so that it says "1-based"
explicitly there.

> >        if (area.mCols.mStart != kAutoLine) {
> > +        area.mCols.mStart = area.mCols.mUntranslatedStart + offsetToColZero;

Fair enough.

> > +   * Additionally, mStart <= mEnd holds when both are definite (non-kAutoLine),
> 
> (I think both mentions of mStart/mEnd here mean to say
> mUntranslatedStart/mUntranslatedEnd? Since mStart & mEnd should never be
> kAutoLine.)

No, mStart & mEnd can still be kAutoLine.  The distinction between
mUntranslatedStart/mStart is merely before/after the translation
to a zero-based grid.  Translation only affects lines that are
definite - it doesn't touch auto LineRanges (an auto LineRange has
mStart=kAutoLine mEnd=<span> so there's no need to translate it).

(I'm going to ignore comments based on that false assumption.)

> >            MOZ_ASSERT(mEnd == kAutoLine ||
> > +                     (aEnd >= nsStyleGridLine::kMinLine &&
> 
> Nit: s/mEnd/aEnd" in the "mEnd == kAutoLine ||" comparison here, I think?
> (to match your conversions to aStart/aEnd in the rest of this section).

Fair enough.

> >        mStart = aStart;
> >        mEnd += aStart;
> 
> I think this should be operating on mUntranslatedStart/mUntranslatedEnd,
> too, right? (I think this is before PlaceGridItems has done the translation
> & initialized mStart/mEnd.)

No, this is ResolveAutoPosition() which is after the translation occured.
(everything related to auto-positioning and layout is after)

> As hinted in comments above, could you add a boolean flag here "mIsDefinite"
> or something -- which we set when we become definite & can start using
> mStart/mEnd -- to let us sanity-check that we're always using the expected
> union-member in a given function?

That might be useful if these methods were a public API or library
functions were we don't control who calls what and in what order.
That's not the case here.  These are non-public methods that are only
called internally and their order is pretty much dictated by the spec.
So, in this case, adding debug flags isn't useful IMHO.  I'm confident
multiple reftests will fail if anyone were to change this and get the
order wrong somehow.

> > + * @return a definite line number, clamped to the kMinLine..kMaxLine range
> 
> this should say whether the returned line-number is 0-based or 1-based.

Yep, it's 1-based here.  I've added "1-based" to the few methods that
expects 1-based lines (i.e. those involved in resolving definite lines).

Comment 44

[Mats Palmgren (inactive)]

Attachment: part 9 - [css-grid] More grid placement tests and new grid clamping tests.

Added tests that fails without the clamping in part 6b.

Comment 45

[Mats Palmgren (inactive)]

Attachment: rollup patch with bug 1147423 included

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8599271 [details] [diff] [review]
part 6b, additional clamping in ResolveAutoPosition

Review of attachment 8599271 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +27,5 @@
>  using namespace mozilla;
>  typedef nsGridContainerFrame::TrackSize TrackSize;
>  const int32_t nsGridContainerFrame::kAutoLine = 12345;
> +const uint32_t nsGridContainerFrame::kTranslatedMaxLine =
> +  uint32_t(nsStyleGridLine::kMaxLine - nsStyleGridLine::kMinLine - 1);

Please add a comment explaining what this constant represents, to help with interpretation of the arithmetic here.

(I think this is the maximum line number, in our zero-based translated representation of the grid, right?)

::: layout/generic/nsGridContainerFrame.h
@@ +127,5 @@
>        mStart = aStart;
>        mEnd += aStart;
> +      if (mEnd > kTranslatedMaxLine) {
> +        mEnd = kTranslatedMaxLine;
> +        mStart = mEnd - 1;

I'm not sure this is right, according to http://dev.w3.org/csswg/css-grid/#overlarge-grids

If *only* mEnd is out-of-bounds, then I think we just want to clamp mEnd and leave mStart untouched, right? (spec quote: "If a grid item spans outside this limit, its span must be clamped to the last line of the limited grid.")

The code you have does seem to be right if *mStart* is larger than kTranslatedMaxLine, though. (spec quote: "If a grid item is placed outside this limit, its span must be truncated to 1 and the item repositioned into the last grid track")  So I think you maybe just need:
  if (mStart > kTranslatedMaxLine) {
    [same code you already have]
  } else if (mEnd > kTranslatedMaxLine) {
    mEnd = kTranslatedMaxLine;
  }
...or something like that.

Comment 47

[Mats Palmgren (inactive)]

Attachment: part 6bv2, additional clamping in ResolveAutoPosition

> If *only* mEnd is out-of-bounds, then I think we just want to clamp mEnd and
> leave mStart untouched, right?

Right, good point.

>   if (mStart > kTranslatedMaxLine) {

I think ">=" is the right test there to put mEnd at kTranslatedMaxLine
and mStart on the line before that (for a span of 1).

I'll add some tests that triggers this case too...

Comment 48

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #47)
> >   if (mStart > kTranslatedMaxLine) {
> 
> I think ">=" is the right test there to put mEnd at kTranslatedMaxLine
> and mStart on the line before that (for a span of 1).

Ah, exactly right.

Comment 49

[Daniel Holbert [:dholbert]]

Comment on attachment 8599434 [details] [diff] [review]
part 6bv2, additional clamping in ResolveAutoPosition

Review of attachment 8599434 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.h
@@ +127,5 @@
>        MOZ_ASSERT(aStart >= 0, "expected a zero-based line number");
>        mStart = aStart;
>        mEnd += aStart;
> +      if (MOZ_UNLIKELY(mStart >= kTranslatedMaxLine)) {
> +        mEnd = kTranslatedMaxLine;

Nit: really, we might as well sanity-check mStart (or even just aStart) right away, before we set anything.

If it's too large, the "mEnd += aStart" line will have been unnecessary/wasteful, since we'll immediately assign mEnd to something else (kTranslatedMaxLine) inside the "if" block.

Maybe better (and 2 lines shorter) as:
  [existing MOZ_ASSERT lines]
  // Clamping per http://dev.w3.org/csswg/css-grid/#overlarge-grids :
  if (MOZ_LIKELY(aStart < kTranslatedMaxLine)) {
     mStart = aStart;
     mEnd = std::min(mEnd + mStart, kTranslatedMaxLine);
  } else {
     mEnd = kTranslatedMaxLine;
     mStart = mEnd - 1;
  }

Perhaps it's clearer as-is, though (handling the normal case straight away, and then handling special cases as an afterthought, albeit with a redundant mEnd assignment). So, r=me either way.

Comment 50

[Mats Palmgren (inactive)]

Attachment: part 7b, make FindNamedLine etc return int32_t

To address bug 1147423 comment 13.
(I'll fold this into part 7 before landing)

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8599451 [details] [diff] [review]
part 7b, make FindNamedLine etc return int32_t

This is good, but there are more pre-translation line numbers that'd need to be converted to int32_t (for "int32_t signals pre-translated lines" per bug 1147423 comment 13) -- namely:
 - "aImplicitLine" parameters
 - "uint32_t line;" in FindLine/RFindLine
 - mExplicitGridRowEnd, mExplicitGridColEnd, and their function-parameter derivatives called "aExplicitGridEnd"

(This conversion might just want its own bug?)

Comment 52

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #43)
> > I think this should be operating on mUntranslatedStart/mUntranslatedEnd,
> > too, right? (I think this is before PlaceGridItems has done the translation
> > & initialized mStart/mEnd.)
> 
> No, this is ResolveAutoPosition() which is after the translation occured.
> (everything related to auto-positioning and layout is after)

(Yeah, you're right -- sorry. This question was premised on my faulty assumption about "auto" only existing pre-translation.)

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8599272 [details] [diff] [review]
part 6, v2 - [css-grid] Make the grid zero-based after resolving definite lines.

r=me on part 6

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8599273 [details] [diff] [review]
part 7, v2 - [css-grid] Make LineRange::mStart/mEnd into a union with both a signed/unsigned members to make it clear when we're working with translated line numbers and when not.

r=me on part 7

(So, I think the only to-be-addressed review comments that remain in this bug are in comment 49 & comment 51.)

Comment 55

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] (out of office 4/30 & 5/1) from comment #49)
> If it's too large, the "mEnd += aStart" line will have been
> unnecessary/wasteful

That's MOZ_UNLIKELY. :-)

> Perhaps it's clearer as-is, though (handling the normal case straight away,
> and then handling special cases as an afterthought

Yes, it separates the clamping which makes the code clearer, IMO.
I added the code comment you suggested about the clamping.

Comment 56

[Mats Palmgren (inactive)]

Comment on attachment 8599451 [details] [diff] [review]
part 7b, make FindNamedLine etc return int32_t

(In reply to Daniel Holbert [:dholbert] (out of office 4/30 & 5/1) from comment #51)
> (This conversion might just want its own bug?)

Filed bug 1160246 on that.

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a45e4b0f23e
https://hg.mozilla.org/integration/mozilla-inbound/rev/68280190891d
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b9547b8521
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2be01a8ae2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d65e0edabc
https://hg.mozilla.org/integration/mozilla-inbound/rev/224b82a7ed13
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1c6c1d1a35
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd838a901830
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8c24568ae4
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f190dd9090
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3116ca2e5c

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a45e4b0f23e
https://hg.mozilla.org/mozilla-central/rev/68280190891d
https://hg.mozilla.org/mozilla-central/rev/32b9547b8521
https://hg.mozilla.org/mozilla-central/rev/e2be01a8ae2c
https://hg.mozilla.org/mozilla-central/rev/a5d65e0edabc
https://hg.mozilla.org/mozilla-central/rev/224b82a7ed13
https://hg.mozilla.org/mozilla-central/rev/3e1c6c1d1a35
https://hg.mozilla.org/mozilla-central/rev/cd838a901830
https://hg.mozilla.org/mozilla-central/rev/6d8c24568ae4
https://hg.mozilla.org/mozilla-central/rev/04f190dd9090
https://hg.mozilla.org/mozilla-central/rev/1b3116ca2e5c