The shorthands are: * grid-column * grid-row * grid-area * grid-template * grid

Have O(N) instead of O(N^4) test cases for grid-area, where N is the number of cases for grid-column-start. The test suite took an unreasonable amount of time to run, and we don’t really need to test all these combinations.

Comment on attachment 8388820 [details] [diff] [review] Patch 1 v2: Add the grid-column, grid-row and grid-area shorthands. >+++ b/layout/style/nsCSSParser.cpp >+bool >+CSSParserImpl::ParseGridArea() >+{ >+ nsCSSValue values[4]; >+ if (ParseVariant(values[0], VARIANT_INHERIT, nullptr)) { >+ AppendValue(eCSSProperty_grid_row_start, values[0]); >+ AppendValue(eCSSProperty_grid_column_start, values[0]); >+ AppendValue(eCSSProperty_grid_row_end, values[0]); >+ AppendValue(eCSSProperty_grid_column_end, values[0]); >+ return true; >+ } >+ >+ int32_t i = 0; >+ for (;;) { >+ if (!ParseGridLine(values[i])) { >+ return false; >+ } >+ if (++i == 4 || !GetToken(true)) { >+ break; >+ } >+ if (!mToken.IsSymbol('/')) { >+ UngetToken(); >+ break; >+ } >+ } >+ Add after the loop: MOZ_ASSERT(i > 0, "should have parsed at least one grid-line (or returned)"); to make it clear that we know at least values[0] is non-bogus. >+ if (i < 4) { >+ HandleGridLineFallback(values[1], values[3]); >+ } >+ if (i < 3) { >+ HandleGridLineFallback(values[0], values[2]); >+ } >+ if (i < 2) { >+ HandleGridLineFallback(values[0], values[1]); >+ } So, two problems here (which are kind of two sides of the same coin) (A) It seems a bit wrong to pass "values[1]" into that first HandleGridLineFallback() call, when we aren't actually sure whether we've put anything useful in values[1]. (Technically it will have been initialized, to eCSSUnit_Null, but it'd be nice to not have to depend on that, since we do have "i" available to us to tell us whether there's anything there.) (B) Right now, if only a single value is provided (and is a <custom-ident>), that keyword is *supposed* to get copied into all four properties. But right now, I don't see how it'd make it into the fourth property. (values[3]) The only thing that would modify values[3] is the "HandleGridLineFallback(values[1], values[3])" line, which (per above) will see a eCSSUnit_Null value in values[1] and will set values[3] to Auto. We can address both concerns by reversing the order of these checks, I think, which would be effectively doing the following (in this order): - if we don't have anything in values[1], invoke fallback code w/ values[0]. - if we don't have anything in values[2], invoke fallback code w/ values[0]. - if we don't have anything in values[3], invoke fallback code w/ values[1]. (note that "if we don't have anything in values[n]" is English for "if (i < [n+1])" Check my logic, but I *think* that correctly populates all the values, and ensures we never read from an uninitialized (eCSSUnit_Null) value.

(The useful thing about reversing the logic is: even if values[1] is uninitialized (since i == 1), by the time we get to maybe-copying it into values[3] with HandleGridLineFallback, it now will have been initialized to a value that will do the right thing. (It'll either have the contents of values[0], so we'll copy that forward into values[3]; or alternately, it'll be eCSSUnit_Auto, which will be ignored and we'll set values[3] to auto.)

Also: you should add a mochitest (or maybe two - one for grid-column/grid-row, one for grid-area?) to test that these shorthands actually set their sub-properties in the way that we expect. The standard property_database.js tests don't test that; they only test whether values are accepted or rejected. (They can't test anything smarter for shorthands, because they don't know how specific shorthand values are supposed to map onto their sub-properties.) Here's a sample mochitest that I wrote, to test a variety of "flex" shorthand-values: http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_flexbox_flex_shorthand.html?force=1

Comment on attachment 8388820 [details] [diff] [review] Patch 1 v2: Add the grid-column, grid-row and grid-area shorthands. Notes on property_database.js: >diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js >index 6ea55cc..e328645 100644 >--- a/layout/style/test/property_database.js >+++ b/layout/style/test/property_database.js >+ // <grid-line> [ / <grid-line> ]? is equivalent to: >+ // <grid-line> | [ <grid-line> / <grid-line> ] >+ // <'grid-column-start'> | <'grid-auto-position'> The comment there kind of comes out of the blue. Maybe replace the first line with: // The properties "grid-column" and "grid-row" take values of the form // <grid-line> [ / <grid-line> ]? // which is equivalent to: // <grid-line> | [ <grid-line> / <grid-line> ] // which is equivalent to: // <grid-line> | <'grid-auto-position'> (Note also that I dropped <'grid-column-start'> -- I think it just confuses things here, because you don't end up actually any "grid-column-start"-specific variables. >+ var gridColumnRowInvalidValues = [].concat( >+ gridLineInvalidValues, >+ gridAutoPositionInvalidValues); >+ // A single <grid-line> is invalid for grid-auto-position, >+ // but not for grid-column or grid-row: >+ gridColumnRowInvalidValues.splice( >+ gridColumnRowInvalidValues.indexOf("foo"), >+ 1); It sounds like you're saying (a) gridAutoPositionInvalidValues contains the value "foo" (b) and we need to remove that here I agree with (b), but I'm not sure (a) is currently true. Looking at... http://hg.mozilla.org/integration/mozilla-inbound/annotate/d935fe80b795/layout/style/test/property_database.js#l5022 ...it seems like gridAutoPositionInvalidValues only contains "foo, bar", "foo / bar / baz", and some longer variations on those. It probably should contain "foo", though. If I'm not missing something, maybe add that in as part of this patch? >+ var gridAreaInvalidValues = [ >+ "foo, bar", >+ "foo / bar / baz / fizz / buzz", >+ ].concat(gridLineInvalidValues); Throw in these, to make sure we reject these in our custom-ident parsing, mid-value: "foo / bar / inherit / baz", "foo / initial / bar / baz" "foo / bar / baz / unset" "foo / default / bar / baz"

Comment on attachment 8389394 [details] [diff] [review] Patch 1 v3: Add the grid-column, grid-row and grid-area shorthands. >+++ b/layout/style/test/test_grid_item_shorthands.html >+<script> >+ >+[ >+ { >+ specified: "3 / 4", >+ expected_start: "3", >+ expected_end: "4", >+ }, Per IRC conversation: please add a comment before this array, to indicate what its purpose is. >+ // "When the second value is omitted, if the first value is a <custom-ident>, >+ // the grid-row-end/grid-column-end longhand is also set to that <custom-ident>; >+ // otherwise, it is set to auto." Those first two lines are just a few characters over 80. Can you rewrap this so it doesn't go over? Also: Add a URL [1] for the section of the spec that you're quoting, so it's clearer what you're quoting here. [1] I think you want http://dev.w3.org/csswg/css-grid/#placement-shorthands >+ { >+ specified: "span foo", >+ expected_start: "span foo", >+ expected_end: "auto", >+ }, >+ { >+ specified: "foo 7 span", >+ expected_start: "span 7 foo", >+ expected_end: "auto", >+ }, >+ { >+ specified: "7 span", >+ expected_start: "span 7", >+ expected_end: "auto", >+ }, >+].forEach(function(test_case) { >+ ["Column", "Row"].forEach(function(axis) { >+ var shorthand = "grid" + axis; >+ var start_longhand = "grid" + axis + "Start"; >+ var end_longhand = "grid" + axis + "End"; Nit: IMHO it'd be a bit clearer & cleaner if you declared thes (large) testcase-arrays as variables, and then invoke forEach() on the variables. This would more clearly separate data from logic, which is generally a Good Thing. It also makes the file easier to parse visually; "OK, here's the list of all the situations to test. [skip down past next comment or blank line] OK, here's the logic." The way it is now, it's a bit harder to find and immediately-parse what's going on at the dividing line between data & logic. I don't feel strongly enough to gate my r+ on it, though, if you strongly prefer things the way you've got them. :) r=me

Patch 1 now seems ready for landing, but IMO it’s unlikely enough to bitrot. I’m planning to add the remaining shorthands (grid-template and grid) to this bug.

Remove some non-ASCII quotes in a comment. (These come from the spec!)

Tests (in property_database.js and test_grid_container_shorthands.html) will come in the next version of this patch.

As requested, splitting up Patch 2 v2.

Comment on attachment 8391324 [details] [diff] [review] Patch 3: Add the grid shorthand. >+++ b/layout/style/Declaration.cpp >+ case eCSSProperty_grid: { An explanatory comment at the top here would be helpful. (indicating that the shorthand syntax can represent either the "grid-template-*" properties, or the grid-auto-* properties, but not both.) >+ const nsCSSValue& areasValue = >+ *data->ValueFor(eCSSProperty_grid_template_areas); >+ const nsCSSValue& columnsValue = >+ *data->ValueFor(eCSSProperty_grid_template_columns); >+ const nsCSSValue& rowsValue = >+ *data->ValueFor(eCSSProperty_grid_template_rows); >+ >+ const nsCSSValue& autoFlowValue = >+ *data->ValueFor(eCSSProperty_grid_auto_flow); >+ const nsCSSValue& autoColumnsValue = >+ *data->ValueFor(eCSSProperty_grid_auto_columns); >+ const nsCSSValue& autoRowsValue = >+ *data->ValueFor(eCSSProperty_grid_auto_rows); >+ if (areasValue.GetUnit() == eCSSUnit_None && >+ columnsValue.GetUnit() == eCSSUnit_None && >+ rowsValue.GetUnit() == eCSSUnit_None) { nit: a blank just before the "if" would be helpful for readability here. (so, each chunk of properties get declared in their own block) >+ if (areasValue.GetUnit() == eCSSUnit_None && >+ columnsValue.GetUnit() == eCSSUnit_None && >+ rowsValue.GetUnit() == eCSSUnit_None) { >+ AppendValueToString(eCSSProperty_grid_auto_flow, >+ aValue, aSerialization); >+ AppendValueToString(eCSSProperty_grid_auto_columns, >+ aValue, aSerialization); >+ aValue.AppendLiteral(" / "); >+ AppendValueToString(eCSSProperty_grid_auto_rows, >+ aValue, aSerialization); >+ } else if (!(autoFlowValue.GetUnit() == eCSSUnit_None && I'm pretty sure we need a "break" right before the "else if" there. (Otherwise we'll fall through to the "grid_template" serialization code, which we don't want or need to do if we made it into this clause.) >+ // Not serializable, bail. >+ break; Add a brief note about why this isn't serializable. (Alternately, maybe this will be made clearer with a high-level comment higher up, per my first note above.) Also: this wants to be "return", not "break". (It looks like this function uses 'return' instead of 'break' for non-serializable situations in other properties -- e.g. for eCSSProperty_text_decoration, eCSSProperty_font, eCSSProperty_background) (Of course, there's no functional difference between "break" and "return" right now, since there's no code after the end of the switch statement. But it's conceivable that we might add code after the switch statement at some point that we'd only want to run if we've successfully serialized.) >+ } >+ // Fall through to eCSSProperty_grid_template >+ } > case eCSSProperty_grid_template: { > const nsCSSValue& areasValue = > *data->ValueFor(eCSSProperty_grid_template_areas); [optional:] It feels a little redundant that we have to re-lookup the three grid-template-* properties' values, after we've fallen through. Maybe worth refactoring the grid_template serialization code into a static helper-function "AppendGridTemplateToString()" which accepts the three nsCSSValues, and then invoking that function (passing in the nsCSSValues) from both cases here, instead of falling through? This is fine, too, though; it looks like ValueFor() is cheap, so this isn't really a perf concern; just an unnecessarily-repeated-steps concern. >+++ b/layout/style/nsCSSParser.cpp >+bool >+CSSParserImpl::ParseGrid() >+{ This function could use a few short comments explaining what it's trying to do in its various chunks (after the first "inherit" chunk, which is self-explanatory). >+bool >+CSSParserImpl::ParseGridShorthandAuto() >+{ I initially assumed that this function had something to do with parsing "grid: auto" or somesuch. (But it looks like it's actually for parsing the grid-auto-* properties as part of the shorthand) Maybe rename to "ParseGridShorthandAutoProperties"? (or AutoSubroperties?)

In patch 3, CSSParserImpl::ParseGrid() > if (ParseVariant(value, VARIANT_NONE, nullptr)) { > if (!ExpectSymbol('/', true)) { > UngetToken(); > return ParseGridShorthandAuto(); > } I have a bug here. I had hoped to unget the 'none' token, but this actually ungets the slash. I’ll refactor ParseGridShorthandAuto() to not include the call to ParseGridAutoFlowValue(). (This is what I get for submitting patches before writing the tests.)

Comment on attachment 8391798 [details] [diff] [review] Patch 2c: Add the grid-template shorthand. Just stylistic nits on Patch 2c for now; I'll review the logic later, when I'm less jetlag-brained :) >@@ -957,16 +958,91 @@ Declaration::GetValue(nsCSSProperty aProperty, nsAString& aValue, >+ if (areasValue.GetUnit() != eCSSUnit_GridTemplateAreas || >+ rowsValue.GetUnit() != eCSSUnit_List) { >+ // Not serializable, bail. >+ break; [...] >+ if ((nRowItems - 1) / 2 != areas.NRows()) { >+ // Not serializable, bail. >+ break; As noted for patch 3, these should probably be "return". >+ for (;;) { >+ bool addSpaceSpearator = true; nit: s/Spearator/Separator/ >+bool >+CSSParserImpl::ParseGridTemplate() >+{ [...] >+ // TODO: add parsing for 'subgrid' by itself Add "(bug 983175)" here, and in the other TODO's for subgrid in this patch. >+ if (mToken.mType == eCSSToken_String) { >+ // [ <track-list> / ]? was ommitted nit: only one "m" in "omitted" >+// Parse [ <line-names>? <string> <track-size>? <line-names>? ]+ >+// with a <line-names>? already consumed, stored in |aFirstLineNames|, >+// and the current token a <string> >+bool >+CSSParserImpl::ParseGridTemplateAfterString(const nsCSSValue& aColumnsValue, >+ const nsCSSValue& aFirstLineNames) >+{ >+ >+ MOZ_ASSERT(mToken.mType == eCSSToken_String, >+ "ParseGridTemplateAfterString called with a non-string token"); >+ Drop the blank first line before MOZ_ASSERT.

(In reply to Daniel Holbert [:dholbert] from comment #20) > Comment on attachment 8391798 [details] [diff] [review] > Patch 2c: Add the grid-template shorthand. [...] > As noted for patch 3, these should probably be "return". (Ah, I see you fixed these in your new Patch 3. Ideally it'd be nice to have them correct in the first place, in the patch that adds them (patch 2c), but not a huge deal.)

Remove changes that are now in Patch 2c.

Comment on attachment 8391796 [details] [diff] [review] Patch 2a: Refactor grid-template-areas, store number of columns Moved to bug 984241.

Rebase on top of bug 984241

Comment on attachment 8391797 [details] [diff] [review] Patch 2b: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. >From: Simon Sapin <simon.sapin@exyr.org> >Bug 981752 part 2b: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. > >... and a bit of grid-auto-flow, for the grid shorthand. > >* Avoid using CheckEndProperty() when possible >* Distinuguish between "error" and "not a <track-size>" nit in this extra commit message info: you've got an extra "u" in "Distinuguish" there (before the "g") >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp >+enum nsParsingStatus { >+ // we have parsed a something: >+ eParsingStatus_Ok, s/a something/something/ (perhaps even "something successfully"?) >+ // we did not find what we were looking for, and called UngetToken: >+ eParsingStatus_NotFound, The "and called UngetToken" isn't necessarily true, e.g. in your "if (!GetToken(true)) {" clause later in this patch. (There's no UngetToken there, because there's nothing to unget) So: this probably needs some minor clarification. (maybe "either because we failed to get a token, or we got a token we did not recognize and called UngetToken"?) >+ // we saw an unexpected token or token value: >+ eParsingStatus_Error This is a bit vague as well - it's not clear in what situations one would use _NotFound (and unget the unrecognized thing) vs. _Error and (not unget the unrecognized thing). Is the idea that you use _Error when it's *too late* to Unget far back enough? (e.g. somewhere far into a parenthesized list) Might be worth making that distinction clearer here. >@@ -6887,35 +6898,36 @@ CSSParserImpl::ParseGridAutoFlow() >- do { >+ for (;;) { > if (!GetToken(true)) { >- return false; >+ break; > } > if (mToken.mType != eCSSToken_Ident) { > UngetToken(); >- return false; >+ break; > } > nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent); > if (keyword == eCSSKeyword_dense && !gotDense) { > gotDense = true; > } else if (keyword == eCSSKeyword_column && !gotColumn && !gotRow) { > gotColumn = true; > } else if (keyword == eCSSKeyword_row && !gotColumn && !gotRow) { > gotRow = true; > } else { >- return false; >+ UngetToken(); >+ break; > } >- } while (!CheckEndProperty()); >+ } I'm realizing this should probably be ParseChoice(). I spun off bug 984728 for that, though feel free to address that here if you like. (That will make this whole "do {... }" loop unnecessary.)

Comment on attachment 8391797 [details] [diff] [review] Patch 2b: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. > // If successful, leaves aValue with eCSSUnit_Null for the empty list, > // or sets it to a eCSSUnit_List of eCSSUnit_Ident. [...] >+// If aValue is already a eCSSUnit_List, append to that list. > bool > CSSParserImpl::ParseGridLineNames(nsCSSValue& aValue) > { >- MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Null, >- "Unexpected unit, aValue should not be initialized yet"); The "Leaves aValue with eCSSUnit_Null" language is now misleading, since aValue may no longer be null (per your other comment and assert-removal) Maybe "Leaves aValue untouched"? >+ if (aValue.GetUnit() == eCSSUnit_List) { >+ // Find the end of an existing list >+ item = aValue.GetListValue(); >+ while (item->mNext) { >+ item = item->mNext; >+ } So here, we walk the whole list that we've parsed in previous call(s) to this function, each time we call this function. This has O(n^2) performance, which would be nice to avoid. For cases where we need to walk to the end of the list, can we just cache the end of the list somewhere? (maybe as an optional outparam, only used by callers who know they might be appending on to the list later?)

Sorry, disregard the ParseChoice thing for ParseGridAutoFlow -- that was probably a red herring. (I'll post more on bug 984728.)

Comment on attachment 8391797 [details] [diff] [review] Patch 2b: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. Also, for the O(n^2) thing at the end of Comment 27 -- as noted in IRC, I suppose it's really only O(2n) at worst, since from looking at "grid-template" property's syntax [the place where this is exercised], we'll only get *at most* two lists that we have to concatenate. So, it's not quite as much of a potential-perf-problem as I'd thought, but it's still probably worth tracking the last thing in the value-list (as an optional outparam or something), to save a bit of wasted work from having to walk the whole list on the second call. (If it severely complicates the code, then maybe not worth it, but I don't think it should complicate things too much.) >+ if (aValue.GetUnit() == eCSSUnit_List) { [...] >+ } else { >+ MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Null, >+ "Unexpected unit, aValue should not be initialized yet"); >+ item = aValue.SetListValue(); The "should not be initialized yet" message isn't right for this assertion anymore. (In the current codebase, with this assertion at the beginning of this function, the message is correct, but now we're allowing aValue to be initialized (as a list).) So, please update the message -- even just "Unexpected unit" would probably be fine in the new context. (that'd make the assertion fit as a one-liner, too).

Comment on attachment 8391797 [details] [diff] [review] Patch 2b: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. >+// On success, |aValue| will be a list of odd length >= 3, >+// starting with a <line-names> (which is itself a list) >+// and alternating between that and <track-size> > bool >-CSSParserImpl::ParseGridTrackList(nsCSSProperty aPropID) >+CSSParserImpl::ParseGridTrackList(nsCSSValue& aValue, >+ const nsCSSValue& aFirstLineNames) Since this no longer exactly parses a <track-list> (but instead only parses the second 2/3 of a <track-list>), this needs a clearer name, per IRL conversation. (Also could benefit from a comment explaining how this relates to <track-size> as defined in the spec, and why we split up the functionality like we do instead of having a "pure" ParseGridTrackList) >+ item->mNext = new nsCSSValueList; >+ item = item->mNext; >+ // FIXME: add repeat() >+ nsParsingStatus result = ParseGridTrackSize(item->mValue); >+ if (result != eParsingStatus_Ok) { >+ // Need at least one <track-size> > return false; [...] >+ result = ParseGridTrackSize(trackSize); >+ if (result == eParsingStatus_Error) { > return false; > } >+ if (result == eParsingStatus_NotFound) { >+ break; >+ } The distinction in return-value-handling between these two ParseGridTrackSize() calls is subtle. Could you add a comment (maybe next to the 'break' statement) explaining the difference?

(In reply to Daniel Holbert [:dholbert] from comment #30) > The distinction in return-value-handling between these two > ParseGridTrackSize() calls is subtle. > > Could you add a comment (maybe next to the 'break' statement) explaining the > difference? (e.g. maybe something like "We didn't find a <track-size>, but that's OK; we can just be done, because what we've parsed up to this point is valid as a <track-list> on its own".)

Simplify the code a bit, based on the newly-learned fact that it’s ok to call AppendValue() before returning false.

Add tests, fix bugs uncovered by tests, and simplify the code a bit, based on the newly-learned fact that it’s ok to call AppendValue() before returning false.

Comment on attachment 8394026 [details] [diff] [review] Patch 2c v4: Add the grid-template shorthand. >diff --git a/layout/style/Declaration.cpp b/layout/style/Declaration.cpp >+ if (areasValue.GetUnit() == eCSSUnit_None) { [...] >+ break; >+ } >+ if (areasValue.GetUnit() != eCSSUnit_GridTemplateAreas || >+ rowsValue.GetUnit() != eCSSUnit_List) { >+ // Not serializable, bail. >+ return; >+ } I don't think we need the areasValue check, do we? It only can be None or GridTemplatesArea, IIRC. (Or inherit/unset/initial, but we don't need to worry about those values on subproperties here -- the CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES code at the top of this function handles various possibilities with those keywords, for all shorthand properties.) So: we should replace that areasValue check here with an assertion (like the ones we have in eCSSProperty_transition). As for the rowsValue check: it looks like we *do* need that one, but maybe explain the "not serializable" comment a bit? e.g. if I'm understanding correctly, I think you're trying to say: // We have "grid-template-areas:[something]; grid-template-rows:none" // which isn't a value that the shorthand can express. Bail. (Handy to be extra-clear about this, since this is true of rows but *not* true of columns.) Also, we should allow for rowsValue to be ListDep here. >+++ b/layout/style/nsCSSParser.cpp >+ nsCSSValue firstLineNames; >+ if (!(ParseGridLineNames(firstLineNames) && >+ GetToken(true))) { >+ return false; >+ } >+ if (mToken.mType == eCSSToken_String) { >+ // [ <track-list> / ]? was omitted >+ value.SetNoneValue(); >+ AppendValue(eCSSProperty_grid_template_columns, value); >+ return ParseGridTemplateAfterString(firstLineNames); >+ } >+ UngetToken(); >+ >+ if (!(ParseGridTrackListWithFirstLineNames(value, firstLineNames) && >+ ExpectSymbol('/', true))) { >+ return false; >+ } >+ AppendValue(eCSSProperty_grid_template_columns, value); >+ return ParseGridTemplateAfterSlash(/* aColumnsIsTrackList = */ true); >+} This part is a little complex, since you're trying to parse several possible grammar-options simultaneously. Could you add a comment or two before this chunk, explaining the high-level strategy here? >+bool >+CSSParserImpl::ParseGridTemplateAfterSlash(bool aColumnsIsTrackList) >+{ >+ nsCSSValue rowsValue; >+ if (!ParseVariant(rowsValue, VARIANT_NONE, nullptr)) { So the none-handling code doesn't happen until much further down. Can we flip the logic so that the none-handling code happens next to the attempted VARIANT_NONE parsing? >+ // <'grid-template-columns'> / <'grid-template-rows'> >+ nsCSSValue areasValue; >+ areasValue.SetNoneValue(); // implied Condense these 2 lines to: nsCSSValue areasValue(eCSSUnit_None); // implied Also: the comment here is a bit confusing. This is really only handling that grammar **with the rows component being 'none'**, right? (Other code handles non-'none' rows, presumably)? Can you clarify that? >+// Parse [ <line-names>? <string> <track-size>? <line-names>? ]+ >+// with a <line-names>? already consumed, stored in |aFirstLineNames|, >+// and the current token a <string> >+bool >+CSSParserImpl::ParseGridTemplateAfterString(const nsCSSValue& aFirstLineNames) >+{ Maybe add something like: // Helper for parsing "grid-template" shorthand: before this comment, as a hint for what the significance is for that particular stretch of grammar. r=me with the above addressed/responded-to.

Comment on attachment 8394024 [details] [diff] [review] Patch 2b v2: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. >+++ b/layout/style/nsCSSParser.cpp >+enum nsParsingStatus { Let's define this enum using MOZ_BEGIN_ENUM_CLASS, to make it easier to catch bugs from implicit casts to boolean values (or other types). >+ // On success, |aValue| will be a list of odd length >= 3, >+ // starting with a <line-names> (which is itself a list) >+ // and alternating between that and <track-size> >+ bool ParseGridTrackListWithFirstLineNames(nsCSSValue& aValue, >+ const nsCSSValue& aFirstLineNames); nit: add period at the end of the comment, after <track-size>. >-// If successful, leaves aValue with eCSSUnit_Null for the empty list, >+// If successful, leaves aValue with untouched for the empty list, This comment needs a bit of tweaking. Maybe "If successful, leaves aValue untouched, to indicate that we parsed the empty list," > bool > CSSParserImpl::ParseGridLineNames(nsCSSValue& aValue) > { [...] > if (!GetToken(true)) { > return true; > } > if (!mToken.IsSymbol('(')) { > UngetToken(); > return true; > } As discussed IRL just now, I think this can be condensed to ExpectSymbol('(', true); like you have for '/' in your other patch. (Double-check my thinking on this, though. :)) >+CSSParserImpl::ParseGridTrackListWithFirstLineNames(nsCSSValue& aValue, >+ const nsCSSValue& aFirstLineNames) > { >+ nsCSSValueList* item = aValue.SetListValue(); >+ item->mValue = aFirstLineNames; > >+ item->mNext = new nsCSSValueList; >+ item = item->mNext; >+ // FIXME: add repeat() >+ if (ParseGridTrackSize(item->mValue) != eParsingStatus_Ok) { >+ // We need at least one <track-size>, >+ // so even eParsingStatus_NotFound is an error here. > return false; So right away here, we make a copy of the potentially-large value |aFirstLineNames|, which is unnecessary copying-work if we end up returning false. (And we may return false right away, for some valid syntaxes in 'grid-template', IIRC.) So: Let's delay those first two lines until the very end, where we know we're returning true. That means instead of doing this... item->mNext = new nsCSSValueList; item = item->mNext; ...we'll instead need to do something like this: nsAutoPtr<nsCSSValueList> listAfterFirstLineNames = new nsCSSValueList; nsCSSValueList* item = listAfterFirstLineNames->mNext; And then at the end of this function, only after we know we're succeeding, you can call aValue.SetListValue and set its mNext pointer to listAfterFirstLineNames.forget(), which transfers ownership from the nsAutoPtr. >+ if (result == eParsingStatus_NotFound) { >+ // What we've parsed so far is a valid <track-size>. Stop here. >+ break; I think this means to say <track-list>. This is essentially r=me with the above, but I'll just set feedback+ for now because I'd like to sanity-check the nsAutoPtr usage that I'm suggesting.

(In reply to Daniel Holbert [:dholbert] from comment #35) > >+ if (areasValue.GetUnit() != eCSSUnit_GridTemplateAreas || > >+ rowsValue.GetUnit() != eCSSUnit_List) { > > [...] > > So: we should replace that areasValue check here with an assertion (like the > ones we have in eCSSProperty_transition). Ok, I removed the check. The assertion is already done in areasValue.GetGridTemplateAreas() below that.

Comment on attachment 8394028 [details] [diff] [review] Patch 3 v4: Add the grid shorthand. >+// <'grid-template'> | >+// [ <'grid-auto-flow'> [ <'grid-auto-columns'> [ / <'grid-auto-rows'> ]? ]? ] >+bool >+CSSParserImpl::ParseGrid() >+{ >+ nsCSSValue value; >+ if (ParseVariant(value, VARIANT_INHERIT, nullptr)) { >+ AppendValue(eCSSProperty_grid_template_areas, value); >+ AppendValue(eCSSProperty_grid_template_columns, value); >+ AppendValue(eCSSProperty_grid_template_rows, value); >+ AppendValue(eCSSProperty_grid_auto_flow, value); >+ AppendValue(eCSSProperty_grid_auto_columns, value); >+ AppendValue(eCSSProperty_grid_auto_rows, value); >+ return true; >+ } Since we've got so many sub-properties here, let's replace this with a loop like the one at the beginning of CSSParserImpl::ParseBackground(), to loop over the sub-properties, instead of listing all of them here. >+ // 'none' at the beginning could be a <'grid-auto-flow'> >+ // or a <'grid-template-columns'> (as part of <'grid-template'>) There's one more possibility: 'none' could be alone & be the entirety of a <'grid-template'> expression. (This possibility isn't explicitly handled right now, but I think that's OK, because the right behavior for this case falls out of our ParseGridShorthandAutoProps() behavior, which sets grid-template-* to 'none'.) Anyway -- please update this comment to mention this usage of 'none' and to indicate that we handle it correctly via ParseGridShorthandAutoProps(). >+ AppendValue(eCSSProperty_grid_template_columns, value); >+ >+ // Set other subproperties to their initial values. >+ AppendValue(eCSSProperty_grid_auto_flow, value); >+ value.SetAutoValue(); >+ AppendValue(eCSSProperty_grid_auto_columns, value); >+ AppendValue(eCSSProperty_grid_auto_rows, value); >+ >+ return ParseGridTemplateAfterSlash(/* aColumnsIsTrackList = */ false); s/other subproperties/grid-auto-* subproperties/ (Since we've only set grid-template-columns so far, "other" sounds like it means "the other ones aside from grid-template-columns", but that's not what you're trying to say) >+// Parse [ <'grid-auto-columns'> [ / <'grid-auto-rows'> ]? ]? >+// for the 'grid' shorthand, >+// given a value for an already-parsed <'grid-auto-flow'> >+bool >+CSSParserImpl::ParseGridShorthandAutoProps() The "given a value" comment is confusing here, since this function doesn't take any arguments. Maybe replace with: // given that the caller has already parsed the <'grid-auto-flow'> part of the // shorthand grammar. or something like that. >+{ >+ nsCSSValue autoColumnsValue; >+ nsCSSValue autoRowsValue; >+ nsCSSValue templateValue; Move the 'templateValue' declaration down where you actually use it, and declare it as nsCSSValue templateValue(eCSSUnit_None); instead of calling SetNoneValue as a separate step. r=me with the above.

Comment on attachment 8394689 [details] [diff] [review] Patch 2b v3: Refactor grid-template-{columns,rows} parsing, preparing for the grid-template shorthand. >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp > struct CSSParserInputState { > nsCSSScannerPosition mPosition; > nsCSSToken mToken; > bool mHavePushBack; > }; > >+ >+MOZ_BEGIN_ENUM_CLASS(nsParsingStatus, int32_t) (probably revert the extra blank line before MOZ_BEGIN_ENUM_CLASS? It wasn't there in previous patch; assuming it's accidental.) >+ // Parse an optional <line-names> expression. >+ // If successful, leave aValue with untouched, s/with untouched/untouched/ >+ item = aValue.SetListValue(); >+ item->mValue = aFirstLineNames; >+ item->mNext = firstTrackSizeItem.forget(); >+ MOZ_ASSERT(aValue.GetListValue() && aValue.GetListValue()->mNext && >+ aValue.GetListValue()->mNext->mNext, > "<track-list> should have a minimum length of 3"); >+ return true; Add a comment before this last chunk to make it clear what we're doing -- something like: // Set up our outparam as a list, with aFirstLineNames as the first entry, // followed by the rest of the <track-list> that we just finished parsing. r=me with that. Thanks!

Batched Try run for this and 984760 https://tbpl.mozilla.org/?tree=Try&rev=a897a2c983b4

(In reply to Simon Sapin (:SimonSapin) from comment #44) > Batched Try run for this and 984760 > https://tbpl.mozilla.org/?tree=Try&rev=a897a2c983b4 So in the windows bustage there, for some reason, nsParsingStatus is confusing our NS_FOR_CSS_SIDES() macro. That macro is just shorthand to loop over the 4 possible values of mozilla::css::Side enum, defined here: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#22 (and Side itself is a traditional enum, not a typed enum, defined here: http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Types.h#263 ) The error message is: { nsCSSParser.cpp(8194) : error C2678: binary '++' : no operator found which takes a left-hand operand of type 'mozilla::css::Side' (or there is no acceptable conversion) nsCSSParser.cpp(86): could be 'int `anonymous-namespace'::operator ++(`anonymous-namespace'::nsParsingStatus::Enum &,int)' nsCSSParser.cpp(86): or 'int &`anonymous-namespace'::operator ++(`anonymous-namespace'::nsParsingStatus::Enum &)' while trying to match the argument list '(mozilla::css::Side, int)' } So the compiler is wanting to make NS_FOR_CSS_SIDES() use an ::operator ++() that takes a nsParsingStatus for some reason. But we provide an inline "operator++(mozilla::css::Side& side, int)" operator in nsStyleConsts.h, right below the NS_FOR_CSS_SIDES() macro, and that's the operator that it *should* be finding and using here. ANYWAY: If you're up for trying to figure out what's going wrong here, please do; but, I'm also fine with reverting the typed-enum change and using a traditional enum here, as long as you file a followup bug for switching to a typed enum. (We really *should* be using a typed-enum for nsParsingStatus, since it is *dangerously* close to a bool, semantically, and people will be tempted to accidentally use it as a bool.)

(Forgot to mention: I tried to reproduce the build error on Windows on my thinkpad, using MSVC2012, and I could not. :-/ It build successfully, with the Try run's patch stack applied. So to address this correctly [potentially in a followup bug], we may have to use a specific MSVC version and/or TryServer trial-and-error.)

(In reply to Daniel Holbert [:dholbert] from comment #45) > (In reply to Simon Sapin (:SimonSapin) from comment #44) > > Batched Try run for this and 984760 > > https://tbpl.mozilla.org/?tree=Try&rev=a897a2c983b4 > > So in the windows bustage there, for some reason, nsParsingStatus is > confusing our NS_FOR_CSS_SIDES() macro. That macro is just shorthand to > loop over the 4 possible values of mozilla::css::Side enum, defined here: > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h#22 > (and Side itself is a traditional enum, not a typed enum, defined here: > http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Types.h#263 ) > > [...] I don’t understand. nsParsingStatus is never used anywhere near mozilla::css::Side as far as I know.

(It makes no sense to me either, FWIW. I'm guessing it's due to some subset of: hidden complexity inside the ENUM_CLASS macros, the way typed enums work, and the way by which the compiler identifies an appropriate operator++ method.)

Looks like declaring the enum class outside of the file's anonymous namespace fixes the Windows bustage, per my Try run with that change: https://tbpl.mozilla.org/?tree=Try&rev=84a812c153d7 (No idea why this would be the case -- maybe a compiler bug? Anyway, we know how to work around it now.) (The builds haven't quite completed yet, but they've been running for ~15 minutes longer than the failed builds from the earlier Try run, which suggests that they successfully got past this code. If the new Try run ends up going well altogether, then I'll go ahead and land these patches with that tweak.)

Enough of that followup Try run has completed to give me confidence in this being good-to-go, so I landed this, with the parsing-status enum moved outside of the anonymous namespace in the patch that adds it (2b). https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4e79053a4e https://hg.mozilla.org/integration/mozilla-inbound/rev/a05f6150fce0 https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1009083dac https://hg.mozilla.org/integration/mozilla-inbound/rev/887bdaaf9829

https://hg.mozilla.org/mozilla-central/rev/9c4e79053a4e https://hg.mozilla.org/mozilla-central/rev/a05f6150fce0 https://hg.mozilla.org/mozilla-central/rev/ab1009083dac https://hg.mozilla.org/mozilla-central/rev/887bdaaf9829