We should remove 'grid-auto-flow: stack' because it has been removed from the spec: http://dev.w3.org/csswg/css-grid/#propdef-grid-auto-flow

Comment on attachment 8543660 [details] [diff] [review] fix Stealing review. >Bug 1117538 - [css-grid] Remove 'grid-auto-flow: stack'. r=simon.sapin > >Because it has been removed from the spec: >http://dev.w3.org/csswg/css-grid/#propdef-grid-auto-flow Might be better to make the commit message link to the www-style post that announced the removal, here: http://lists.w3.org/Archives/Public/www-style/2014Dec/0321.html ...since commit messages are largely for future archeologists, and the spec ED on dev.w3.org could change completely (e.g. maybe the "grid-auto-flow" property gets renamed at the last minute -- stranger things have happened) by the time someone visits that link when poking at your commit message. (Also: that www-style announcement included a caveat: "Unless the WG objects [...]". I'm not sure how much time needs to pass before we can conclude that the WG doesn't object; I suppose in the unlikely event that "stack" is added back due to an objection, we can just revert this patch.) >+++ b/layout/style/nsCSSParser.cpp >@@ -8524,18 +8517,17 @@ CSSParserImpl::ParseGrid() > if (!GetToken(true)) { > return false; > } > > // The values starts with a <'grid-auto-flow'> if and only if > // it starts with a 'stack', 'dense', 'column' or 'row' keyword. > if (mToken.mType == eCSSToken_Ident) { > nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent); >- if (keyword == eCSSKeyword_stack || >- keyword == eCSSKeyword_dense || >+ if (keyword == eCSSKeyword_dense || The comment here needs updating to remove 'stack'. r=me with that comment-fix.

Comment on attachment 8543660 [details] [diff] [review] fix Review of attachment 8543660 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +7659,2 @@ > if (!(bitField & NS_STYLE_GRID_AUTO_FLOW_ROW || > + bitField & NS_STYLE_GRID_AUTO_FLOW_COLUMN)) { (Code change needed here.) The new grammar is: [ row | column ] || dense So 'grid-auto-flow: dense;' is acceptable, but would be rejected by this code. On the other hand, this code accepts 'grid-auto-flow: column row;' which should be invalid. So this should be changed to if (bitField & NS_STYLE_GRID_AUTO_FLOW_ROW && bitField & NS_STYLE_GRID_AUTO_FLOW_COLUMN) { (ParseBitmaskValues already returns false when it can not parse at least one keyword.) @@ -7661,5 @@ > - bitField & NS_STYLE_GRID_AUTO_FLOW_STACK)) { > - return false; > - } > - > - // 'stack' without 'row' or 'column' defaults to 'stack row' (Code change not necessarily needed here.) Note that the spec now says: > If neither row nor column is provided, row is assumed. So 'grid-auto-flow: dense;' is identical to 'grid-auto-flow: dense row;', but it’s up to you how you want to represent it internally. Rather than a bitfield with one bit per keyword, I think it would make sense to have two bits: one for column v.s. row (defaulting to row), and one for sparse packing v.s. dense packing.

Comment on attachment 8543660 [details] [diff] [review] fix Review of attachment 8543660 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSParser.cpp @@ +7646,5 @@ > } > > static const int32_t mask[] = { > NS_STYLE_GRID_AUTO_FLOW_ROW | NS_STYLE_GRID_AUTO_FLOW_COLUMN, > + NS_STYLE_GRID_AUTO_FLOW_DENSE, This should be removed: 'mask' (should be plural) is an "array of masks for mutually-exclusive property values" (quoting the ParseBitmaskValues comment) so a mask with only one value does nothing. @@ +7659,2 @@ > if (!(bitField & NS_STYLE_GRID_AUTO_FLOW_ROW || > + bitField & NS_STYLE_GRID_AUTO_FLOW_COLUMN)) { Please disregard my earlier comment here, I misremembered how the 'mask' parameter of ParseBitmaskValues works. 'mask' already takes care of rejecting 'grid-auto-flow: column row;', so this 'if' black can be remove entirely.

[marking my comment 4 as obsolete, since comment 5 expresses the same thing in a more actionable way]

Comment on attachment 8543660 [details] [diff] [review] fix Review of attachment 8543660 [details] [diff] [review]: ----------------------------------------------------------------- Oops, sorry Daniel for the redundant comments/review. It’s twice now that I start typing something up, go do something else, and by the time I hit submit you’ve commented as well :] ::: layout/style/test/property_database.js @@ +5075,5 @@ > "column", > "column dense", > "row dense", > "dense column", > "dense row", Please add a value here with just "dense".

(In reply to Simon Sapin (:SimonSapin) from comment #3) > (Code change needed here.) The new grammar is: > > [ row | column ] || dense Good catch, I didn't notice the grammar changed in that way too. I'll do that change as a separate patch...

Fixing dholbert's nits, carrying r+.

I think you still need to remove the "NS_STYLE_GRID_AUTO_FLOW_DENSE" entry from the mask[] array, in ParseGridAutoFlow(), as noted at beginning of comment 5 (and the very end of hidden comment 4). This should probably happen in "part 1", since that array-entry becomes useless as of that patch.

(In reply to Daniel Holbert [:dholbert] from comment #4) > Now that I focus on that code, too, I'm realizing the second entry (for > "_DENSE") in that mask array is now useless -- that line should just be > removed. I think you're mistaken; ParseBitmaskValues accepts zero or one value from each group, so a single value makes perfect sense. I think the code: static const int32_t mask[] = { NS_STYLE_GRID_AUTO_FLOW_ROW | NS_STYLE_GRID_AUTO_FLOW_COLUMN, NS_STYLE_GRID_AUTO_FLOW_DENSE, MASK_END_VALUE }; implements the desired "[ row | column ] || dense" exactly. If I remove the second group, then we'd have to manually check for 'dense' before the ParseBitmaskValues call, and then again after it, but only if we didn't already find it before... which seems unnecessary when ParseBitmaskValues can do that for us.

(In reply to Mats Palmgren (:mats) from comment #12) > I think you're mistaken; ParseBitmaskValues accepts zero or one value from > each group I agree that things *do work* if you include this entry, but I'm asserting that things *also* work if you drop the entry. (And perf will be slightly better, since we have one fewer mask-array-entry to iterate through, in MergeBitmaskValue().) Hence, we should remove it. > If I remove the second group, then we'd have to manually check for > 'dense' before the ParseBitmaskValues call, and then again after it, Why would you need to do that? I think ParseBitmaskValues handles this correctly on its own, by deferring to ParseEnum(), which finds the keyword/bit mapping in kGridAutoFlowKTable, and then MergeBitmaskValue(), which immediately fails if the bit has already been set. Could you clarify what sorts of values you think would be incorrectly parsed/rejected, if we removed NS_STYLE_GRID_AUTO_FLOW_DENSE from the mask array? (and/or, what code relies on that mask being present in order for things to parse properly)

Ah, now I see what you mean. I didn't realize that ParseBitmaskValues accepted all values from the keyword table unless you mentioned them in the masks somewhere. I agree it's better without NS_STYLE_GRID_AUTO_FLOW_DENSE then.

Removed the NS_STYLE_GRID_AUTO_FLOW_DENSE line, as requested so r+ implied.

Bah, right file this time...

Looks good -- thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f96dee075de https://hg.mozilla.org/integration/mozilla-inbound/rev/576c21be1808

https://hg.mozilla.org/mozilla-central/rev/5f96dee075de https://hg.mozilla.org/mozilla-central/rev/576c21be1808