This implements Step 1. in the grid item placement algorithm: http://dev.w3.org/csswg/css-grid/#auto-placement-algo which is probably best described as line-based placement here: http://dev.w3.org/csswg/css-grid/#line-placement with some error resolution here: http://dev.w3.org/csswg/css-grid/#grid-placement-errors

Comment on attachment 8421973 [details] [diff] [review] part 1, Implement line-based placement First wave of feedback on part 1 -- mostly just style nits for now (haven't fully groked the patch yet) ># User Mats Palmgren <matspal@gmail.com> >Bug 1009776 - part 1, Implement line-based placement. r=dholbert (maybe add "...for CSS grid"?) >diff --git a/layout/generic/nsGridContainerFrame.cpp b/layout/generic/nsGridContainerFrame.cpp >+/** >+ * Search for the aNth occurence 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 anImplicitLine. >+ * Return the last match if aNth occurences can't be found, or zero if no >+ * occurence can be found. typo: "occurrence" has a double-r, not a single-r. (3 instances in this paragraph) >+ */ >+static uint32_t >+FindLine(const nsString& aName, uint32_t aNth, >+ uint32_t aFromIndex, uint32_t anImplicitLine, >+ const nsTArray<nsTArray<nsString>>& aNameList) >+{ Should this be "aImplicitLine"? (not "an"?) >+ MOZ_ASSERT(aNth > 0); In the caller of this function (FindNamedLine), there's an identical assertion, but with != 0 instead of > 0. Let's make those consistent. (I marginally prefer != 0, since it feels more explicit about what's being checked, given that the variable is unsigned.) >+const GridNamedArea* >+nsGridContainerFrame::FindNamedArea(const nsSubstring& aName) >+{ >+ const nsStylePosition& pos = *StylePosition(); As in bug 1009282, we probably should be dealing with nsStylePosition as a pointer here, for consistency with how other layout/style-system code works with style structs. (FWIW, the only instance of "nsStylePosition&" in our code right now is in nsStylePosition's copy-constructor and CalcDifference() method. Everything else uses/passes around nsStylePosition* pointers.) (And where possible, we should probably be using the already-cached nsStylePosition pointer from the nsHTMLReflowState, instead of looking it up again. I'm not sure whether it's better pass around the reflowstate itself or just to to pass around a nsStylePosition* that ::Reflow() grabs off of the reflow state, but one of those may be a good idea.) (This applies to a few other functions in this patch, too.) >+ // If mLineName ends in -start/-end, try the prefix as a named area. >+ auto implicitLine = 0U; [...] >+ if (area) { >+ implicitLine = area->*areaEdge; >+ } I don't think you want "auto" here. IIUC, the point of auto is to make types match without needing to restate the type. But the type that we *care* about matching here -- the type of area->*areaEdge, and of the function-params that we end up passing implicitLine for later on -- is not the one that "auto" ends up copying here. Instead, it copies the type of 0U, which is just 'unsigned'. (not uint32_t -- which is really the type that it wants, to match those other things) In practice, probably not a big deal, but it feels a bit broken to have this type mismatch here. So, I'd prefer s/auto/uint32_t/ on this particular variable. >diff --git a/layout/generic/nsGridContainerFrame.h b/layout/generic/nsGridContainerFrame.h > /** >+ * 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 >+ * (both 1-based) where mStart < mEnd. Before it's definite it can also >+ * represent an auto position with a span, where mStart == 0 and mEnd is >+ * the (non-zero positive) span. >+ * In both states the invariant mEnd > mStart holds. Probably worth asserting this invariant ^^ in a few places -- at least in the first constructor, and in the Extent() impl (where we subtract mStart - mEnd, which would be bogus if the assertion failed). >+ /** >+ * Return a LineRange based on the given style data, non-auto lines >+ * are resolved to a definite line number per: s/data, non/data. Non/

Comment on attachment 8421973 [details] [diff] [review] part 1, Implement line-based placement Review of attachment 8421973 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay on this. More notes on part 1: ::: layout/generic/nsGridContainerFrame.cpp @@ +389,5 @@ > + const nsStylePosition& pos = *StylePosition(); > + uint32_t colEnd = pos.mGridTemplateColumns.mLineNameLists.Length(); > + uint32_t rowEnd = pos.mGridTemplateRows.mLineNameLists.Length(); > + mozilla::css::GridTemplateAreasValue* gridTemplateAreas = > + pos.mGridTemplateAreas; Add "using namespace mozilla;" up top, so we don't need to bother with the mozilla:: here. (Or perhaps better, "using mozilla::css::GridTemplateAreasValue", so we don't need any prefixing) Either way, that'll lets this assignment fit on one line. @@ +392,5 @@ > + mozilla::css::GridTemplateAreasValue* gridTemplateAreas = > + pos.mGridTemplateAreas; > + if (gridTemplateAreas) { > + colEnd = std::max(colEnd, gridTemplateAreas->mNColumns + 1); > + rowEnd = std::max(rowEnd, gridTemplateAreas->NRows() + 1); This needs: s/colEnd/*colEnd/g s/rowEnd/*rowEnd/g (two instances of each) Though as noted in my comment below for PlaceGridItems(), I think it'd be cleaner to just directly modify the member-vars (like mExplicitGridRowEnd) here. @@ +395,5 @@ > + colEnd = std::max(colEnd, gridTemplateAreas->mNColumns + 1); > + rowEnd = std::max(rowEnd, gridTemplateAreas->NRows() + 1); > + } > + *aColEnd = std::max(colEnd, 1U); > + *aRowEnd = std::max(rowEnd, 1U); The "}" there should probably be "} else {". (If we entered the "if (gridTemplateAreas)" block, then we'll already have clamped to be >= 1, due to the $unsignedVal+1 clamping that we're already doing there.) Also, as above, this needs a "*" on both of colEnd/rowEnd inside of the ::max() calls. @@ +403,5 @@ > +nsGridContainerFrame::PlaceGridItems() > +{ > + CalculateExplicitGridBounds(&mExplicitGridColEnd, &mExplicitGridRowEnd); > + mGridColEnd = mExplicitGridColEnd; > + mGridRowEnd = mExplicitGridRowEnd; CalculateExplicitGridBounds() already has access to mExplicitGridColEnd and mExplicitGridRowEnd -- why does it also need to take them as parameters? Seems like this would be a bit cleaner if CalculateExplicitGridBounds() just directly set those member-vars, instead of taking them as params. And while we're at it, maybe it should also directly set mGridColEnd/mGridRowEnd at the end, to reduce the opportunity for letting those fall out of sync with the mExplicit* versions. ::: layout/generic/nsGridContainerFrame.h @@ +42,3 @@ > friend nsIFrame* NS_NewGridContainerFrame(nsIPresShell* aPresShell, > nsStyleContext* aContext); > nsGridContainerFrame(nsStyleContext* aContext) : nsContainerFrame(aContext) {} We probably should initialize the new member-vars (mExplicitGridColEnd, etc) in the constructor, right? (to 0, I guess?) Alternately, if you'd rather leave those member-vars explicitly-uninitialized & rely on the fact that we'll compute these before we use them, then it's probably worth stating that explicitly here.

Comment on attachment 8421973 [details] [diff] [review] part 1, Implement line-based placement Review of attachment 8421973 [details] [diff] [review]: ----------------------------------------------------------------- More on part 1: ::: layout/generic/nsGridContainerFrame.cpp @@ +231,5 @@ > + } > + // XXX must Implicit Named Areas have all four lines? > + // http://dev.w3.org/csswg/css-grid/#implicit-named-areas > + line = ::FindNamedLine(lineName, aNth, aFromIndex, implicitLine, > + aLineNameList); I think the answer to that XXX question is "no", based on http://dev.w3.org/csswg/css-grid/#grid-placement-slot which says: # if there is a named line with the name '<custom-ident>-start # (for grid-*-start) / <custom-ident>-end' (for grid-*-end), # contributes the first such line to the grid item's placement. It sounds to me like we could have a line named "foo-start" (or maybe multiple such lines), without necessarily having any line named "foo-end". @@ +241,5 @@ > + auto implicitLine = 0U; > + auto index = aLine.mLineName.RFind("-start"); > + auto GridNamedArea::* areaEdge = aAreaStart; > + if (index == kNotFound) { > + index = aLine.mLineName.RFind("-end"); Similar to in bug 1009282 comment 2, I don't think we want RFind here. It's extra-expensive (searching through the whole string) and possibly false-positivey as a result (e.g. if "-start" occurs as a substring in the middle, not as a suffix, then we'll get weird behavior here). We should just be checking the end of the string directly, using e.g. StringTail() and EqualsLiteral("-start"), I think. (and maybe a #define START_SUFFIX_LEN NS_ARRAY_LEN("-start") or something like that at the top of this file, and similar for "-end"?) @@ +246,5 @@ > + areaEdge = aAreaEnd; > + } > + if (index != kNotFound) { > + const GridNamedArea* area = > + FindNamedArea(nsDependentSubstring(aLine.mLineName, 0, index)); That nsDependentSubstring expression can be condensed a bit to: StringHead(aLine.mLineName, index); StringHead is defined here: http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsTDependentSubstring.h?rev=eb3b772bac41#114 @@ +270,5 @@ > + } > + } else { > + MOZ_ASSERT(aNth != 0); > + line = std::max(int32_t(aFromIndex) + aNth, 1); > + } Nit: perhaps this 2-line "else" clause would make more sense up top (i.e. with the initial "if (!aLine.mLineName.IsEmpty())" check's logic inverted)? This seems like an "easy" case that we can knock out before getting into the complex stuff, rather than doing all the complex stuff and tacking on this exception at the very bottom. I say this in part because having the (conditional) assertion about aNth up at the top would make the function's preconditions a bit clearer, without having to read through the whole thing. Also, could you add a message for the MOZ_ASSERT here? Note that 2 of our callers do pass 0 for aNth, so it'd be good to briefly explain why we're claiming it can't be 0 here. (presumably because those callers should take the other branch, I guess? but I'm sure it can be explained more conceptually than that. :)) @@ +273,5 @@ > + line = std::max(int32_t(aFromIndex) + aNth, 1); > + } > + // The only case which can result in "auto" is a plain <custom-ident> (without > + // <integer> or 'span') if it's not found. > + MOZ_ASSERT(line != 0 || (!aLine.mHasSpan && aLine.mInteger == 0)); The connection between the comment and the assertion isn't clear. But from reading other code in this patch, I think the connection is that line==0 indicates "auto" here, right? Maybe reword to make that more explicit. And ideally, the comment should be converted into an assertion-message, so that we get more useful output (and more searchable bug reports) if/when Jesse makes the assertion fail. @@ +296,5 @@ > + return LineRange(0, aStart.mInteger); > + } > + // span <custom-ident> / span * > + // span <custom-ident> / auto > + return LineRange(0, 1); The hardcoded span of "1" here isn't necessarily correct if we're a subgrid. (when we add support for subgrid). Maybe worth mentioning that in an XXX comment, to help when we add subgrid support? Though, this actually probably applies to a lot of this function... maybe just mention somewhere that all of the "auto" & error-handling cases will need more nuance & can't assume a span of 1 when we add subgrid support? @@ +359,5 @@ > + MOZ_ASSERT(r.mEnd != 0); > + > + // http://dev.w3.org/csswg/css-grid/#grid-placement-errors > + if (r.mEnd <= r.mStart) { > + r.mEnd = r.mStart + 1; IIUC, we're supposed to basically behave as if the "grid-*-end" property were "auto" in this error case. "auto" means a span of 1 (hence your +1 here), *unless* we're a subgrid, in which case it's more complicated. You should probably mention that in an XXX comment here. (We don't support subgrid in layout yet, of course, but it's nice to leave hints where things will need to be changed / broadened.) ::: layout/generic/nsGridContainerFrame.h @@ +91,5 @@ > + * Return a line number for (non-auto) aLine, per: > + * http://dev.w3.org/csswg/css-grid/#line-placement > + * @param aLine style data for the line (must be non-auto) > + * @param aNth a number of lines to find from aFromIndex, negative if the > + * search should be in reverse order (^this is the documentation for ResolveLine()) Could you document what it means for aNth to be 0? Two of the callers pass 0, and I don't immediately understand what that means. In one of the code-paths, ResolveLine checks aNth for 0 and replaces it with 1 right away; and if it takes the other code-path, it asserts that aNth can *not* be 0. Maybe better to just completely prohibit it from being 0, and depend on the callers to pass in 1 when they want 1? (instead of converting 0 to 1? Or if that makes the callers need to be more complex, then just document the 0 behavior.

(In reply to Daniel Holbert [:dholbert] from comment #6) Fixed all the nits in this comment, except this one: > >+ * In both states the invariant mEnd > mStart holds. > > Probably worth asserting this invariant ^^ in a few places Actually, I discarded this invariant in a later patch that implements abs.pos. grid items, where zero means "attach this side to the container edge". The code comment is removed in that later patch. (In reply to Daniel Holbert [:dholbert] from comment #7) > Add "using namespace mozilla;" up top, so we don't need to bother with the > mozilla:: here. (Or perhaps better, "using > mozilla::css::GridTemplateAreasValue", so we don't need any prefixing) I added "using namespace mozilla;" since that's probably good in general and then used 'auto' for this specific declaration. > Though as noted in my comment below for PlaceGridItems(), I think it'd be > cleaner to just directly modify the member-vars (like mExplicitGridRowEnd) > here. Yeah, I re-wrote this method to do that and renamed it InitializeGridBounds. > ::: layout/generic/nsGridContainerFrame.h > > nsGridContainerFrame(nsStyleContext* aContext) : ... > > We probably should initialize the new member-vars (mExplicitGridColEnd, etc) > in the constructor, right? (to 0, I guess?) All new frame class instances are already zeroed out by the allocator (the shell arena) so there's no need to initialize members to zero/ false/nullptr etc. (In reply to Daniel Holbert [:dholbert] from comment #8) > > + // XXX must Implicit Named Areas have all four lines? > > I think the answer to that XXX question is "no", based on Reading the spec again, I still think it's ambiguous, so I think I'll ask on www-style for clarification. > Similar to in bug 1009282 comment 2, I don't think we want RFind here. Yep, fixed that. > > + const GridNamedArea* area = > > + FindNamedArea(nsDependentSubstring(aLine.mLineName, 0, index)); > > That nsDependentSubstring expression can be condensed a bit to: > > StringHead(aLine.mLineName, index); I'm not convinced about the virtues of StringHead to be honest. I think nsDependentSubstring is much more clear that it's not making a string copy for example. Also, I don't see much use of StringHead in our code so I think we should consider deprecating it. > Nit: perhaps this 2-line "else" clause would make more sense up top Fixed. > Also, could you add a message for the MOZ_ASSERT here? Fixed. > > + // The only case which can result in "auto" is a plain <custom-ident> (without > > + // <integer> or 'span') if it's not found. > > + MOZ_ASSERT(line != 0 || (!aLine.mHasSpan && aLine.mInteger == 0)); > > The connection between the comment and the assertion isn't clear. But from > reading other code in this patch, I think the connection is that line==0 > indicates "auto" here, right? Maybe reword to make that more explicit. And > ideally, the comment should be converted into an assertion-message, so that > we get more useful output (and more searchable bug reports) if/when Jesse > makes the assertion fail. Fixed. > @@ +296,5 @@ > > + return LineRange(0, aStart.mInteger); > > + } > > + // span <custom-ident> / span * > > + // span <custom-ident> / auto > > + return LineRange(0, 1); > > The hardcoded span of "1" here isn't necessarily correct if we're a subgrid. This code is doing the 2nd & 3rd paragraphs of 9.2.1: http://dev.w3.org/csswg/css-grid/#grid-placement-errors which explicitly says "one". Does the spec say somewhere else that this "one" should be the explicit subgrid size? > > + MOZ_ASSERT(r.mEnd != 0); > > + > > + // http://dev.w3.org/csswg/css-grid/#grid-placement-errors > > + if (r.mEnd <= r.mStart) { > > + r.mEnd = r.mStart + 1; > > IIUC, we're supposed to basically behave as if the "grid-*-end" property > were "auto" in this error case. > > "auto" means a span of 1 (hence your +1 here), *unless* we're a subgrid, in > which case it's more complicated. You should probably mention that in an > XXX comment here. This code is doing the 1st paragraph of 9.2.1 - same reply as above. > (We don't support subgrid in layout yet, of course, but it's nice to leave > hints where things will need to be changed / broadened.) I agree. > ::: layout/generic/nsGridContainerFrame.h > (^this is the documentation for ResolveLine()) > > Could you document what it means for aNth to be 0? ResolveLineRangeHelper() would need to have that logic duplicated in a number of places so it's more convenient to do it in ResolveLine(). I've added the requested documentation in the header. (In the case we're given aLine without a specified line name then we can say for certain that aNth must be non-zero or the style system screwed up (all the <integer>s in 9.2 must be non-zero) so it seemed worth asserting that.)

BTW, the spec has changed one detail regarding creating implicit lines - they can now expand before the start row/column of the explicit grid. I plan to address that later as a separate bug.

(In reply to Mats Palmgren (:mats) from comment #9) > > > + // XXX must Implicit Named Areas have all four lines? > > > > I think the answer to that XXX question is "no", based on > > Reading the spec again, I still think it's ambiguous, so I think > I'll ask on www-style for clarification. (Remember to send this post, if you haven't already done so) > > The hardcoded span of "1" here isn't necessarily correct if we're a subgrid. > > This code is doing the 2nd & 3rd paragraphs of 9.2.1: > http://dev.w3.org/csswg/css-grid/#grid-placement-errors > which explicitly says "one". Does the spec say somewhere else that this > "one" should be the explicit subgrid size? [...] > This code is doing the 1st paragraph of 9.2.1 - same reply as above. My concern here was probably based on section 9.5, http://dev.w3.org/csswg/css-grid/#auto-placement-algo , which says "Grid spans ... default to either 1 or, for subgrids, the size of their explicit grid." It sounds like maybe that's irrelevant for this chunk of code, though -- sorry if so.

Comment on attachment 8566028 [details] [diff] [review] part 1, [css-grid] Implement line-based placement. Review of attachment 8566028 [details] [diff] [review]: ----------------------------------------------------------------- part 1 review, first pass: ========================== First, a general comment -- after re-reading this spec & your patch, I'm realizing that (I think?) authors can do something crazy like the following: - Create a named area called "foo-start" (with implicit lines "foo-start-start" and "foo-start-end") - Also create a named area "foo" (or, alternately: create explicit lines "foo-start" and "foo-end") In this situation, a value like "grid-row-start: foo-start" or "grid-row-end: foo-start" would get interesting, per http://dev.w3.org/csswg/css-grid/#grid-placement-slot -- the spec says to check the <custom-ident> for an exact match against named areas *first* (which your patch does, I think). So the upshot is that these properties would end up picking the implicit grid lines "foo-start-start" and "foo-start-end" -- and *not* picking the exactly-matching grid line name, "foo-start". We need tests for this sort of ambiguous scenario, if you don't already have them. ::: layout/generic/nsGridContainerFrame.cpp @@ +17,5 @@ > > +/** > + * 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. It'd be worth documenting that aImplicitLine can be 0, which means no unconditional match (I think). (This is the FindLine documentation, if it's not clear from splinter-context.) @@ +32,5 @@ > + uint32_t lastFound = 0; > + uint32_t line; > + uint32_t i = aFromIndex; > + for (; i < len; i = line) { > + line = i + 1; I'd lean towards "++i" instead of "i = line" here, in the for-loop-counter-updating expression -- that keeps this closer to the standard for-loop idiom, which makes it more skimmable. The "i = line" version might save us a tiny amount of math, but it also suggests (to me at least) that "line" might be modified in several places, and we're trying to keep our loop-counter "i" in step with it. Also, this assignment mixes up an 0-based index with a 1-based index a little more than I'd like. (We're already intermixing them a little with "line = i + 1", but that seems not as bad, since it's more explicit.) @@ +42,5 @@ > + } > + } > + if (aImplicitLine > i) { > + // aImplicitLine is after the lines we searched above so it's last. > + lastFound = aImplicitLine; I think we can only hit this case if grid-template-areas has more tracks in this dimension than grid-template-[rows|columns], right? If so, maybe worth mentioning that in this comment (since this seems odd/impossible, if you don't have that possibility in your mind). @@ +229,5 @@ > + MOZ_ASSERT(aNth != 0, "css-grid 9.2: <integer> must not be zero."); > + line = std::max(int32_t(aFromIndex) + aNth, 1); > + } else { > + if (aNth == 0) { > + aNth = 1; This 0-to-1 correction is a bit hand-wavy / magical. Add a brief comment. (e.g. "<integer> was omitted; treat it as 1" -- I think that's what's going on in the cases that you're covering here, at least.) @@ +289,5 @@ > + line = aNth >= 0 ? 1 : aExplicitGridEnd; > + } else { > + // http://dev.w3.org/csswg/css-grid/#grid-placement-slot > + // Treat '<custom-ident>' as 'auto'. > + // (fall through, returning zero) I don't see support for this chunk (treating a not-found '<custom-ident>' as 'auto') in the spec. Based on http://dev.w3.org/csswg/css-grid/#grid-placement-slot , I'd think the spec says the following about this case: 1) First we treat '<custom-ident>' as a named area, and find nothing. 2) Then, the spec tells us "treat this as if the integer 1 had been specified along with the <custom-ident>." This makes us fall through to the <integer> && <custom-ident>? case. 3) That case says: "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." I would consider "no matches" to be included in "not enough lines with that name exist". 4) So I think this means we match the first line (since we end up treating all lines as if they match, and we have a dummy integer of '1'). So, that leads me to believe we should do "line = 1" here (instead of falling through & returning zero as the patch currently does). Am I missing/misunderstanding something? @@ +296,5 @@ > + } > + // The only case which can result in "auto" (line == 0) is a plain > + // <custom-ident> (without <integer> or 'span') which wasn't found. > + MOZ_ASSERT(line != 0 || (!aLine.mHasSpan && aLine.mInteger == 0), > + "Given a <integer> or 'span' the result should never be auto"); If you agree with me on the comment directly-above this one, then I think this assertion can be simplified to just check that line != 0. (similarly, the logic at the call-sites (in ResolveLineRangeHelper) would change a bit as well, since it would no longer have to check for this function (ResolveLine) returning 0.) @@ +386,5 @@ > + aAreaEnd, aExplicitGridEnd, aStyle); > + MOZ_ASSERT(r.mEnd != 0); > + > + // http://dev.w3.org/csswg/css-grid/#grid-placement-errors > + if (r.mEnd <= r.mStart) { This check implies that we're (briefly at least) creating LineRange objects whose mEnd <= mStart, in violation of the LineRange documentation about the valid states that LineRange objects can be in. (I suppose it'd violate the assertion I suggest adding to the LineRange constructor elsewhere in this comment, too.) That seems a big bogus. We should either explicitly prevent these invalid states, or else indicate somewhere (maybe in the ResolveLineRangeHelper documentation?) to indicate that it can produce invalid LineRange objects, and the caller is responsible for cleaning them up. @@ +398,5 @@ > + const nsStylePosition* aStyle) > +{ > + const nsStylePosition* item = aChild->StylePosition(); > + return GridArea( > + ResolveLineRange(item->mGridColumnStart, item->mGridColumnEnd, Maybe s/item/itemStyle/? @@ +414,5 @@ > +{ > + // http://dev.w3.org/csswg/css-grid/#grid-definition > + uint32_t colEnd = aStyle->mGridTemplateColumns.mLineNameLists.Length(); > + uint32_t rowEnd = aStyle->mGridTemplateRows.mLineNameLists.Length(); > + auto areas = aStyle->mGridTemplateAreas; aStyle->mGridTemplateAreas has type nsRefPtr<mozilla::css::GridTemplateAreasValue>, so I think that means this local-variable "areas" will also be a nsRefPtr. So, this means we'll be doing an unnecessary AddRef/Release here. To prevent this, I think we should add ".get()" to the end of aStyle->mGridTemplateAreas, so the "auto" type will just be an actual raw pointer. ::: layout/generic/nsGridContainerFrame.h @@ +53,5 @@ > + * In both states the invariant mEnd > mStart holds. > + */ > + struct LineRange { > + LineRange(uint32_t aStart, uint32_t aEnd) > + : mStart(aStart), mEnd(aEnd) {} Add an assertion in the constructor here, to verify that mEnd > mStart. (You document this expectation, but we should sanity-check it too.) @@ +55,5 @@ > + struct LineRange { > + LineRange(uint32_t aStart, uint32_t aEnd) > + : mStart(aStart), mEnd(aEnd) {} > + LineRange(const LineRange& aOther) > + : mStart(aOther.mStart), mEnd(aOther.mEnd) {} You can get rid of this copy-constructor -- it's identical to the one that the compiler should auto-generate for you. @@ +84,5 @@ > + * @param aStyle the StylePosition() for the grid container > + * @return null if not found > + */ > + static const GridNamedArea* FindNamedArea(const nsSubstring& aName, > + const nsStylePosition* aStyle); (optional: looks like this could just be a static helper-function in the .cpp file, rather than an actual static class-method; it doesn't use any private/protected nsGridContainerFrame stuff, so it doesn't really need to live in the class, & hence we could shorten the .h file a bit by punting it to the .cpp file.) @@ +120,5 @@ > + /** > + * Return a LineRange based on the given style data. Non-auto lines > + * are resolved to a definite line number per: > + * http://dev.w3.org/csswg/css-grid/#line-placement > + * and placement errors have been corrected per: The phrase "have been corrected" is a bit temporally-ambiguous here. Maybe replace with "will be corrected before this function returns"? @@ +146,5 @@ > + * 'auto'. > + * @param aChild the grid item > + * @param aStyle the StylePosition() for the grid container > + */ > + GridArea PlaceDefinite(nsIFrame* aChild, const nsStylePosition* aStyle); Maybe s/aStyle/aGridStyle/ (or aGridStylePos), to make it more superficially clear that this is associated with the *grid*, not with aChild. (I guess a lot of the new functions here take this |aStyle| param, so if you wanted to act on this, you'd want to do it across the board; feel free to ignore/punt on it for now.)

Comment on attachment 8566028 [details] [diff] [review] part 1, [css-grid] Implement line-based placement. Review of attachment 8566028 [details] [diff] [review]: ----------------------------------------------------------------- part 1 review, second pass (of 2 passes total): =============================================== Not marking r+ yet, because I'm not clear on some parts (in particular the not-found <custom-ident> behavior); but once I'm on the same page as you there, this is probably r+ w/ comments addressed. ::: layout/generic/nsGridContainerFrame.cpp @@ +330,5 @@ > + if (end == 0) { > + // span * / <custom-ident> that can't be found > + return LineRange(0, aStart.mInteger); > + } > + int32_t span = std::max(aStart.mInteger, 1); Why this std::max with 1 here? Is this another "treat 0 [no-integer-provided] as 1" scenario? In particular: I *think* 0 is the only value we're correcting here, right? Because, aStart.mInteger is signed & hence could in theory be negative, but it looks like 'span negative-integer' is invalid & should've been rejected by the parser, which means we shouldn't get here with a negative aStart.mInteger. This is all non-obvious, though. So: maybe assert that it's nonnegative, and/or document that we're just fixing up "0" here, as as "no integer provided" sentinel (& we're not expecting/fixing-up negative values). @@ +334,5 @@ > + int32_t span = std::max(aStart.mInteger, 1); > + auto start = ResolveLine(aStart, -span, end, aLineNameList, aAreaStart, > + aAreaEnd, aExplicitGridEnd, eLineRangeSideStart, > + aStyle); > + MOZ_ASSERT(start > 0); Why do we know that the ResolveLine return value must be nonzero here? (whereas in the previous call we have to explicitly check if it's 0 & handle that case) If this stays like this*, we need a comment (or a message in the MOZ_ASSERT) to explain why we know this second call can't return 0. * (Per my previous comment's notes on ResolveLine -- starting w/ "I don't see support for" -- I'm not sure ResolveLine should *ever* be able to return 0. So, this suggestion here might become obsolete, if you agree on that point.) @@ +355,5 @@ > + MOZ_ASSERT(aEnd.mInteger != 0); > + return LineRange(0, aEnd.mInteger); > + } > + // auto (or not found <custom-ident>) / span <custom-ident> > + return LineRange(0, 1); It's non-obvious why this syntax in your comment triggers "return LineRange(0, 1);" here. I think it's because of http://dev.w3.org/csswg/css-grid/#grid-placement-errors -- please add a link to that section here, and/or add a comment like "A span to a named line is treated as a span of 1, when combined with automatic position." (Also: For reasons described above in my previous bug-comment (after "I don't see support for"): are you sure that a not-found <custom-ident> should cause auto-placement? From my reading of spec section 9.2, it should end up being treated as "1". This applies to various places where you have a "not-found <custom-ident>" comment.) @@ +430,5 @@ > + // http://dev.w3.org/csswg/css-grid/#auto-placement-algo > + // Step 1, place definite positions. > + for (nsFrameList::Enumerator e(PrincipalChildList()); !e.AtEnd(); e.Next()) { > + nsIFrame* child = e.get(); > + PlaceDefinite(child, aStyle); Just to make sure I'm understanding correctly -- this patch doesn't actually affect our observable behavior, right? I think PlaceDefinite (which does the bulk of the work here) doesn't actually modify anything -- it just returns a GridArea to indicate the child's position/size. And we're not capturing that GridArea yet (though you do in a later patch), so I'd expect behavior to be unaffected until we start using this GridArea.

(In reply to Daniel Holbert [:dholbert] from comment #15) > My concern here was probably based on section 9.5, > http://dev.w3.org/csswg/css-grid/#auto-placement-algo , which says "Grid > spans ... default to either 1 or, for subgrids, the size of their explicit > grid." Hmm, possibly. It's hard to tell what the spec authors intend since the quote above contradicts the spec in other places. Let's not worry too much about subgrids for now since we're not implementing them in the first round. Anyway, I've added XXX comments in a few places that assigns the span to '1' which might be wrong for subgrid. (In reply to Daniel Holbert [:dholbert] from comment #16) > First, a general comment -- after re-reading this spec & your patch, I'm > realizing that (I think?) authors can do something crazy like the following: > - Create a named area called "foo-start" (with implicit lines > "foo-start-start" and "foo-start-end") > - Also create a named area "foo" (or, alternately: create explicit lines > "foo-start" and "foo-end") Yes, that is correct. > In this situation, a value like "grid-row-start: foo-start" or > "grid-row-end: foo-start" would get interesting, per > http://dev.w3.org/csswg/css-grid/#grid-placement-slot -- the spec says to > check the <custom-ident> for an exact match against named areas *first* > (which your patch does, I think). So the upshot is that these properties > would end up picking the implicit grid lines "foo-start-start" and > "foo-start-end" -- and *not* picking the exactly-matching grid line name, > "foo-start". Correct. > We need tests for this sort of ambiguous scenario, if you don't already have > them. I have tests for this. > ::: layout/generic/nsGridContainerFrame.cpp > It'd be worth documenting that aImplicitLine can be 0, which means no > unconditional match (I think). Sure, fixed. > > + for (; i < len; i = line) { > > + line = i + 1; > > I'd lean towards "++i" instead of "i = line" here, in the This is a somewhat hot path, so I think I'll keep it for performance. Besides, the loop body here is 5 simple lines so it's easy to grasp. > > + if (aImplicitLine > i) { > > + // aImplicitLine is after the lines we searched above so it's last. > > + lastFound = aImplicitLine; > > I think we can only hit this case if grid-template-areas has more tracks in > this dimension than grid-template-[rows|columns], right? Right. > If so, maybe worth mentioning that in this comment Done. > This 0-to-1 correction is a bit hand-wavy / magical. > > Add a brief comment. (e.g. "<integer> was omitted; treat it as 1" Sure, done. > > + // http://dev.w3.org/csswg/css-grid/#grid-placement-slot > > + // Treat '<custom-ident>' as 'auto'. > > + // (fall through, returning zero) > > I don't see support for this chunk (treating a not-found '<custom-ident>' as > 'auto') in the spec. Good catch; when I originally wrote this code the spec said: "Otherwise, the property contributes nothing (just as if auto were specified)." > [...] I would consider "no matches" to be included in "not enough > lines with that name exist". Me too. > 4) So I think this means we match the first line (since we end up treating > all lines as if they match, and we have a dummy integer of '1'). Yeah, "line = 1" is OK for now, but we're really supposed to take the implicit grid into account here. I'll add a XXX comment about it and address that together with comment 14. > If you agree with me on the comment directly-above this one, then I think > this assertion can be simplified to just check that line != 0. > > (similarly, the logic at the call-sites (in ResolveLineRangeHelper) would > change a bit as well, since it would no longer have to check for this > function (ResolveLine) returning 0.) I'll punt on these for the same reason. It's not clear what changes are necessary at this point so I'd rather adjust these later. > > + // http://dev.w3.org/csswg/css-grid/#grid-placement-errors > > + if (r.mEnd <= r.mStart) { > > This check implies that we're (briefly at least) creating LineRange objects > whose mEnd <= mStart, in violation of the LineRange documentation In this prototype state where the lines aren't fully resolved - yes. I fixed this by making the helper function return a std::pair instead to avoid any confusion about LineRange invariants. > > + const nsStylePosition* item = aChild->StylePosition(); > > + return GridArea( > > + ResolveLineRange(item->mGridColumnStart, item->mGridColumnEnd, > > Maybe s/item/itemStyle/? Fixed. > To prevent this, I think we should add ".get()" to the end of > aStyle->mGridTemplateAreas, so the "auto" type will just be an actual raw > pointer. Fixed. > ::: layout/generic/nsGridContainerFrame.h > Add an assertion in the constructor here, to verify that mEnd > mStart. (You > document this expectation, but we should sanity-check it too.) As I said in an earlier comment, I discarded this invariant in a later patch that implements abs.pos. grid items, where zero means "attach this side to the container edge". > > + LineRange(const LineRange& aOther) > > + : mStart(aOther.mStart), mEnd(aOther.mEnd) {} > > You can get rid of this copy-constructor -- it's identical to the one that > the compiler should auto-generate for you. Right, I used to have an assertion here in some patch, but later decided to remove it. Fixed. > > + static const GridNamedArea* FindNamedArea(const nsSubstring& aName, > > + const nsStylePosition* aStyle); > > (optional: looks like this could just be a static helper-function in the > .cpp file, rather than an actual static class-method; Good point, fixed. > > + * Return a LineRange based on the given style data. Non-auto lines > > + * are resolved to a definite line number per: > > + * http://dev.w3.org/csswg/css-grid/#line-placement > > + * and placement errors have been corrected per: > > The phrase "have been corrected" is a bit temporally-ambiguous here. > > Maybe replace with "will be corrected before this function returns"? I think the "before this function returns" is implicit, the paragraph starts with "Return a LineRange..." after all. But yeah, it doesn't read that well. I think the problem is "have been" doesn't match "are resolved" since it's just one sentence with two URLs breaking it up. So I changed it to: * Return a LineRange based on the given style data. Non-auto lines * are resolved to a definite line number per: * http://dev.w3.org/csswg/css-grid/#line-placement * with placement errors corrected per: * http://dev.w3.org/csswg/css-grid/#grid-placement-errors This reads better in my mind, and also connects the "corrected" to the outcome of the placement per the first URL. > > + * @param aStyle the StylePosition() for the grid container > > + */ > > + GridArea PlaceDefinite(nsIFrame* aChild, const nsStylePosition* aStyle); > > Maybe s/aStyle/aGridStyle/ (or aGridStylePos), to make it more superficially > clear that this is associated with the *grid*, not with aChild. The @param comment is clear IMO, but I'll consider it for a later patch. (I'd rather not introduce conflicts with all the later patches for this...) (In reply to Daniel Holbert [:dholbert] from comment #17) > > + int32_t span = std::max(aStart.mInteger, 1); > > Why this std::max with 1 here? Is this another "treat 0 > [no-integer-provided] as 1" scenario? > In particular: I *think* 0 is the only value we're correcting here, right? Yes. The style system rejects specified zero/negative integers in 'span'. It uses zero internally to encode "span A", i.e. no-integer-provided. > So: maybe assert that it's nonnegative, I changed the code to just check aStart.mInteger == 0. I'm not keen to assert every possible invariant that the style system should maintain in the layout code here, since it just clutters the code and makes it harder to read. The reader is expected to understand the style system representation for various specified values to some degree. > > + int32_t span = std::max(aStart.mInteger, 1); > > + auto start = ResolveLine(aStart, -span, end, aLineNameList, aAreaStart, > > + aAreaEnd, aExplicitGridEnd, eLineRangeSideStart, > > + aStyle); > > + MOZ_ASSERT(start > 0); > > Why do we know that the ResolveLine return value must be nonzero here? The only case where a specifed span can resolve to 'auto' is "span * / span *" where end span is ignored (i.e. becomes 'auto'). http://dev.w3.org/csswg/css-grid/#grid-placement-errors http://dev.w3.org/csswg/css-grid/#grid-placement-span-int > (whereas in the previous call we have to explicitly check if it's 0 & handle > that case) The difference is that this code is conditioned on 'aStart.mHasSpan'. There's no condition on |end|. (Well, we did handle the error cases "aEnd.mHasSpan || aEnd.IsAuto()" up front, but except for those.) > * (Per my previous comment's notes on ResolveLine -- starting w/ "I don't > see support for" -- I'm not sure ResolveLine should *ever* be able to return > 0. So, this suggestion here might become obsolete, if you agree on that > point.) Yeah, as I said above, the spec has changed since this code was written. I believe you're right that ResolveLine shouldn't return zero (auto) after being updated to the new spec. As I said, I'd rather deal with that as an incremental change. > > + // auto (or not found <custom-ident>) / span <custom-ident> > > + return LineRange(0, 1); > > It's non-obvious why this syntax in your comment triggers "return > LineRange(0, 1);" here. Added the suggested spec link. > > + // http://dev.w3.org/csswg/css-grid/#auto-placement-algo > > + // Step 1, place definite positions. > > + for (nsFrameList::Enumerator e(PrincipalChildList()); !e.AtEnd(); e.Next()) { > > + nsIFrame* child = e.get(); > > + PlaceDefinite(child, aStyle); > > Just to make sure I'm understanding correctly -- this patch doesn't actually > affect our observable behavior, right? Correct. None of the patches in this bug does, since we're not reflowing items yet.

(In reply to Mats Palmgren (:mats) from comment #18) > > 4) So I think this means we match the first line (since we end up treating > > all lines as if they match, and we have a dummy integer of '1'). > > Yeah, "line = 1" is OK for now, but we're really supposed to take the > implicit grid into account here. I'll add a XXX comment about it and > address that together with comment 14. (optional: perhaps worth filing these bug(s) now, to get a bug number which you can use in these XXX comments.) > > This check implies that we're (briefly at least) creating LineRange objects > > whose mEnd <= mStart, in violation of the LineRange documentation > > In this prototype state where the lines aren't fully resolved - yes. > I fixed this by making the helper function return a std::pair instead > to avoid any confusion about LineRange invariants. Hmm, it seems unnecessarily-complex to create a whole new type (exposed in the .h file) for this. Also, dbaron discourages the use of std::pair, per bug 1125750 comment 4. I'd suggest we do one of the following, instead of using this new std::pair-based type: (1) Make the LineRange constructor do the fixup itself (which doesn't change behavior, because I think you're *already* effectively doing this fixup after all the places where you use this constructor). (2) Give LineRange two constructors: - a main constructor asserts that the invariant holds (mStart < mEnd) - a 2nd "fixup" constructor does fixup to *make* the invariant hold. We could just use the 2nd constructor in places where we know fixup may be needed (in some, if not all, ResolveLineRangeHelper cases). Code can invoke the 2nd constructor using a a dummy enum arg, like the various "ConstructorType" enums we use to distinguish between the various StyleAnimationValue constructors. (3) Give LineRange() an optional "aMayNeedFixup" boolean arg, which determines whether the constructor asserts or does fixup. Option #1 is pretty equivalent (code-path-wise) to what you had in the previous patch revision, I think. Option #2 is pretty equivalent to your std::pair version, albeit with a new constructor instead of a new type. Option #3 is a simpler version of #2, but I suspect you'll lean against it since it requires an extra boolean check at runtime. (though perhaps it's better at sharing code than #2) All of these options would also remove the need for the ResolveLineRange vs ResolveLineRangeHelper distinction, I think, because that separation only exists to enforce invariants which could now be enforced in the LineRange constructor(s). > > > + * @param aStyle the StylePosition() for the grid container > > > + */ > > > + GridArea PlaceDefinite(nsIFrame* aChild, const nsStylePosition* aStyle); > > > > Maybe s/aStyle/aGridStyle/ (or aGridStylePos), to make it more superficially > > clear that this is associated with the *grid*, not with aChild. > > The @param comment is clear IMO, but I'll consider it for a later patch. > (I'd rather not introduce conflicts with all the later patches for this...) Right, sorry -- I didn't explain this very well. It is indeed clear in the documentation here. The goal of this suggestion was to make it clearer in the code itself, in the .cpp file, that "aStyle" is for the grid & not the item (particularly in item-centric functions). Anyway, I agree it's best to punt on this to avoid bitrot. > > So: maybe assert that it's nonnegative, > > I changed the code to just check aStart.mInteger == 0. > I'm not keen to assert every possible invariant Sure, that's fair -- this is better with the zero check. (My concern was over the *appearance* of correcting negative values, which is confusing given that the style-system ensures that this is nonnegative.)

Comment on attachment 8571538 [details] [diff] [review] part 1, [css-grid] Implement line-based placement. r=me on part 1, ideally with LinePair removed (and ResolveLineRange/ResolveLineRangeHelper potentially merged) via one of the options in comment 19, or something similar.

Comment on attachment 8566031 [details] [diff] [review] part 2, [css-grid] Add a method to inflate the implicit grid to include a given grid area. Review of attachment 8566031 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGridContainerFrame.h @@ +59,5 @@ > : mStart(aOther.mStart), mEnd(aOther.mEnd) {} > bool IsAuto() const { return mStart == 0; } > bool IsDefinite() const { return mStart != 0; } > uint32_t Extent() const { return mEnd - mStart; } > + uint32_t End() const { return IsAuto() ? mEnd + 1 : mEnd; } Needs a comment documenting what its return-value means, particularly in the "IsAuto()" case, where we don't have an explicit position. Though, per my next comment, we may want to do away with the IsAuto() check altogether (and replace it with an assertion), in which case the function's a bit more self-explanatory. @@ +165,5 @@ > void InitializeGridBounds(const nsStylePosition* aStyle); > > /** > + * Inflate the implicit grid to include aArea. > + * @param aArea may be definite or auto The one caller of this method doesn't actually exercise the auto-GridArea codepath. It only calls this function if area.IsDefinite(). Given that, is it actually useful to support an "auto"-position aArea here? Maybe better to just have LineRange::End() return the *actual end grid-line*, and assert that the LineRange is definite... (and document this dependency here) (I don't really understand what it'd actually mean, to inflate the grid to "include" an auto-positioned area. I guess it's assuming that the area is positioned at the upper-left corner? Given that we never actually invoke this function to do that, I don't know why that would be a useful thing to do.)

Comment on attachment 8566034 [details] [diff] [review] part 3, [css-grid] Store the grid item's grid area on a frame property. >+++ b/layout/generic/nsGridContainerFrame.h >@@ -48,36 +48,38 @@ protected: > struct LineRange { >- LineRange(uint32_t aStart, uint32_t aEnd) >- : mStart(aStart), mEnd(aEnd) {} >+ LineRange() : mStart(0), mEnd(0) {} // intentionally bogus to assert if used >+ LineRange(uint32_t aStart, uint32_t aEnd) >+ : mStart(aStart), mEnd(aEnd) { MOZ_ASSERT(mEnd != 0); } [...] > struct GridArea { >+ GridArea() {} Hmm, so we're relaxing restrictions on LineRange/GridArea here -- allowing ourselves to default-construct these objects, with known-bogus values, and then hoping we'll catch any accidental usages of these with assertions. I'd prefer that we keep the compile-time restrictions in place. (and don't introduce default constructors here). See below. > nsGridContainerFrame::PlaceGridItems(const nsStylePosition* aStyle) > { [...] > for (nsFrameList::Enumerator e(PrincipalChildList()); !e.AtEnd(); e.Next()) { > nsIFrame* child = e.get(); >- const GridArea& area = PlaceDefinite(child, aStyle); >- if (area.IsDefinite()) { >- InflateGridFor(area); >+ GridArea* area = GetGridAreaForChild(child); >+ if (!area) { >+ area = new GridArea(); >+ child->Properties().Set(GridAreaProperty(), area); >+ } >+ PlaceDefinite(child, aStyle, area); It looks like the restriction-relaxing is just so we can do "new GridArea()" here, and then populate it after that. I'd rather we do this without needing a default constructor. Right now the only way I can think to do this will have a small cost -- we'll have to copy 2 LineRanges from a local variable into our heap-allocated structure -- but that cost should be negligible since LineRanges are pretty small (each one fits in a single word, on a 64-bit system), and the local-variable will be in the CPU's cache anyway, so the copying should be plenty fast. So, I don't think it's worth removing compile-time sanity checks to avoid this cost, particularly if we make the existing LineRange constructor(s) more comprehensive about enforcing the mStart < mEnd invariant, as suggested in comment 19. One way to rewrite this ^ code to not require a default constructor: - PlaceDefinite() could manage the frame-property checking/updating internally. - It can store its ResolveLineRange return-values in local LineRange variables. - If we need to create a new GridArea, we can instantiate one, passing in those variables. Otherwise, we can copy them into the existing GridArea object. Or alternately, we can leave PlaceDefinite untouched (i.e. don't modify it in this patch), and just invoke it like so: GridArea* area = GetGridAreaForChild(child); if (area) { area = PlaceDefinite(child, aStyle); } else { child->Properties().Set(GridAreaProperty(), new GridArea(PlaceDefinite(child, aStyle)));

(I'm also not clear on why we're storing the child's grid area in a frame property at this point; we never use the stored value in any of the patches here. I assume we'll do so in a later patch somewhere else?)

Comment on attachment 8566036 [details] [diff] [review] part 4, [css-grid] Add a cell map to keep track of which grid cells are occupied. >+void >+nsGridContainerFrame::CellMap::Fill(const GridArea& aGridArea) >+{ >+ const auto rowEnd = aGridArea.mRows.mEnd - 1; >+ const auto colEnd = aGridArea.mCols.mEnd - 1; >+ mCells.EnsureLengthAtLeast(rowEnd); >+ for (auto row = aGridArea.mRows.mStart - 1; row < rowEnd; ++row) { >+ nsTArray<Cell>& columns = mCells[row]; >+ columns.EnsureLengthAtLeast(colEnd); >+ for (auto col = aGridArea.mCols.mStart - 1; col < colEnd; ++col) { Document the reason for the "- 1" usages here, so they're not so magic. (I think for mStart it's because the lines are 1-based, and for mEnd it's because there's 1 more line than there are tracks) Also, maybe s/rowEnd/numRows/ and s/colEnd/numCols/, to make it unambiguous what those numbers represent & that they're 0-based. ("rowEnd" sounds like it could be the grid-line-number for the end of the rows, and it's unclear from the name whether it's 1-based (like a grid-line) or 0-based (like a track index)). Also: "columns" is a confusingly-named variable. It's actually a "row of cells". Each entry is a cell -- not a "column". Maybe rename it to "cellsInRow"? >+void >+nsGridContainerFrame::CellMap::ClearOccupied() >+{ >+ const size_t end = mCells.Length(); >+ for (size_t i = 0; i < end; ++i) { >+ nsTArray<Cell>& columns = mCells[i]; >+ const size_t end = columns.Length(); >+ for (size_t i = 0; i < end; ++i) { >+ columns[i].mBits = 0; Two things: (1) I'm uncomfortable shadowing "i" and "end" here. (and I think we even have that treated as a compiler-warning in some layout subdirectories) Let's use "j", and "numRows" / "numCols". (2) "columns" vs. "mCells" here is a bit confusing, since mCells is an array of *rows*, and "columns" is an array of *Cells*. Seems clearer to just treat mCells as a 2D matrix, like: mCells[i][j].mBits = 0; Or alternately, maybe rename "columns" to "cellsInRow" (as suggested for a different loop above) >+void >+nsGridContainerFrame::CellMap::Dump() const >+{ >+ const size_t end = mCells.Length(); >+ for (size_t i = 0; i < end; ++i) { >+ const nsTArray<Cell>& columns = mCells[i]; >+ const size_t end = columns.Length(); >+ printf("%lu:\t", i + 1); >+ for (size_t i = 0; i < end; ++i) { >+ printf(columns[i].IsOccupied() ? "X " : ". "); >+ } Both points in previous comment applies here, too. Also: your first printf here probably triggers a build warning on some platforms -- http://stackoverflow.com/questions/2524611/how-to-print-size-t-variable-portably backs this up. (IIRC "%u" warns with size_t on one arch, and "%lu" warns on the other). I think you need to static_cast "i + 1" to unsigned long, to make this portable. (or you can use %z, but I don't think MSVC supports that) >+++ b/layout/generic/nsGridContainerFrame.h > /** >+ * A CellMap holds state for each cell in the grid. >+ * It's row major and sparse in the column dimension. What do you mean here by "sparse in the column dimension"? I take this to mean "space-efficient" (like we don't allocate space for unused cells), but it doesn't actually seem to be that way, to me. At least, CellMap::Fill calls "columns.EnsureLengthAtLeast(colEnd)", for every single row. (note that the variable "columns" here is really a row of cells.) >+ struct Cell { >+ Cell() : mBits(0) {} >+ bool IsOccupied() const { return mBits & 1; } >+ uint8_t mBits; >+ }; mBits should perhaps be replaced with "bool mIsOccupied:1;" That way we don't have to bother with bitwise arithmetic, or having to manage 7 unused bits that we may end up subtly accidentally misusing somehow. (and even if we use them correctly, it's easier to audit usage of a bool than usage of a bitfield.) >+ nsTArray<nsTArray<Cell>> mCells; >+ void Fill(const GridArea& aGridArea); >+ void ClearOccupied(); The member-var ("mCells") should probably be declared after all the methods, for consistency (and so if we add another member-var, we don't end up with methods in between them).

Comment on attachment 8566037 [details] [diff] [review] part 5, [css-grid] Fill the cell map if the grid area is definite. >+++ b/layout/generic/nsGridContainerFrame.cpp >@@ -434,29 +434,31 @@ nsGridContainerFrame::InitializeGridBoun > void > nsGridContainerFrame::PlaceGridItems(const nsStylePosition* aStyle) > { >+ mCellMap.ClearOccupied(); > InitializeGridBounds(aStyle); If the author has changed style to reduce the number of rows or columns, since the last time we reflowed, is that going to create a problem here? mCellMap would end up having too many cells (though they'll be marked as unoccupied at least). I don't know yet if that'll be a problem, since I'm not sure how mCellMap will be used & how much we're assuming about its size being equal to the grid size.

(In reply to Daniel Holbert [:dholbert] from comment #19) > Hmm, it seems unnecessarily-complex to create a whole new type (exposed in > the .h file) for this. Also, dbaron discourages the use of std::pair, per > bug 1125750 comment 4. Well, dbaron qualified that with "in general". I don't see anything in our Code Style Guide to discourage its use. I typedef'd it to give a hint of what values it carries. I don't think there are any meaningful member names in this case that's any better than first/second. BTW, Rust made tuples a first-class construct in the language because *convenience* also matters, no? > I'd suggest we do one of the following, instead of using this new > std::pair-based type: Thanks, but I think your alternatives are more complicated than a simple typedef. It's not a big deal to use once for an internal protected helper method, IMO. Regarding LineRange, there are at least two major changes coming up affecting it (due to spec changes). First, the "implicit grid grows to the left/top of the explicit grid" issue, which probably means we'll have to abandon using zero for "auto" and use the full (signed) integer range for line numbers. Second, the "(repeat auto)" thing. And there's also the abs.pos. item support which will re-use LineRange. So it's a bit premature to try to nail down the exact representation / invariants for LineRange at this point. We can revisit this later when LineRange has settled if you wish.

(In reply to Daniel Holbert [:dholbert] from comment #21) > > + uint32_t End() const { return IsAuto() ? mEnd + 1 : mEnd; } > > Needs a comment documenting what its return-value means, particularly in the > "IsAuto()" case, where we don't have an explicit position. Yeah, sorry about that, this would have been clearer with my auto-placement patch (bug 1107778) which is coming up next. I'll post that ASAP, and also a WIP patch for abs.pos. item support to give you more insight into how these things are being used. End() is used by InflateGridFor() which also works for not yet placed auto items. This is for Step 2 in: http://dev.w3.org/csswg/css-grid/#auto-placement-algo "The largest column span of all items." I renamed it HypotheticalEnd() and documented what it's for. Let me know if you can think of a better name. > The one caller of this method doesn't actually exercise the > auto-GridArea codepath. There will be more callers in bug 1107778, also for the auto path.

(In reply to Daniel Holbert [:dholbert] from comment #22) > Hmm, so we're relaxing restrictions on LineRange/GridArea here Fair enough, I reverted those changes for now.

(In reply to Daniel Holbert [:dholbert] from comment #24) > What do you mean here by "sparse in the column dimension"? I take this to > mean "space-efficient" I've amended the code comment to explain this in more detail. Fixed the other nits, thanks.

(In reply to Daniel Holbert [:dholbert] from comment #25) > >+ mCellMap.ClearOccupied(); > > If the author has changed style to reduce the number of rows or columns, > since the last time we reflowed, is that going to create a problem here? > mCellMap would end up having too many cells (though they'll be marked as > unoccupied at least). It's not a problem since they are marked as unoccupied. We could be smarter about managing memory here in the future, but it's not a priority at the moment. I would very much like to preserve placement data when possible for example, since it's a bit expensive to do for each reflow.

(In reply to Mats Palmgren (:mats) from comment #26) > > I'd suggest we do one of the following, instead of using this new > > std::pair-based type: > > Thanks, but I think your alternatives are more complicated than > a simple typedef. I disagree -- my suggestions were aimed at removing a layer of "Helper()" logic and removing the mental burden of one helper-type ("LinePair") by making another type ("LineRange") slightly smarter & making it enforce invariants a bit more effectively. So I think it'd be a net complexity-win -- but I'm also OK keeping LinePair as-is, as long as it doesn't leak too far outside of its current usage in that protected method (so that it can mostly just be ignored & doesn't add much of a mental burden). Particularly given your point here: > Regarding LineRange, there are at least two major changes coming up [...] > So it's a bit premature to try to nail down the exact representation / > invariants for LineRange at this point. > > We can revisit this later when LineRange has settled if you wish. That's fair. r=me on part 1, then.

Comment on attachment 8572716 [details] [diff] [review] part 2, [css-grid] Add a method to inflate the implicit grid to include a given grid area. r=me on part 2

Comment on attachment 8572717 [details] [diff] [review] part 3, [css-grid] Store the grid item's grid area on a frame property. r=me on part 3

(In reply to Mats Palmgren (:mats) from comment #29) > (In reply to Daniel Holbert [:dholbert] from comment #24) > > What do you mean here by "sparse in the column dimension"? I take this to > > mean "space-efficient" > > I've amended the code comment to explain this in more detail. Thanks! (Turns out my non-sparseness concerns were just from me misunderstanding how it gets populated. I was imagining CellMap::Fill() running just once, and making a rectangular cell-map -- but really it gets called for wildly-different & potentially-small GridAreas. And a GridArea positioned at e.g. the bottom-right corner will expand the bottom rows rightwards without expanding the higher-up rows. Hence, sparse. Cool.)

Comment on attachment 8572720 [details] [diff] [review] part 4, [css-grid] Add a cell map to keep track of which grid cells are occupied. r=me on part 4

Comment on attachment 8572721 [details] [diff] [review] part 5, [css-grid] Fill the cell map if the grid area is definite. r=me on part 5

Tests for the code in this bug. I'll land it together with bug 1139539 since it requires that code to pass.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc169f08032f https://hg.mozilla.org/integration/mozilla-inbound/rev/2d56a7bfeda6 https://hg.mozilla.org/integration/mozilla-inbound/rev/366808d42d0f https://hg.mozilla.org/integration/mozilla-inbound/rev/4c763292430d https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ea81e3381a https://hg.mozilla.org/integration/mozilla-inbound/rev/162620cecffc https://hg.mozilla.org/integration/mozilla-inbound/rev/a56c919af21d

https://hg.mozilla.org/mozilla-central/rev/bc169f08032f https://hg.mozilla.org/mozilla-central/rev/2d56a7bfeda6 https://hg.mozilla.org/mozilla-central/rev/366808d42d0f https://hg.mozilla.org/mozilla-central/rev/4c763292430d https://hg.mozilla.org/mozilla-central/rev/c9ea81e3381a https://hg.mozilla.org/mozilla-central/rev/162620cecffc https://hg.mozilla.org/mozilla-central/rev/a56c919af21d