http://dev.w3.org/csswg/css-grid/#overlarge-grids 8. Clamping Overlarge Grids Since memory is not infinite, UAs may clamp the the possible size of the grid to within a UA-defined limit, dropping all lines outside that limit. If a grid item spans outside this limit, its span must be clamped to the last line of the limited grid. 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 on that side of the grid.

We already have "#define GRID_TEMPLATE_MAX_REPETITIONS 10000" to limit repeat() but I think specified line numbers / spans are not clamped. Perhaps 10000 is a reasonable limit in layout too? That gives us 100 million cells potentially but I suspect that will rarely be the case in practice. If it's that big it'll be very sparse and I think we might cope with that if we're careful.

The clamping should be absolute numbers of course, so -10000 to 10000 which means (max) 400 million cells, but still the same order of magnitude.

This is mostly in preparation for bug 1146051 but it made sense to do it before the next patch here to avoid having to tweak that later.

Comment on attachment 8594996 [details] [diff] [review] part 1 - [css-grid] Use a signed integer type for some line variables in preparation for negative implicit lines. So, now we'll have got some line-number variables being int32_t (the ones in this patch) and others remaining uint32_t (e.g. aExplicitGridEnd, implicitLine, & the return value of ::FindNamedLine) And we assign between them -- e.g. in nsGridContainerFrame::ResolveLine, this patch converts the local variable 'line' to be signed, but there's still code that sets it to the unsigned result of FindNamedLine(), or to the unsigned param 'aExplicitGridEnd'. Two questions (though the first may be irrelevant if the second is a "yes"): (1) What determines whether a particular 'line' variable is signed vs. unsigned? (2) Should we just make them all signed (with foreknowledge that some of them can only be nonnegative), to make assignment / type usage cleaner? I tend to think maybe we should... (If the type difference is really meaningful, we should perhaps use something more type-safe than just int32_t / uint32_t -- or even just typedefs to make things clearer.) >diff --git a/layout/generic/nsGridContainerFrame.h b/layout/generic/nsGridContainerFrame.h [...] >- uint32_t mStart; // the start line, or zero for 'auto' >- uint32_t mEnd; // the end line, or the span length for 'auto' >+ int32_t mStart; // the start line, or zero for 'auto' >+ int32_t mEnd; // the end line, or the span length for 'auto' Since negative nubmers are valid here now [given that these are becoming int32_t]: can we still treat 0 as meaning "auto", or is it actually becoming a valid index as well? (I admittedly haven't thought through this very fully yet, but it seems like a logical concern, given that up until now the lowest valid line-index was 1, and now that's no longer the case.)

Comment on attachment 8594997 [details] [diff] [review] part 2 - [css-grid] Clamp grid lines to the -10000 .. 10000 range. Review of attachment 8594997 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGridContainerFrame.cpp @@ +332,5 @@ > // (x-start x-end) > // (x-start) 0 (x-start) 0 (x-end) > // (x-end) 0 (x-start) 0 (x-end) > // (x-start) 0 (x-end) 0 (x-start) 0 (x-end) > + const uint32_t len = nit: end-of-line whitespace @@ +556,5 @@ > + r.second = std::min(r.second, nsStyleGridLine::kMaxLine - 1); > + } else if (r.second <= r.first) { > + // http://dev.w3.org/csswg/css-grid/#grid-placement-errors > + if (MOZ_UNLIKELY(r.first == nsStyleGridLine::kMaxLine)) { > + r.first = nsStyleGridLine::kMaxLine - 1; Perhaps link to http://dev.w3.org/csswg/css-grid/#overlarge-grids here. (for "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 on that side of the grid.") ::: layout/style/nsStyleStruct.cpp @@ +38,5 @@ > ~(NS_STYLE_INHERIT_MASK)) == 0, > "Not enough bits in NS_STYLE_INHERIT_MASK"); > > +const int32_t nsStyleGridLine::kMinLine = -10000; > +const int32_t nsStyleGridLine::kMaxLine = 10000; Perhaps worth adding a comment to mention that these particular values are arbitrary & should be enforced according to http://dev.w3.org/csswg/css-grid/#overlarge-grids

(In reply to Daniel Holbert [:dholbert] from comment #5) > (1) What determines whether a particular 'line' variable is signed vs. > unsigned? The explicit grid (still) only has numbers >= 1, so the result of FindNamedLine, etc can be unsigned. > (2) Should we just make them all signed (with foreknowledge that some of > them can only be nonnegative), to make assignment / type usage cleaner? > > (If the type difference is really meaningful, we should perhaps use > something more type-safe than just int32_t / uint32_t -- or even just > typedefs to make things clearer.) I think it helps that some are signed and some not. It makes it clear which variables may hold untranslated implicit lines. But yeah, I guess a typedef could do that as well. I'll consider it. I think I'd prefer to keep uint32_t for the rest though, since it's only a short and distinct phase at the beginning where line numbers can be negative. > Since negative nubmers are valid here now [given that these are becoming > int32_t]: can we still treat 0 as meaning "auto", or is it actually becoming > a valid index as well? I'm changing the representation of 'auto' in part 1 of bug 1146051. TL;DR 'auto' becomes an constant > 10000 Also note that it's part 5 in that bug that will actually resolve lines into the negative range.

In case it helps with the review - here's a rollup patch to bug 1146051 part 5 (inclusive).

ditto - including part 6.

Comment on attachment 8594997 [details] [diff] [review] part 2 - [css-grid] Clamp grid lines to the -10000 .. 10000 range. One more review comment on part 2 -- right now, LineRange's method "ResolveAutoPosition" has the following (in nsGridContainerFrame.h): > mStart = aStart; > mEnd += aStart; > } Don't we need to clamp mEnd to kMaxLine there? (This patch doesn't touch ResolveAutoPosition right now, but I think it perhaps should, to add this clamping if it's needed.)

(In reply to Daniel Holbert [:dholbert] from comment #10) > Don't we need to clamp mEnd to kMaxLine there? That's a good catch, thanks! I'll address this as a separate patch in bug 1146051 if you don't mind, since otherwise I'd just have to write it twice (before and after the translation changes).

(In reply to Mats Palmgren (:mats) from comment #7) > (In reply to Daniel Holbert [:dholbert] from comment #5) > > (1) What determines whether a particular 'line' variable is signed vs. > > unsigned? > > The explicit grid (still) only has numbers >= 1, so the result of > FindNamedLine, > etc can be unsigned. Pushing back here a bit: I think FindNamedLine(), and every pre-translated line number, should be consistent about using int32_t (signed). Even though *some* of these line-numbers are guaranteed to be >= 1, they still exist in a *space of lines* where negative numbers are valid. So we may (and do) end up comparing these unsigned variables to other potentially-negative signed variables, or assigning them into signed variables. And these signed/unsigned comparisons & assignments are messy & failure-prone. (And it makes it harder to keep track of which variables are in which pre-translated vs. post-translated number space, when there are type overlaps.) Case in point: In the latest rollup patch (attachment 8599275 [details] [diff] [review]), the function FindNamedLine() seems very odd, because: (1) Its *official* return-type is uint32_t, ...BUT... (2) Its second "return" (of two return statements) actually returns a int32_t variable. (3) At both of its call-sites, we stuff the returned value into "int32_t line" So it says it returns uint32_t, but that's really confusing, because it's *called* (and partly implemented) with an *effective* return-type of int32_t. The declared return type of uint32_t doesn't give us any strictness win; it just confuses things. Given that we've got two parallel number-spaces here (pre/post-translation) -- one of which has valid negative values, one of which is entirely-nonnegative -- I think it'd be much easier to keep track of things if we *consistently* represented pre-translated lines as "int32_t" and post-translated lines as "uint32_t". Then we'd need fewer casts, and fewer hacky implicit-conversions (like the ones at the callsite & 2nd return statement for FindNamedLine). This could easily happen in a followup -- I don't want you to have to rebase all your patches in bug 1147423. What do you think?

(In reply to Daniel Holbert [:dholbert] (out of office 4/30 & 5/1) from comment #12) > The declared return type of uint32_t doesn't give us any > strictness win; It guarantees that the function will not return negative numbers, which in general seems like a win in clarity, since the reader can exclude the possibility that the function might return negative numbers. I don't feel strongly about it though, and I do see your point about "int32_t signals pre-translated lines", so I don't mind changing these. I'll do that in bug 1146051 though, to avoid conflicts.

(In reply to Mats Palmgren (:mats) from comment #13) > It guarantees that the function will not return negative numbers, (But it doesn't guarantee this -- that's my point. Maybe it "hints" that the function will not return negative numbers :) but it's only a hint. Since at least one of our return statements is returning something signed, and the caller always puts the return-value into a signed variable, the unsigned return value is just a temporary wrapper around something that could really be negative -- hence, no guarantees. I'm pretty sure there's *logic* to ensure we return something nonnegative -- but it's the logic that protects us & gives us guarantees, not the unsigned return type in any way.) > I don't feel strongly about it though, and I do see your point about > "int32_t signals pre-translated lines", so I don't mind changing these. > > I'll do that in bug 1146051 though, to avoid conflicts. Thanks!

Comment on attachment 8594996 [details] [diff] [review] part 1 - [css-grid] Use a signed integer type for some line variables in preparation for negative implicit lines. r=me on part 1, given the improvements/followups in bug 1146051, and given the general guideline "int32_t signals pre-translated lines" to decide whether a given line-variable should be signed vs. unsigned.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa682ace7366 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7c7a133dd6

https://hg.mozilla.org/mozilla-central/rev/fa682ace7366 https://hg.mozilla.org/mozilla-central/rev/9a7c7a133dd6