Closed Bug 976787 Opened 10 years ago Closed 10 years ago

Add style-system support for CSS Grid longhand properties

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: SimonSapin, Assigned: SimonSapin)

References

(Blocks 2 open bugs, )

Details

Attachments

(5 files, 20 obsolete files)

34.02 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
21.30 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
19.68 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
33.78 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
51.52 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
Splitting up bug 975501.
Patch 1 (Attachment 8381727 [details] [diff]) is the same as Patch 3 v2 in bug 975501.
Summary: CSS Grid Layout style system, part 2 → Add style-system support for grid container CSS properties
Comment on attachment 8381727 [details] [diff] [review]
Patch 1: Add the grid-template-{columns,rows} properties the style system.

Partial review (more coming):

>+++ b/layout/style/nsStyleStruct.cpp
> nsStylePosition::nsStylePosition(const nsStylePosition& aSource)
>+  : mGridTemplateRows(aSource.mGridTemplateRows)
>+  , mGridTemplateColumns(aSource.mGridTemplateColumns)
> {
>   MOZ_COUNT_CTOR(nsStylePosition);

The initializer list there is backwards, with respect to the .h file's ordering of these member variables. This triggers a build warning (treated as an error in warnings-as-errors builds).

Swap the order to fix the warning, and consider adding "ac_add_options --enable-warnings-as-errors" to your .mozconfig. :)

>-  memcpy((nsStylePosition*)this, &aSource, sizeof(nsStylePosition));
>+  // XXX this relies on the order of field declarations, see nsStyleStruct.h.
>+  memcpy((nsStylePosition*)this, &aSource, offsetof(nsStylePosition, mGridTemplateColumns));

In Gecko, "XXX" generally means "TODO" (or "something is wrong here"). That's not the impression you're trying to give, here, though, so XXX is probably not what you want.

I think this wants to be "// IMPORTANT: ", and it probably should say something a bit more explicit, e.g.
 // "IMPORTANT: This assumes that mGridTemplateColumns is the
 // first non-memcpyable member variable in nsStylePosition. See
 // comments nsStyleStruct.h for more.

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>index 5337e6d..4a9934d 100644
>+struct nsStyleGridTrackList {
>+  // none: both arrays are empty
>+  // <track-list>: mLineNameLists.Length() == mTrackSizes.Length() + 1
>+  nsTArray<nsTArray<nsString>> mLineNameLists;
>+  nsTArray<nsStyleCoord> mTrackSizes;

Why the +1?

Also: does this mean that we could have mTrackSizes.Length() == 0 and mLineNameLists.Length() == 1, or is that invalid? (i.e. if one is non-empty, must the other also be non-empty?)

>@@ -1191,16 +1203,20 @@ struct nsStylePosition {
>+  // Fields so far can be memcpy()âed.

Fix weird ascii characters here         ^^^^

>+  // XXX Following fields need to  have their copy constructor called.
>+  nsStyleGridTrackList mGridTemplateColumns;
>+  nsStyleGridTrackList mGridTemplateRows;

Drop the extra space between "to" and "have".

And as noted above, XXX probably wants to be "IMPORTANT" or "NOTE" here as well. (and maybe add "...when we're being copied" to the end of this comment, or else it's a bit unclear when/why these fields "need to have their copy constructor called")

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
>+	gCSSProperties["grid-template-columns"] = {
>+		domProp: "gridTemplateColumns",
>+		inherited: false,
>+		type: CSS_TYPE_LONGHAND,
>+		initial_values: [ "none" ],
>+		other_values: [ "40px", "40px 2em", "() 40px (foo) 2em (bar baz This\ is\ one\ ident)" ],
>+		invalid_values: [ "(foo)" ]
>+	};

We should test a lot more values here, to get good coverage. (which means you'll probably want to switch to listing one value per line, as we do for some other properties that have long lists)

(Take my suggestions below with a grain of salt, since I've only just skimmed the spec about these properties so far, so I might be wrong on specifics)

I think other_values should include at least the following:
 - "auto" (by itself and as part of a list)
 - percent lengths
 - intrinsic-size keywords like -moz-max-content
 - minmax() values
 - calc() values
 - possibly some commented-out values that use "fr" and "repeat()", ideally with a reference to followup bug number(s) for bugs where we'll implement these features (which I'd suggest filing now, to make sure we don't lose track of those features).
 - possibly 'subgrid' (similarly commented-out with a bug number), though maybe not, depending on how at-risk it is. (the spec says it's at risk, but I don't know how likely/unlikely it is to be removed)

I think invalid_values should include at least the following:
 * Unbalanced parenthesis (by themselves and as part of otherwise-valid values)
 * Empty string
 * At least one otherwise-valid value that uses a reserved identifier (i.e. invalid <custom-ident>) as the name of a line (e.g. "inherit", "initial", "auto")
 * Values with multiple parenthesized sets of names in a row, e.g. "(foo) (bar) 4px", and "(foo) 4px (bar) (baz)"
   (Is that invalid? if not, redirect this to 'other_values' :) )
(In reply to Daniel Holbert [:dholbert] from comment #3)
> I think this wants to be "// IMPORTANT: ", and it probably should say
> something a bit more explicit, e.g.
>  // "IMPORTANT: This assumes that mGridTemplateColumns is the
>  // first non-memcpyable member variable in nsStylePosition. See
>  // comments nsStyleStruct.h for more.

Per IRC, we can probably enforce this assumption with something like...
  static_assert(sizeof(nsStylePosition) ==
                offsetof(nsStylePosition, mGridTemplateColumns) +
                sizeof(mGridTemplateColumns) + sizeof(mGridTemplateRows));

...with a code-comment above it, saying something like "If you add any memcpy'able member vars, they should be declared before mGridTemplateColumns.  If you add any non-memcpy'able member vars, they should be declared after mGridTemplateColumns, and you should invoke their copy constructor in the init list above and update this static-assert to include their "sizeof()".

(This static_assert might be technically invalid if there's any packing space between member variables, but we can add hacks to work around that if & when we run up against it.)
Comment on attachment 8381727 [details] [diff] [review]
Patch 1: Add the grid-template-{columns,rows} properties the style system.

Second wave of review comments:
===============================

First, a few more "other_values" worth testing (building on the suggestions from my previous comment):
 * Something involving "( )"  (empty list, with a space between the parens)
 * Something involving " ( foo ) " (whitespace padding on all sides of the parenthesis -- which hopefully tests that we're using GetToken(true) and not GetToken(false) in our parsing code)
 * Something involving "(-foo)", with a leading-dash on the ident inside of the parenthesis (which is valid, e.g. to allow -vendor-prefixed values)

...and a few more "invalid_values":
 * Something with a non-IDENT (e.g. a number or percentage or length) value inside the parenthesized name list.
 * Something with a negative percentage value. (Per one of the comments below, you've got a FIXME to reject those, which I think can be easily remedied with ParseNonNegativeVariant)
 * Something with a negative length value. (similarly, to be sure ParseNonNegativeVariant is working)

>From: Simon Sapin <simon.sapin@exyr.org>
>Add the grid-template-{columns,rows} properties the style system.

Add the (new) bug number to the commit message.

>+++ b/layout/style/nsCSSParser.cpp
>+// Parse an optional <line-names>.

nit: maybe "an optional <line-names> expression." to make the pluralization make more sense (an...expression).

>+// If successful, sets aValue to a eCSSUnit_None for the empty list,
>+// or a eCSSUnit_List of eCSSUnit_Ident.
>+bool
>+CSSParserImpl::ParseGridLineNames(nsCSSValue& aValue)
>+{

Mention in the comment that if we don't see an open-paren character, we'll trivially succeed (behaving as if we parsed an empty list).

>+  if (GetToken(true)) {
>+    if (mToken.IsSymbol('(')) {
[...]
>+    } else {  // Not a '('
>+      UngetToken();
>+    }
>+  }
>+  // No list of names, same as an empty list of names
>+  aValue.Reset();
>+  return true;

IMHO, it's cleaner (and more indentation-saving, and easier to reason about) if you handle special/error cases via early-returns rather than nested logic. (It also make the cleanup work -- UngetToken() in this case -- a little clearer, since it ends up be adjacent to the GetToken that it's reversing).

So I'd suggest switching to use early-returns when these initial GetToken() & IsSymbol('(') calls fail.

>+        if (mToken.IsSymbol(')')) {
>+          // Empty list of names "()"
>+          aValue.Reset();
>+          return true;
>+        } else if (eCSSToken_Ident == mToken.mType) {

Unnecessary "else" after return. Replace with "if" (on a new line).

>+          nsCSSValueList* item = aValue.SetListValue();
>+          item->mValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);

It looks to me like this will (incorrectly) accept "inherit", "initial", etc. inside the parenthesized list, which is bad because those aren't valid <custom-ident> values.

We recently fixed a related issue with counter names, in https://hg.mozilla.org/mozilla-central/rev/f30dfa0f184d . You probably want to refactor that cset's checks into a helper-method, which you can use here to check the ident.

I *think* you also need to exclude "auto", "minmax", and "subgrid" from being accepted as <custom-ident> values here, since they're keywords that appear in the property's value's definition. (And possibly also "span"? but probably not)
See my posts to www-style, sanity-checking these points:
 http://lists.w3.org/Archives/Public/www-style/2014Feb/0734.html
 http://lists.w3.org/Archives/Public/www-style/2014Feb/0735.html

>+          for (;;) {
>+            if (!GetToken(true) || mToken.IsSymbol(')')) {
>+              return true;
>+            }

Does the first clause here mean we'll treat "(foo" as valid? That seems wrong. It looks like the close-paren is a required part of <line-names> in the spec, so once we've parsed an open-paren, we should *only* return true once we've parsed a close-paren.

>+            if (eCSSToken_Ident == mToken.mType) {
>+              item->mNext = new nsCSSValueList;
>+              item = item->mNext;
>+              item->mValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
>+            } else {
>+              break;
>+            }

We'll need to check the ident here, too (to be sure it's not a reserved keyword name).

>+// Parse a <track-size>
>+bool
>+CSSParserImpl::ParseGridTrackSize(nsCSSValue& aValue)
>+{
>+  // FIXME: add <flex> | min-content | max-content | minmax() | auto
>+  // FIXME: reject negative percentages
>+  return ParseVariant(aValue, VARIANT_LPCALC | VARIANT_NONNEGATIVE_DIMENSION, nullptr);

I think this should be ParseNonNegativeVariant. (That addresses your second FIXME there, and I think prevents you from needing to pass VARIANT_NONNEGATIVE_DIMENSION.)

And for your first FIXME, you can add address most of those cases by changing your flags to:
   VARIANT_AUTO | VARIANT_LPCALC | VARIANT_KEYWORD
...and pass in kWidthKTable instead of nullptr your property-table (assuming this takes the same length keywords as "width", which I'd expect it does).

That just leaves "minmax()" and "fr" units; if you want to hold off on those, you should probably spin off a followup for them, and mention the bug number in the FIXME comment here.
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (It also make the cleanup work -- UngetToken() in this case -- a
> little clearer, since it ends up be adjacent to the GetToken that it's
> reversing).

(er, not quite adjacent, but much closer at least)
Comment on attachment 8381727 [details] [diff] [review]
Patch 1: Add the grid-template-{columns,rows} properties the style system.

Third wave of review comments:
=============================

>+bool
>+CSSParserImpl::ParseGridTrackList(nsCSSProperty aPropID)
>+{
>+  nsCSSValue value;
>+  if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
>+    // 'inherit', 'initial', 'unset' and 'none' must be alone
>+    if (!ExpectEndProperty()) {
>+      return false;
>+    }
>+  } else {
>+    // FIXME: add repeat()
>+    nsCSSValueList* item = value.SetListValue();
[...]
>+  }
>+  AppendValue(aPropID, value);
>+  return true;
>+}

If you like, I think this would be clearer if you copypasted the "AppendValue; return true;" up into the end of the INHERIT | NONE clause (so that all of the inherit/initial/etc stuff is handled entirely there, including the return).

Then the rest of the function can be de-indented (since it no longer would need "else") and would be entirely about the "normal" case, which IMHO makes this easier to reason about.

>+    // Value is a list of odd length >= 3,
>+    // starting with a <line-names> and alternating between that and <track-size>

Could you move this comment earlier (maybe right before we call "value.SetListValue()", and then start it with "|value| will be [...]"

Might be worth also modifying the comment to say "<line-names> (which is itself a list)", to make it clear that we have sub-lists branching off of this list, rather than everything being in one continuous list (as it might appear from a brief skiming of this method & ParseGridLineNames.

ALSO: RE "odd length >= 3" -- could you assert this, somewhere before the final return, to sanity-check / prove this comment?

e.g.
  MOZ_ASSERT(value.GetListValue() && value.GetListValue()->mNext &&
             value.GetListValue()->mNext->mNext,
             "<track-list> should have a minimum length of 3");

>@@ -1289,17 +1289,24 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
[...]
>-    GetListValue()->AppendToString(aProperty, aResult, aSerialization);
>+    switch (aProperty) {
>+      case eCSSProperty_grid_template_columns:
>+      case eCSSProperty_grid_template_rows:
>+        GetListValue()->AppendGridTemplateToString(aProperty, aResult, aSerialization);
>+        break;
>+      default:
>+        GetListValue()->AppendToString(aProperty, aResult, aSerialization);
>+    }

I don't think you want to make this change at this level. This check should probably happen *inside* of nsCSSValueList::AppendToString().  (And AppendGridTemplateToString should just be a static helper-function, only mentioned inside the .cpp file; no need to expose it as a public method in the .h file.)

Ah, but that doesn't quite work, because AppendGridTemplateToString itself calls nsCSSValueList::AppendToString() for its sub-lists passing in this property, which would result in a recursive death-spiral.

I'd suggest the following:
 (A) separate out the current guts of AppendToString() into a static helper-function, "AppendCSSListToString()" or something.
 (B) Make nsCSSValueList::AppendToString check the property & call either your grid helper function, or this new helper-function.
 (C) Your grid helper-function should call this new helper-function to append its sublists.

This is sort of similar to what you have now, except pushed down one layer of abstraction (and now making nsCSSValueList::AppendToString always do the right thing, and removing the need to expose an additional public method).

>+void
>+nsCSSValueList::AppendGridTemplateToString(nsCSSProperty aProperty, nsAString& aResult,
>+                                           nsCSSValue::Serialization aSerialization) const
>+{
>+  // This is called for the "list" thatâs the top-level value of the proprety

Here's one more non-ASCII quote character; please replace with standard '. (From a quick search, it looks like this is the last one of those in the patch.)

Also: s/proprety/property/

(Also: per above, this shouldn't be a nsCSSValueList method anymore; it should just be a static function at the file scope.)

>+  for (;;) {
>+    bool addSpaceSpearator = true;
>+    switch (val->mValue.GetUnit()) {
>+
>+      // Represents an empty <line-names>. Serializes to nothing.
>+      case eCSSUnit_Null:
[...]
>+      // Non-empty <line-names>
>+      case eCSSUnit_List:
>+      case eCSSUnit_ListDep:
[...]
>+      default:
>+        val->mValue.AppendToString(aProperty, aResult, aSerialization);

Add a comment saying "<track-size>" over that 'default' case, for consistency with the cases above it.
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Second wave of review comments:
> ===============================
[...]
> I *think* you also need to exclude "auto", "minmax", and "subgrid" from
> being accepted as <custom-ident> values here, since they're keywords that
> appear in the property's value's definition. (And possibly also "span"? but
> probably not)
> See my posts to www-style, sanity-checking these points:
>  http://lists.w3.org/Archives/Public/www-style/2014Feb/0734.html
>  http://lists.w3.org/Archives/Public/www-style/2014Feb/0735.html

Following up on this -- Tab replied to my www-style posts, confirming that "auto" and "subgrid" need to be explicitly rejected as <custom-ident> values here, though "minmax" and "span" should not be rejected. (since 'minmax' is a function, not a keyword, and 'span' doesn't appear in these properties' value-definition)
Comment on attachment 8381727 [details] [diff] [review]
Patch 1: Add the grid-template-{columns,rows} properties the style system.

Fourth (and final) wave of review comments:
===========================================

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+CSSValue*
>+nsComputedDOMStyle::GetGridTrackSize(const nsStyleCoord& aTrackSize)
>+{
>+  // FIXME: Every <track-breadth> should be resolved into 'px' here,
>+  // based on layout results.
>+  // (Ie. the "resolved value" is not the computed value.)
>+  // http://dev.w3.org/csswg/css-grid/#resolved-track-list

This is only true if we're not in a "display:none" subtree, per
 http://lists.w3.org/Archives/Public/www-style/2014Feb/0762.html

Can you clarify it comment to mention that? (Maybe just "FIXME: If we have a frame, then our <track-breadth> should [etc]")

>+  nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>+  if (aTrackSize.IsCalcUnit()) {
>+    SetValueToCalc(aTrackSize.GetCalcValue(), val);
>+  } else if (aTrackSize.GetUnit() == eStyleUnit_Coord) {
>+    val->SetAppUnits(aTrackSize.GetCoordValue());
>+  } else {
>+    val->SetPercent(aTrackSize.GetPercentValue());
>+  }

I think this "if" cascade can be entirely replaced with a "SetValueToCoord" invocation, like the one we use in DoGetFlexBasis().

>+CSSValue*
>+nsComputedDOMStyle::GetGridTrackList(const nsStyleGridTrackList& aTrackList)
>+{
[...]
>+  if (numSizes == 0) {
>+    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
>+    val->SetIdent(eCSSKeyword_none);
>+    return val;
>+  }
>+
>+  nsDOMCSSValueList *valueList = GetROCSSValueList(false);

Nit: s/nsDOMCSSValueList */nsDOMCSSValueList* /, for consistency.

>+  for (uint32_t i = 0;; i++) {
>+    // mLineNameLists is expected to have numSizes + 1 items
>+    const nsTArray<nsString>& lineNames = aTrackList.mLineNameLists[i];

Promote this comment into an actual assertion, to sanity-check that it's telling the truth, before you start indexing into mLineNameLists. (Maybe outside the loop.)

It would also help perhaps to add a brief explanation for this "+1", alongside this comment/assertion, so that it's not quite so mysterious.  (Per my first chunk of review feedback, I didn't understand where the +1 came from, when I was first glancing at this.) Maybe something like:
 // (We have one grid-line before each track, plus one at the very end.)

>+    uint32_t nameListLength = lineNames.Length();
>+    if (nameListLength > 0) {

Replace with:
 if (!lineNames.IsEmpty())

>+++ b/layout/style/nsRuleNode.cpp
>+static void
>+SetGridTrackList(const nsCSSValue& aValue,
[...]

At the very end of this method, add a fatal assertion that aResult satisfies our "both empty, or both nonempty with mLineNameLists.Length() == mTrackSizes.Length() + 1" invariant.

(Fatal because if that ends up failing, then things are very wrong and we're likely to index out of bounds, so we really want to make sure we notice that sort of thing.)

>+  switch (aValue.GetUnit()) {
[...]
>+  default:
>+    aResult.mLineNameLists.Clear();
>+    aResult.mTrackSizes.Clear();
>+    const nsStyleCoord dummyParentCoord;

Declare this (dummyParentCoord) further down, where it's actually used.

>+    const nsCSSValueList* item =  aValue.GetListValue();
                                  ^
Delete extra space ---------------

Also: It might be useful to add a brief comment here (before you start walking over the list) mentioning the expected structure of aValue's list. (There should be an odd number of entries, with the odd entries representing lists of track names, and the even entries representing track sizes.)

>+      // None means empty list, nothing more to do.
>+      if (item->mValue.GetUnit() != eCSSUnit_Null) {
>+        const nsCSSValueList* subItem = item->mValue.GetListValue();

I think you mean s/None/Null unit/, right?

>+      // Compute a <track-size> value
>+      nsStyleCoord size;
>+      SetCoord(item->mValue, size, dummyParentCoord, SETCOORD_LP | SETCOORD_STORE_CALC,
>+               aStyleContext, aPresContext, aCanStoreInRuleTree);
>+      aResult.mTrackSizes.AppendElement(size);

Rewrap this so that it fits in 80 characters.

Also, you'll need to add a few more flags here, to reflect the updated parser flags I suggested at the end of comment 5. I think you just need to add SETCOORD_AUTO and SETCOORD_ENUMERATED.

Also: before the SetCoord call, please add a comment saying that dummyParentCoord won't be used, because our item->mValue won't be eCSSUnit_Inherit nor eCSSUnit_Unset (the only values that could make us look at dummyParentCoord), because the parser should have rejected those values.

(You might even add an assertion that item->mValue isn't inherit or unset, to enforce/prove this? A comment should be fine too, though.)

>+      item = item->mNext;
>+      NS_ASSERTION(item, "Expected a eCSSUnit_List of odd length.");

Two things:
 (a) Drop the period at the end of the assertion message (otherwise, we end up printing assertion messages with ".:" at the end of the description, like:
 ###!!! ASSERTION: This is my assertion.: 'foo == bar', foo.cpp, line 1

 (b) Use MOZ_ASSERT instead of NS_ASSERTION here. NS_ASSERTION is useful when something's not *too* bad and we can still proceed usefully and see how far we get. But in this case, it's pretty bad [i.e. it could hypothetically mean we index past the end of an array], so we definitely want to catch it. And we can't usefully proceed -- we're going to crash on the next loop-iteration, as soon as we dereference the null |item| on the next loop iteration. So we might as well abort more proactively with a useful message, in debug builds.

>+++ b/layout/style/nsStyleStruct.cpp
>+nsStylePosition::nsStylePosition(void)
>+{
[...]
>+  // mGridTemplateRows and mGridTemplateColumns get their default constructors.
>+  // Empty arrays in nsStyleGridTrackList represent 'none'

Maybe reword that last line as:
  // which initializes them with empty arrays, which represent the
  // properties' initial value, "none".

(Otherwise, that comment seems like a bit of a non sequitor)
Blocks: 978212
(In reply to Daniel Holbert [:dholbert] from comment #3)
> consider adding "ac_add_options --enable-warnings-as-errors" to your .mozconfig. :)

I was sceptical, given the "871 compiler warnings present" output the build usally gives me, but this worked as intended.


> >diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
> >index 5337e6d..4a9934d 100644
> >+struct nsStyleGridTrackList {
> >+  // none: both arrays are empty
> >+  // <track-list>: mLineNameLists.Length() == mTrackSizes.Length() + 1
> >+  nsTArray<nsTArray<nsString>> mLineNameLists;
> >+  nsTArray<nsStyleCoord> mTrackSizes;
> 
> Why the +1?

Drawing N columns requires N+1 lines.


> Also: does this mean that we could have mTrackSizes.Length() == 0 and
> mLineNameLists.Length() == 1, or is that invalid? (i.e. if one is non-empty,
> must the other also be non-empty?)

That should never happen. I rewrote the comment; this hopefully clarifies things:

  // http://dev.w3.org/csswg/css-grid/#track-sizing
  // This represents either:
  // * 'none': both arrays are empty
  // * A <track-list>: mTrackSizes is non-empty, and
  //   mLineNameLists.Length() == mTrackSizes.Length() + 1
  //   (Drawing N columns requires N+1 lines.)
  //   An ommited <line-names> is still represented in mLineNameLists,
  //   as an empty array.


(In reply to Daniel Holbert [:dholbert] from comment #4)
> >  // "IMPORTANT: This assumes that mGridTemplateColumns is the
> >  // first non-memcpyable member variable in nsStylePosition. See
> >  // comments nsStyleStruct.h for more.
> 
> Per IRC, we can probably enforce this assumption with something like...
>   static_assert(sizeof(nsStylePosition) ==
>                 offsetof(nsStylePosition, mGridTemplateColumns) +
>                 sizeof(mGridTemplateColumns) + sizeof(mGridTemplateRows));
> 
> [...]
> 
> (This static_assert might be technically invalid if there's any packing
> space between member variables, but we can add hacks to work around that if
> & when we run up against it.)

Do you think this is necessary? It feels like more of a hack than just having a comment to document the assumption.


(In reply to Daniel Holbert [:dholbert] from comment #5)
> >+  if (GetToken(true)) {
> >+    if (mToken.IsSymbol('(')) {
> [...]
> >+    } else {  // Not a '('
> >+      UngetToken();
> >+    }
> >+  }
> >+  // No list of names, same as an empty list of names
> >+  aValue.Reset();
> >+  return true;
> 
> IMHO, it's cleaner (and more indentation-saving, and easier to reason about)
> if you handle special/error cases via early-returns rather than nested
> logic. (It also make the cleanup work -- UngetToken() in this case -- a
> little clearer, since it ends up be adjacent to the GetToken that it's
> reversing).
> 
> So I'd suggest switching to use early-returns when these initial GetToken()
> & IsSymbol('(') calls fail.

This adds a bit of repetition (code doing the same thing in different cases), but I guess the locality is worth it.


> >+          for (;;) {
> >+            if (!GetToken(true) || mToken.IsSymbol(')')) {
> >+              return true;
> >+            }
> 
> Does the first clause here mean we'll treat "(foo" as valid?

Yes.

http://www.w3.org/TR/CSS2/syndata.html#parsing-errors
> User agents must close all open constructs (for example: blocks, parentheses,
> brackets, rules, strings, and comments) at the end of the style sheet.


> It looks like the close-paren is a required part of <line-names> in the spec

Too keep grammars simple, this "close without error at EOF" behavior is implicit.

http://dev.w3.org/csswg/css-syntax/#rule-defs
> When defining a function or a block, the ending token must be specified in
> the grammar, but if it’s not present in the eventual token stream,
> it still matches.


> And for your first FIXME, you can add address most of those cases by
> changing your flags to:
>    VARIANT_AUTO | VARIANT_LPCALC | VARIANT_KEYWORD
> ...and pass in kWidthKTable instead of nullptr your property-table (assuming
> this takes the same length keywords as "width", which I'd expect it does).

Not kWidthKTable as it has fill-available which does not apply here, and I’d rather not have prefixes. (The property is already behind a pref.) But yeah, I get the idea.
Attachment #8381727 - Attachment is obsolete: true
Attachment #8381727 - Flags: review?(dholbert)
Attachment #8383951 - Flags: review?(dholbert)
(In reply to Simon Sapin (:SimonSapin) from comment #10)
> That should never happen. I rewrote the comment; this hopefully clarifies
> things:
[snip]
>   //   An ommited <line-names> is still represented in mLineNameLists,
>   //   as an empty array.

typo: s/ommited/omitted/

Also: s/empty array/empty sub-array/ perhaps?
(for clarity about which array is empty, since mLineNameLists is itself an *array of arrays*, and it's the *internal* arrays you're talking about here)

> > Per IRC, we can probably enforce this assumption with something like...
> >   static_assert(sizeof(nsStylePosition) == [etc]
> Do you think this is necessary? It feels like more of a hack than just
> having a comment to document the assumption.

Yes, I do. 

It's a hack that accompanies another hack (the memcpy()-only-up-to-this-offset), which makes that other hack more palatable & future-proof. :)

This static_assert is important to help out future people adding to this style struct. Without it, someone will inevitably add a new member-variable at the end of the struct (after your variables), and they'll be baffled at why their added member-variable isn't getting automagically memcpy'd like virtually every other style struct's member-variables are.

With a static_assert, we'll catch that (for any member variables added after yours) and point them right at the style struct's copy constructor which they need to look at & update.
Comment on attachment 8383951 [details] [diff] [review]
Patch 1 v2: Add the grid-template-{columns,rows} properties to the style system.

>+++ b/layout/style/nsCSSKeywordList.h
[...]
>+CSS_KEY(max-content, max_content)
[...]
>+CSS_KEY(min-content, min_content)

I don't think we want to add these unprefixed keywords as part of this patch.

We should be using -moz-max-content and -moz-min-content for now, since those are the versions of these keywords that we use elsewhere. When we decide to unprefix these keywords (or add unprefixed aliases), we can do that *globally* - we shouldn't use prefixed versions for some properties & unprefixed for others.

>--- a/layout/style/nsCSSParser.cpp
>+const nsCSSProps::KTableValue kGridTemplateKTable[] = {
>+  eCSSKeyword_auto,
>+  eCSSKeyword_min_content,
>+  eCSSKeyword_max_content,
>+  // TODO: add eCSSKeyword_subgrid here when we add subgrid support.
>+  eCSSKeyword_UNKNOWN
>+};

This isn't really an array of nsCSSProps::KTableValue (i.e. int16_t)'s -- it's an array of nsCSSKeyword values.

The *real* nsCSSProps::KTableValue[] arrays (in nsCSSProps.cpp) generally alternate between keyword & #defined-integer; this table doesn't fit that pattern (it's only keywords), so it doesn't need to be (& shouldn't be) declared as a nsCSSProps::KTableValue array.

Also: it looks like this is only used as an parameter to ParseGridLineNames (and it's the only array that any clients pass to that method) so if we keep it, we should probably drop the parameter and declare it locally inside of ParseGridLineNames.

BUT, I think we probably should drop it altogether -- see below comment about ParseCustomIdent.

>+const nsCSSProps::KTableValue kCounterDataKTable[] = {
>+  eCSSKeyword_none,
>+  eCSSKeyword_UNKNOWN
>+};
>+

The above all applies here, too.

>+// http://dev.w3.org/csswg/css-values/#custom-idents
>+// Parse an identifier that is not a CSS-wide keyword, nor "default",
>+// nor one of the other keywords that can appear in the value of the property.
>+bool
>+CSSParserImpl::ParseCustomIdent(nsCSSValue& aValue,
>+                                const nsAutoString& aIdentValue,
>+                                const KTableValue* aPropertyKeywordTable)

This either should take an array of nsKeywords, per above, *or* (probably better) it should take an *actual* "KTableValue" keyword-table from nsCSSProps (where the even values are the keyword names you're interested in, and the odd values are #defines & should be skipped).

I think we should try to do the latter thing (take an actual keyword-table from nsCSSProps).

Currently, the grid clients would pass in kGridTrackBreadthKTable, and the counter client doesn't have a keyword-table that it could pass in, but maybe it should instead pass in VARIANT_NONE (in an additional aVariantMask parameter) which signals "this is another keyword, not in the table, which should be excluded"?

Your grid clients would probably want to pass in VARIANT_AUTO | VARIANT_NONE in that additional flags parameter, too, I think.

>+// aPropertyKeyword contains additional keywords for the 'grid' shorthand.
>+bool
>+CSSParserImpl::ParseGridLineNames(nsCSSValue& aValue,
>+                                  const KTableValue aPropertyKeyword[])
>+{
>+  if (!GetToken(true)) {
>+    // No list of names, same as an empty list of names
>+    aValue.SetNoneValue();
>+    return true;
>+  }
> [etc -- subsequent "empty list" clauses in this function]

A "none" nsCSSValue usually represents "the keyword 'none'", which isn't what you're actually trying to represent here. (Note that these properties *do actually* accept the value "none", so usages of eCSSUnit_None that don't actually represent that value are confusing.)

So I don't think it's the right thing to use to represent empty name-lists here.

Instead of that, could you just leave aValue untouched in all of these clauses (at its default eCSSUnit_Null value), and use that to represent an empty list?

(Might be worth adding an assertion at the beginning of this function to check that its initial value is indeed eCSSUnit_Null, so that you'll be doing the right thing by leaving it untouched in all of these early-returns.)

Then you'd want to check for "Null unit" instead of "None unit" after all, in nsRuleNode.cpp's SetGridTrackList function.

>+CSSParserImpl::ParseGridTrackBreadth(nsCSSValue& aValue)
>+{
[...]
+    return true;
+  }
>+  if (!GetToken(true)) {
>+    return false;
>+  }
>+  if (!(eCSSToken_Dimension == mToken.mType &&
>+        mToken.mIdent.LowerCaseEqualsASCII("fr"))) {
>+    UngetToken();

Add a comment above the GetToken call here, to explain this chunk & logically separate it from the stuff above it. e.g.:
 // Attempt to parse a <flex> value (a number with 'fr' units)

>+CSSParserImpl::ParseGridTrackSize(nsCSSValue& aValue)
>+{
>+  if (ParseGridTrackBreadth(aValue) ||
>+      ParseVariant(aValue, VARIANT_AUTO, nullptr)) {
>+    return true;
>+  }
>+  if (!GetToken(true)) {
>+    return false;

Same here (though in this case it's "attempt to parse a minmax() function")

>+  nsCSSValue minSizingFunction;
>+  nsCSSValue maxSizingFunction;
>+  if (!(ParseGridTrackBreadth(minSizingFunction) &&
>+        ExpectSymbol(',', true) &&
>+        ParseGridTrackBreadth(maxSizingFunction) &&
>+        ExpectSymbol(')', true))) {
>+    SkipUntil(')');
>+    return false;
>+  }

Nit: It looks like this mandates that minmax() must have the close-paren, which I suspect is incorrect, per end of comment 10.

If I'm not missing something, could you fix this that so that e.g. "minmax(10px, 30px" is accepted? (and add a test)

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+CSSValue*
>+nsComputedDOMStyle::GetGridTrackSize(const nsStyleCoord& aMinValue,
>+                                     const nsStyleCoord& aMaxValue)
>+{
[...]
>+  nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>+  nsAutoString valStr, str;
>+  str.Append(valStr);
[...]
>+  str.Append(char16_t(')'));
>+  val->SetString(str);
>+  return val;

Variable names could be improved here; right now it's a bit confusing. (We have val, valStr, and str; I'd expect that valStr is what goes into val, but in fact it ends up being str that goes into val.)

Maybe name the strings 'str' and 'strComponent' or something like that? (where strComponent is the smaller one)

>+  // Drawing N tracks requires N+1 lines:
>+  // one before each track, plus one at the very end.
>+  MOZ_ASSERT(aTrackList.mLineNameLists.Length() == numSizes + 1,
>+             "Unexpected number of line name lists");

The comment is a bit confusing in this context, since this code isn't tied to drawing at all. (This is in a DOM getter.)

Maybe replace the first line with:
 // We should have one more grid-line than we have tracks:
?

>+++ b/layout/style/nsStyleStruct.cpp
>+  // See comments nsStyleStruct.h for more.

s/comments/comments in/
Blocks: 978478
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Comment on attachment 8383951 [details] [diff] [review]
> Patch 1 v2: Add the grid-template-{columns,rows} properties to the style
> system.
> 
> >+++ b/layout/style/nsCSSKeywordList.h
> [...]
> >+CSS_KEY(max-content, max_content)
> [...]
> >+CSS_KEY(min-content, min_content)
> 
> I don't think we want to add these unprefixed keywords as part of this patch.
> 
> We should be using -moz-max-content and -moz-min-content for now, since
> those are the versions of these keywords that we use elsewhere. When we
> decide to unprefix these keywords (or add unprefixed aliases), we can do
> that *globally* - we shouldn't use prefixed versions for some properties &
> unprefixed for others.

I disagree. Even though the keywords’ names are the same, this is a new feature and prefixing it would go against the decision to not expose new prefixed stuff to the Web.

This feature is independent from the one that uses the same keywords in the 'width' and related properties. They only share the concept of intrinsic sizes, which has been around ever since we’ve had floats and tables. The layout algorithm[1] to implement this is sepecific to CSS Grid, as grid sizing functions apply to a grid track rather than a single element.

Even if we were still using prefixes, IMO the decision to unprefix one of these two features would not necessarily imply that the other one is ready as well.

[1] http://dev.w3.org/csswg/css-grid/#resolve-intrinsic


> >--- a/layout/style/nsCSSParser.cpp
> >+const nsCSSProps::KTableValue kGridTemplateKTable[] = {
> 
> [...] it looks like this is only used as an parameter to ParseGridLineNames
> (and it's the only array that any clients pass to that method) so if we keep
> it, we should probably drop the parameter and declare it locally inside of
> ParseGridLineNames.

The intent is that the parsing code for the 'grid' shorthand will pass a different keyword array that additionally contains "row", "column", and "dense". I feel the whole thing is unnecessary (as the parens make parsing unambiguous), but if we’re gonna apply the letter of the spec on <custom-ident> we’ll need this parameter.

http://dev.w3.org/csswg/css-grid/#grid-shorthand
http://dev.w3.org/csswg/css-grid/#propdef-grid-auto-flow


> >+// http://dev.w3.org/csswg/css-values/#custom-idents
> >+// Parse an identifier that is not a CSS-wide keyword, nor "default",
> >+// nor one of the other keywords that can appear in the value of the property.
> >+bool
> >+CSSParserImpl::ParseCustomIdent(nsCSSValue& aValue,
> >+                                const nsAutoString& aIdentValue,
> >+                                const KTableValue* aPropertyKeywordTable)
> 
> This either should take an array of nsKeywords, per above, *or* (probably
> better) it should take an *actual* "KTableValue" keyword-table from
> nsCSSProps (where the even values are the keyword names you're interested
> in, and the odd values are #defines & should be skipped).
> 
> I think we should try to do the latter thing (take an actual keyword-table
> from nsCSSProps).
> 
> Currently, the grid clients would pass in kGridTrackBreadthKTable, and the
> counter client doesn't have a keyword-table that it could pass in, but maybe
> it should instead pass in VARIANT_NONE (in an additional aVariantMask
> parameter) which signals "this is another keyword, not in the table, which
> should be excluded"?
> 
> Your grid clients would probably want to pass in VARIANT_AUTO | VARIANT_NONE
> in that additional flags parameter, too, I think.

I went with arrays of nsKeywords. IMO an actual KTableValue is not appropriate here, as the add values would have to be dummy fillers.

kGridTemplateKTable is really not the same as kGridTrackBreadthKTable. The former is just a bunch of keywords to exclude, while the latter is a mapping of keywords to integers to be used in a nsCSSValue with a eCSSUnit_Enumerated unit. VARIANT_AUTO and VARIANT_NONE (meant to parse eCSSUnit_Auto and eCSSUnit_None) are also unrelated.

Independently of the above, they do not contain the same sets of keywords. I don’t think we want to add VARIANT_DENSE (for the grid shorthand) and the like to work around this.


> >+    // No list of names, same as an empty list of names
> >+    aValue.SetNoneValue();
> 
> A "none" nsCSSValue usually represents "the keyword 'none'", which isn't
> what you're actually trying to represent here. (Note that these properties
> *do actually* accept the value "none", so usages of eCSSUnit_None that don't
> actually represent that value are confusing.)

I went back and worth a few times between Null and None for this. Null seemed more "dangerous" since it has a very special meaning when used at the top-level of a property value which does not apply here, but made serialization easier as it serializes to the empty string. Until I refactored serialization and it wasn’t especially easier anymore.

I’ll go with Null if that’s what you prefer.


> >+  nsCSSValue minSizingFunction;
> >+  nsCSSValue maxSizingFunction;
> >+  if (!(ParseGridTrackBreadth(minSizingFunction) &&
> >+        ExpectSymbol(',', true) &&
> >+        ParseGridTrackBreadth(maxSizingFunction) &&
> >+        ExpectSymbol(')', true))) {
> >+    SkipUntil(')');
> >+    return false;
> >+  }
> 
> Nit: It looks like this mandates that minmax() must have the close-paren,
> which I suspect is incorrect, per end of comment 10.

I was gonna fix this, but it turns out ExpectSymbol() already handles this and returns true on EOF.


> e.g. "minmax(10px, 30px" is accepted? (and add a test)

I tried adding this test in other_values in property_database.js, but that breaks layout/style/test/test_value_cloning.html which creates stylesheets like this:

    sheet_data += "#test"+idx+" { ";
    sheet_data += current_item.prop + ": " + current_item.value;
    sheet_data += "}";

It seems we can not test parsing behavior at EOF with other_values, we’d need a more specific test.


> >+  // Drawing N tracks requires N+1 lines:
> >+  // one before each track, plus one at the very end.
> >+  MOZ_ASSERT(aTrackList.mLineNameLists.Length() == numSizes + 1,
> >+             "Unexpected number of line name lists");
> 
> The comment is a bit confusing in this context, since this code isn't tied
> to drawing at all. (This is in a DOM getter.)

Yes, "drawing" was bad wording here. Even in gfx code, grid lines are never rendered. I changed this to "delimiting N tracks…"


> I think other_values should include at least the following: [...]
> - possibly 'subgrid' (similarly commented-out with a bug number),

There is a proposal to punt subgrid to level 2. I seem to remember Microsoft agreeing to this, but I can’t find an official WG resolution.

http://lists.w3.org/Archives/Public/www-style/2014Feb/0047.html
Attachment #8383951 - Attachment is obsolete: true
Attachment #8383951 - Flags: review?(dholbert)
Attachment #8384177 - Flags: review?(dholbert)
Comment on attachment 8384177 [details] [diff] [review]
Patch 1 v3: Add the grid-template-{columns,rows} properties to the style system.

>From: Simon Sapin <simon.sapin@exyr.org>
>Bug 976787: Add the grid-template-{columns,rows} properties to the style system.
>
>
>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
>index 2b91d7a..122e2e2 100644
>--- a/layout/style/nsCSSKeywordList.h
>+++ b/layout/style/nsCSSKeywordList.h
>@@ -371,21 +371,23 @@ CSS_KEY(lowercase, lowercase)
> CSS_KEY(ltr, ltr)
> CSS_KEY(luminance, luminance)
> CSS_KEY(luminosity, luminosity)
> CSS_KEY(manual, manual)
> CSS_KEY(margin-box, margin_box)
> CSS_KEY(markers, markers)
> CSS_KEY(matrix, matrix)
> CSS_KEY(matrix3d, matrix3d)
>+CSS_KEY(max-content, max_content)
> CSS_KEY(medium, medium)
> CSS_KEY(menu, menu)
> CSS_KEY(menutext, menutext)
> CSS_KEY(message-box, message_box)
> CSS_KEY(middle, middle)
>+CSS_KEY(min-content, min_content)
> CSS_KEY(mix, mix)
> CSS_KEY(mm, mm)
> CSS_KEY(monospace, monospace)
> CSS_KEY(move, move)
> CSS_KEY(ms, ms)
> CSS_KEY(multiply, multiply)
> CSS_KEY(n-resize, n_resize)
> CSS_KEY(narrower, narrower)
>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>index 7b2a39f..9dba96c 100644
>--- a/layout/style/nsCSSParser.cpp
>+++ b/layout/style/nsCSSParser.cpp
>@@ -617,16 +617,23 @@ protected:
>   bool ParseCalcTerm(nsCSSValue& aValue, int32_t& aVariantMask);
>   bool RequireWhitespace();
> 
>   // For "flex" shorthand property, defined in CSS Flexbox spec
>   bool ParseFlex();
>   // For "flex-flow" shorthand property, defined in CSS Flexbox spec
>   bool ParseFlexFlow();
> 
>+  // CSS Grid
>+  bool ParseGridLineNames(nsCSSValue& aValue,
>+                          const nsCSSKeyword aPropertyKeyword[]);
>+  bool ParseGridTrackBreadth(nsCSSValue& aValue);
>+  bool ParseGridTrackSize(nsCSSValue& aValue);
>+  bool ParseGridTrackList(nsCSSProperty aPropID);
>+
>   // for 'clip' and '-moz-image-region'
>   bool ParseRect(nsCSSProperty aPropID);
>   bool ParseColumns();
>   bool ParseContent();
>   bool ParseCounterData(nsCSSProperty aPropID);
>   bool ParseCursor();
>   bool ParseFont();
>   bool ParseFontSynthesis(nsCSSValue& aValue);
>@@ -767,16 +774,19 @@ protected:
>                     int32_t aVariantMask,
>                     const KTableValue aKeywordTable[]);
>   bool ParseNonNegativeVariant(nsCSSValue& aValue,
>                                int32_t aVariantMask,
>                                const KTableValue aKeywordTable[]);
>   bool ParseOneOrLargerVariant(nsCSSValue& aValue,
>                                int32_t aVariantMask,
>                                const KTableValue aKeywordTable[]);
>+  bool ParseCustomIdent(nsCSSValue& aValue,
>+                        const nsAutoString& aIdentValue,
>+                        const nsCSSKeyword aPropertyKeyword[] = nullptr);
>   bool ParseCounter(nsCSSValue& aValue);
>   bool ParseAttr(nsCSSValue& aValue);
>   bool SetValueToURL(nsCSSValue& aValue, const nsString& aURL);
>   bool TranslateDimension(nsCSSValue& aValue, int32_t aVariantMask,
>                             float aNumber, const nsString& aUnit);
>   bool ParseImageOrientation(nsCSSValue& aAngle);
>   bool ParseImageRect(nsCSSValue& aImage);
>   bool ParseElement(nsCSSValue& aValue);
>@@ -6134,16 +6144,19 @@ CSSParserImpl::ParseVariant(nsCSSValue& aValue,
>           aValue.SetAutoValue();
>           return true;
>         }
>       }
>       if ((aVariantMask & VARIANT_INHERIT) != 0) {
>         // XXX Should we check IsParsingCompoundProperty, or do all
>         // callers handle it?  (Not all callers set it, though, since
>         // they want the quirks that are disabled by setting it.)
>+
>+        // IMPORTANT: If new keywords are added here,
>+        // they probably need to be added in ParseCustomIdent as well.
>         if (eCSSKeyword_inherit == keyword) {
>           aValue.SetInheritValue();
>           return true;
>         }
>         else if (eCSSKeyword_initial == keyword) {
>           aValue.SetInitialValue();
>           return true;
>         }
>@@ -6368,16 +6381,51 @@ CSSParserImpl::ParseVariant(nsCSSValue& aValue,
>     return ParseCalc(aValue, aVariantMask & VARIANT_LP);
>   }
> 
>   UngetToken();
>   return false;
> }
> 
> 
>+// http://dev.w3.org/csswg/css-values/#custom-idents
>+// Parse an identifier that is not a CSS-wide keyword, nor "default",
>+// nor one of the other keywords that can appear in the value of the property.
>+bool
>+CSSParserImpl::ParseCustomIdent(nsCSSValue& aValue,
>+                                const nsAutoString& aIdentValue,
>+                                const nsCSSKeyword aPropertyKeywordTable[])
>+{
>+  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(aIdentValue);
>+  if (keyword == eCSSKeyword_UNKNOWN) {
>+    // Fast path for identifiers that are not known CSS keywords:
>+    aValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
>+    return true;
>+  }
>+  if (keyword == eCSSKeyword_inherit ||
>+      keyword == eCSSKeyword_initial ||
>+      keyword == eCSSKeyword_unset ||
>+      keyword == eCSSKeyword_default) {
>+    return false;
>+  }
>+  if (aPropertyKeywordTable) {
>+    for (uint32_t i = 0;; i++) {
>+      nsCSSKeyword key = nsCSSKeyword(aPropertyKeywordTable[i]);
>+      if (key == eCSSKeyword_UNKNOWN) {
>+        break;
>+      }
>+      if (keyword == key) {
>+        return false;
>+      }
>+    }
>+  }
>+  aValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);
>+  return true;
>+}
>+
> bool
> CSSParserImpl::ParseCounter(nsCSSValue& aValue)
> {
>   nsCSSUnit unit = (mToken.mIdent.LowerCaseEqualsLiteral("counter") ?
>                     eCSSUnit_Counter : eCSSUnit_Counters);
> 
>   // A non-iterative for loop to break out when an error occurs.
>   for (;;) {
>@@ -6783,16 +6831,178 @@ CSSParserImpl::ParseFlexFlow()
> 
>   // Store these values and declare success!
>   for (size_t i = 0; i < numProps; i++) {
>     AppendValue(kFlexFlowSubprops[i], values[i]);
>   }
>   return true;
> }
> 
>+// Parse an optional <line-names> expression.
>+// If successful, sets aValue to a eCSSUnit_None for the empty list,
>+// or a eCSSUnit_List of eCSSUnit_Ident.
>+// Not finding an open paren is considered the same as an empty list.
>+
>+// aPropertyKeyword contains additional keywords for the 'grid' shorthand.
>+bool
>+CSSParserImpl::ParseGridLineNames(nsCSSValue& aValue,
>+                                  const nsCSSKeyword aPropertyKeyword[])

Two comments here:

(1) It seems like these GetToken/IsSymbol/Ident==mToken.mType checks before the loop are duplicating the same checks that are inside the loop.  Can they be merged?  (That requires rotating the contents of the loop, but that shouldn't be a problem since it's just a for (;;) loop.

It also seems like you have some bugs in one of the halves but not the other.  For example, if the first token after the ( is not an ident, you fail to call UngetToken on it (which will lead to incorrect matching if it's a '{' or '[').

(2) If ParseCustomIdent fails, you should UngetToken (technically, although really it's not needed, so you could just comment instead) and break rather than returning false.

>+  if (!GetToken(true)) {
>+    // Empty list of names (closed implicitly at EOF)
>+    return true;
>+  }
>+
>+  if (mToken.IsSymbol(')')) {
>+    // Empty list of names "()"
>+    return true;
>+  }
>+
>+  if (eCSSToken_Ident == mToken.mType) {
>+    nsCSSValueList* item = aValue.SetListValue();
>+    for (;;) {
>+      if (!ParseCustomIdent(item->mValue, mToken.mIdent, aPropertyKeyword)) {
>+        return false;
>+      }
>+      if (!GetToken(true) || mToken.IsSymbol(')')) {
>+        return true;
>+      }
>+      if (eCSSToken_Ident != mToken.mType) {
>+        UngetToken();
>+        break;
>+      }
>+      item->mNext = new nsCSSValueList;
>+      item = item->mNext;
>+    }
>+  }
>+  // Unexpected token inside ( ... )
>+  SkipUntil(')');
>+  return false;
>+}


>+bool
>+CSSParserImpl::ParseGridTrackList(nsCSSProperty aPropID)
>+{
>+  nsCSSValue value;
>+  if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
>+    // 'inherit', 'initial', 'unset' and 'none' must be alone
>+    if (!ExpectEndProperty()) {
>+      return false;
>+    }

No need to check ExpectEndProperty; the property parsing code will handle that for you.  (In general, a recursive descent parsing function checking for what it expects to be after it, rather than what it can consume, is a sign that it's written the wrong way.  It leads to problems when we try to add new things like @supports syntax or shorthands that contain other property values as components.)  I admit that sometimes it makes the code substantially easier, though.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #14)
> >+CSSParserImpl::ParseGridTrackList(nsCSSProperty aPropID)
> >+{
> >+  nsCSSValue value;
> >+  if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
> >+    // 'inherit', 'initial', 'unset' and 'none' must be alone
> >+    if (!ExpectEndProperty()) {
> >+      return false;
[...]
> No need to check ExpectEndProperty; the property parsing code will handle
> that for you.

Are you sure? Note that we have this in the "inherit" case for a bunch of other properties (shorthands like ParseBackground, longhands like ParseBackgroundRepeat). And as indicated in bug 940216, without this ExpectEndProperty(), we seem to give poorer error messages.
(In reply to Daniel Holbert [:dholbert] from comment #15)
> > No need to check ExpectEndProperty; the property parsing code will handle
> > that for you.
> 
> Are you sure? Note that we have this in the "inherit" case for a bunch of
> other properties (shorthands like ParseBackground, longhands like
> ParseBackgroundRepeat).

It was needed a long time ago, but it hasn't been for years.

> And as indicated in bug 940216, without this
> ExpectEndProperty(), we seem to give poorer error messages.

The error messages should be fixed; that should be quite simple.
(In reply to Simon Sapin (:SimonSapin) from comment #13)
> > We should be using -moz-max-content and -moz-min-content for now, since
> > those are the versions of these keywords that we use elsewhere. When we
> > decide to unprefix these keywords (or add unprefixed aliases), we can do
> > that *globally* - we shouldn't use prefixed versions for some properties &
> > unprefixed for others.
> 
> I disagree. Even though the keywords’ names are the same, this is a new
> feature and prefixing it would go against the decision to not expose new
> prefixed stuff to the Web.
> 
> This feature is independent from the one that uses the same keywords in the
> 'width' and related properties.

I'm torn on this. I do agree that it's confusing for authors to specify a prefixed "-moz-min-content" in the context of an unprefixed "grid" syntax.  And yes, min-content means something different here than it does in 'width'.

However, despite the semantic difference, it's still the same keyword, and it seems very strange indeed for us to require "-moz-min-content" for one set of properties vs. "min-content" for another set of properties (particularly if each property accepts *exactly one* of those -- i.e. they're not even aliases).

So I'm still leaning slightly towards using the existing (prefixed) versions, for consistency, and perhaps pushing to support unprefixed min-content/max-content in the near term. But I'll defer to dbaron's opinion. dbaron, what do you think?

> The intent is that the parsing code for the 'grid' shorthand will pass a
> different keyword array that additionally contains "row", "column", and
> "dense".

Hmm. This makes custom-ident parsing a bit annoying (in that we need to support multiple blacklists), for any properties that are part of a shorthand.

I posted to www-style, to confirm whether we really do need to blacklist these extra values in the shorthand:
 http://lists.w3.org/Archives/Public/www-style/2014Mar/0011.html

> I’ll go with Null if that’s what you prefer.

I do prefer "Null". I wouldn't worry about the special meaning it has as a top-level nsCSSValue, since this isn't at the top level.  eCSSUnit_Null just represents "uninitialized nsCSSValue" (no unit), and that's more appropriate representation for [empty list] than "none" (the CSS keyword).

> > I think other_values should include at least the following: [...]
> > - possibly 'subgrid' (similarly commented-out with a bug number),
> 
> There is a proposal to punt subgrid to level 2. I seem to remember Microsoft
> agreeing to this, but I can’t find an official WG resolution.
> 
> http://lists.w3.org/Archives/Public/www-style/2014Feb/0047.html

Looks like fantasai disagrees about punting, per:
 http://lists.w3.org/Archives/Public/www-style/2014Mar/0006.html

Either way, looks like we're not adding support for it in this bug.  Still, it won't hurt to include a (commented-out) line in invalid_values in property_database.js, using it as a <custom-ident>, with a comment saying we don't yet support it but that it should be considered invalid as a <custom-ident> when we do add support.
[needinfo=dbaron on the min-content vs. -moz-min-content thing at the beginning of comment 17.]
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #14)
> (1) It seems like these GetToken/IsSymbol/Ident==mToken.mType checks before
> the loop are duplicating the same checks that are inside the loop.  Can they
> be merged?

I did merge the Ident==mToken.mType check, but the rest is necessary because aValue.SetListValue() should only be called for a non-empty list of names.


(In reply to Daniel Holbert [:dholbert] from comment #17)
> Either way, looks like we're not adding support for subgrid in this bug.
> Still, it won't hurt to include a (commented-out) line in invalid_values in
> property_database.js, using it as a <custom-ident>, with a comment saying we
> don't yet support it but that it should be considered invalid as a
> <custom-ident> when we do add support.

I actually added "subgrid" to the array of excluded keywords for <custom-ident>, and a test for it.

I considered adding style system support for subgrid, but it’s not obvious what data strucutures to use for it in specified or computed values.
Attachment #8384177 - Attachment is obsolete: true
Attachment #8384177 - Flags: review?(dholbert)
Attachment #8384584 - Flags: review?(dholbert)
Compared to v4: Use eCSSUnit_Function instead of eCSSUnit_Pair for minmax(), which removes one special case in serialization.
Attachment #8384584 - Attachment is obsolete: true
Attachment #8384584 - Flags: review?(dholbert)
Attachment #8384679 - Flags: review?(dholbert)
Tweak naming.
Attachment #8384679 - Attachment is obsolete: true
Attachment #8384679 - Flags: review?(dholbert)
Attachment #8385450 - Flags: review?(dholbert)
Tweak naming and remove an obsolete comment.
Attachment #8384600 - Attachment is obsolete: true
Attachment #8384600 - Flags: review?(dholbert)
Attachment #8385453 - Flags: review?(dholbert)
Update nsStylePosition::CalcDifference() for the new properties.
Attachment #8384709 - Attachment is obsolete: true
Attachment #8384709 - Flags: review?(dholbert)
Attachment #8385456 - Flags: review?(dholbert)
Update nsStylePosition::CalcDifference() for the new properties.
Attachment #8384790 - Attachment is obsolete: true
Attachment #8384790 - Flags: review?(dholbert)
Attachment #8385458 - Flags: review?(dholbert)
This patch adds the remaining longhand properties defined in CSS Grid, and should be the last for this bug. Shorthand properties will go in another bug.
Attachment #8385462 - Flags: review?(dholbert)
Summary: Add style-system support for grid container CSS properties → Add style-system support for CSS Grid longhand properties
Comment on attachment 8385450 [details] [diff] [review]
Patch 1 v6: Add the grid-template-{columns,rows} properties to the style system.

>+++ b/layout/style/nsCSSParser.cpp
>+// Parse an optional <line-names> expression.
>+// If successful, sets aValue to a eCSSUnit_None for the empty list,
>+// or a eCSSUnit_List of eCSSUnit_Ident.
>+// Not finding an open paren is considered the same as an empty list.
>+
>+// aPropertyKeywords contains additional keywords for the 'grid' shorthand.
>+bool
>+CSSParserImpl::ParseGridLineNames(nsCSSValue& aValue,
>+                                  const nsCSSKeyword aPropertyKeywords[])
>+{
>+  MOZ_ASSERT(aValue.GetUnit() == eCSSUnit_Null,
>+             "Unexpected unit, aValue should not be initialized yet");
>+  if (!GetToken(true)) {
>+    return true;
>+  }

The comment needs updating:
 s/sets aValue to a eCSSUnit_None/leaves aValue with eCSSUnit_Null/

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+nsComputedDOMStyle::GetGridLineNames(const nsTArray<nsString>& aLineNames)
>+{
[...]
>+}
>+
>+
>+CSSValue*
>+nsComputedDOMStyle::GetGridTrackSize(const nsStyleCoord& aMinValue,

nit: Drop one of the blank lines between those functions.

>+CSSValue*
>+nsComputedDOMStyle::GetGridTrackList(const nsStyleGridTrackList& aTrackList)
>+{
>+  // An empty <track-list> is represented as "none" in syntax.
>+  uint32_t numSizes = aTrackList.mMinTrackSizingFunctions.Length();
>+  MOZ_ASSERT(aTrackList.mMaxTrackSizingFunctions.Length() == numSizes,
>+             "Different number of min and max track sizing functions");
>+  if (numSizes == 0) {
>+    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
>+    val->SetIdent(eCSSKeyword_none);
>+    return val;
>+  }

Nit: the comment there should probably move down a few lines, to be right above or below the "== 0" check, so that it's closer to the actual "eCSSKeyword_none" usage that it's explaining.

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>index 8e722fa..db6dbee 100644
>--- a/layout/style/nsStyleStruct.h
>+++ b/layout/style/nsStyleStruct.h
>@@ -1147,16 +1147,49 @@ struct nsStyleList {
>   uint8_t   mListStylePosition;         // [inherited]
> private:
>   nsRefPtr<imgRequestProxy> mListStyleImage; // [inherited]
>   nsStyleList& operator=(const nsStyleList& aOther) MOZ_DELETE;
> public:
>   nsRect        mImageRegion;           // [inherited] the rect to use within an image
> };
> 
>+
>+struct nsStyleGridTrackList {

Nit: Don't add an extra blank line above 'struct' there.

>+  //   The units for nsStyleCoord are:
>+  //   * eStyleUnit_Percent represents a <percentage>
>+  //   * eStyleUnit_Factor represents a <flex> flexible fraction
>+  //   * eStyleUnit_Coord represents a <length>
>+  //   * eStyleUnit_Enumerated represents min-content or max-content

Comment needs updating:
 s/Factor/FlexFraction/

I'm holding off on final r+ on patch 1, pending:
 - dbaron's thoughts on min-content vs -moz-min-content, per comment 18. [& potential resulting patch changes]
 - Result of CSSWG discussion on what are valid <custom-ident> values. (which would mean we wouldn't need to exclude any keywords there anymore, which would mean that ParseCustomIdent() and the "subgrid" keyword should no longer be defined in this patch)
Comment on attachment 8385453 [details] [diff] [review]
Patch 2 v2: Add the grid-template-areas property to the style system.

>+++ b/layout/style/nsCSSParser.cpp
>+  bool ParseGridTemplateAreasLine(const nsAutoString& input,
>+                                  nsTArray<nsCSSGridNamedArea>& namedAreas,
>+                                  uint32_t row,
>+                                  uint32_t& columns);
[...]
>+bool
>+CSSParserImpl::ParseGridTemplateAreasLine(const nsAutoString& input,
>+                                          nsTArray<nsCSSGridNamedArea>& namedAreas,
>+                                          uint32_t row,
>+                                          uint32_t& columns)
>+{

Rename args to be like "aFoo" instead of "foo".

(in both function-decl & function-definition)

>+  uint32_t column = 0;
>+  nsCSSGridNamedArea* currentArea = nullptr;
>+  while (scanner.Next(token)) {
>+    if (token.isTrash) {
>+      return false;
>+    }
>+    column++;  // Need to happen before any "continue".

It looks like this column++ makes all of the column-indexing 1-based, since we increment before we've ever used 'column').

Is that intentional? (I'm guessing so, since it looks like the row-indexing is also 1-based?)  If so, it's a bit unclear, because right now, "column = 0" makes it look like it's intending to be 0-based.

IMHO it would be cleaner if we declared "column = [whatever its base value is]", moved the column++ to the end of the loop, and either rewrite the logic to avoid needing 'continue', or just add a duplicate 'column++' right before the continue.  (I'd prefer to do away with the continue -- I think you can get rid of it if you just replace the "!token.mName.IsEmpty()" check with "!currentArea && !token.mName.IsEmpty()". I'd marginally prefer that over the current flow, I think.)

>+    if (currentArea) {
>+      if (token.mName == currentArea->mName) {
>+        if (currentArea->mRowStart == row) {
>+          // Next column in the first row of this name area.

s/name area/named area/

>+      if (currentArea->mColumnEnd != column) {

I didn't initially understand what this clause was about. Add a comment above the "if" line to make it clearer - something like:
 // We're exiting |currentArea|, so currentArea is ending at |column|.
 // Make sure that this is consistent with currentArea on previous rows:

>+    if (!token.mName.IsEmpty()) {
>+      // Named cell that doesnât have a cell with the same name on its left.
                                ^^^
Nit: non-ASCII single-quote character. I count four of these in this patch. Please replace with normal single-quote, and fix your editor to not insert these. :)

Probably best to just open the patch file itself and do a global search-and-replace. Best to do this for all of your patches, to avoid bitrotting them and in case they introduce any additional fancy-quote characters.

>+      for (uint32_t i = 0, end = namedAreas.Length(); i < end; i++) {
>+        if (namedAreas[i].mName == token.mName) {

Add a comment above this loop saying "// See if it belongs to an area defined in an earlier row" or something like that.

>+      if (!currentArea) {
>+        // New named area
>+        currentArea = namedAreas.AppendElement();
>+        currentArea->mName.Assign(token.mName);

Maybe use operator= instead of Assign() here, to make things a bit less abstract?

>+++ b/layout/style/nsCSSPropList.h
>@@ -2004,16 +2004,28 @@ CSS_PROP_UIRESET(
>     CSS_PROPERTY_PARSE_VALUE |
>         CSS_PROPERTY_VALUE_NONNEGATIVE,
>     "",
>     VARIANT_HI,
>     nullptr,
>     CSS_PROP_NO_OFFSET,
>     eStyleAnimType_None) // bug 58646
> CSS_PROP_POSITION(
>+    grid-template-areas,
>+    grid_template_areas,
>+    GridTemplateAreas,
>+    CSS_PROPERTY_PARSE_FUNCTION |
>+        CSS_PROPERTY_STORES_CALC |
>+        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH,

STORES_CALC and LAYOUT_FLUSH aren't needed here.

(This property does not store calc() values, and it doesn't need layout information in getComputedStyle().)

NOTE: This also applies to the properties in Patch 4 and Patch 5, too -- those currently have STORES_CALC and NEEDS_LAYOUT_FLUSH but don't need them.

>+nsCSSGridTemplateAreaScanner::Next(nsCSSGridTemplateAreaToken& aTokenResult)
>+{
>+  int32_t ch;
>+  // Skip whitespace
>+  do {
>+    if (mOffset >= mCount) {
>+      return false;
>+    }
>+    ch = mBuffer[mOffset];
>+    mOffset++;
>+  } while (IsWhitespace(ch));
>+
>+  if (IsOpenCharClass(ch, IS_IDCHAR)) {
>+    uint32_t start = mOffset - 1;

Add a comment at the end of that last line, saying something like:
  // offset of |ch|

(Otherwise, it's a bit unclear why we're stepping backwards here. A comment [plus a skim of the above code] makes it clearer.)

>+  } else if (ch == '.') {
>+    aTokenResult.mName.Truncate();
>+    aTokenResult.isTrash = false;

Maybe add "// null cell" at the end of the first or second line there?

>diff --git a/layout/style/nsCSSScanner.h b/layout/style/nsCSSScanner.h
>+// Scanner for the grid-template-areas micro-syntax
>+// http://dev.w3.org/csswg/css-grid/#propdef-grid-template-areas
>+struct nsCSSGridTemplateAreaToken {
>+  nsAutoString mName;  // Empty for a null cell, non-empty for a named cell
>+  bool isTrash;  // True for a trash token, mName is ignored in this case.
>+};
>+
>+class nsCSSGridTemplateAreaScanner {
>+  public:
>+  nsCSSGridTemplateAreaScanner(const nsAString& aBuffer);

Seems like the "// Scanner for..." comment should be adjacent to nsCSSGridTemplateAreaScanner, not nsCSSGridTemplateAreaToken.  Right now, it looks like you're documenting that nsCSSGridTemplateAreaToken is the scanner, and nsCSSGridTemplateAreaScanner is unlabeled.

Also: nsAutoString is only a good member-var type in exclusively *stack-allocated* structs. Fortunately, this struct does seem to be exclusively stack-allocated.  So, declare it as "struct MOZ_STACK_CLASS nsCSSGridTemplateAreaToken", to enforce this (and to make it clearer why the nsAutoString usage is OK).

Also: de-indent "public:" in nsCSSGridTemplateAreaScanner.

>+  // Get the next token.  Return false on EOF.
>+  // aTokenResult is filled in with the data for the token.
>+  bool Next(nsCSSGridTemplateAreaToken& aTokenResult);
>+
>+  const char16_t *mBuffer;
>+  uint32_t mOffset;
>+  uint32_t mCount;

Add a 'private:' header above the member variables there, since it looks like no one outside of this class needs to access those.

>diff --git a/layout/style/nsCSSValue.cpp b/layout/style/nsCSSValue.cpp
>+size_t
>+nsCSSValueGridTemplateAreas::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
>+{
>+  size_t n = mNamedAreas.SizeOfExcludingThis(aMallocSizeOf);
>+  n += mTemplates.SizeOfIncludingThis(aMallocSizeOf);
>+  return n;
>+}
>+
>diff --git a/layout/style/nsCSSValue.h b/layout/style/nsCSSValue.h

Nit: This patch is adding an unnecessary blank line at the end of nsCSSValue.cpp (beyond the customary newline).  Revert that.

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+CSSValue*
>+nsComputedDOMStyle::DoGetGridTemplateAreas()
>+{
>+  const nsTArray<nsString>& templates =
>+    StylePosition()->mGridTemplateAreas.mTemplates;
>+  uint32_t length = templates.Length();
>+  if (length == 0) {
>+    nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>+    val->SetIdent(eCSSKeyword_none);
>+    return val;
>+  } else {

Drop the 'else' (not needed after return), and deindent its body.

>+    nsDOMCSSValueList *valueList = GetROCSSValueList(false);
>+    for (uint32_t i = 0; i < length; i++) {
>+      nsString str;
>+      nsStyleUtil::AppendEscapedCSSString(templates[i], str);
>+      nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>+      val->SetString(str);

s/nsString/nsAutoString/

Also: consider dropping |length|, and replaicing the length == 0 check with "templates.IsEmpty()", and the i < length check with i < templates.Length(). (IsEmpty() is generally preferred over Length() == 0, and we usually don't bother factoring Length() calls out of the loop condition, since it's cheap.)

>+static void
>+SetGridTemplateAreas(const nsCSSValue& aValue,
>+                     nsCSSValueGridTemplateAreas& aResult,
>+                     const nsCSSValueGridTemplateAreas& aParentValue,
>+                     bool& aCanStoreInRuleTree)
>+
>+{

Drop blank line before "{"

>+  switch (aValue.GetUnit()) {
>+  case eCSSUnit_Null:
>+    break;
>+
>+  case eCSSUnit_Inherit:
>+    aCanStoreInRuleTree = false;
>+    aResult.mNamedAreas.Clear();
>+    aResult.mNamedAreas.AppendElements(aParentValue.mNamedAreas);
>+    aResult.mTemplates.Clear();
>+    aResult.mTemplates.AppendElements(aParentValue.mTemplates);
>+    break;

Replace Clear() / AppendElements() with "=".

(Same for the "default" case, a bit lower down)

>+++ b/layout/style/nsStyleStruct.cpp
>-  // mGridTemplateRows and mGridTemplateColumns get their default constructors
>-  // which initializes them to empty arrays,
>-  // which represent the propertyâs initial value 'none'
>+  // mGridTemplateRows, mGridTemplateColumns, and mGridTemplateAreas
>+  // get their default constructors
>+  // which initialize them to empty arrays,
>+  // which represent the propertiesâ initial value 'none'
>+  // represent 'none'.

The last line there looks like left-over cruft -- delete it.

>+++ b/layout/style/test/property_database.js
>+		other_values: [
>+			"''",
>+			"'' ''",
>+			"'1a-é_ .' 'b .'",
>+			"' Z\t\\aZ' 'Z Z'",
>+		],
>+		invalid_values: [
>+			"'a/b'",
>+			"'a . a'",
>+			"'. a a' 'a a a'",
>+			"'a a .' 'a a a'",
>+			"'a a' 'a .'",
>+			"'a a' '. .' 'a a'",
>+		]

These are <string>, which allow double-quote delimiters per http://dev.w3.org/csswg/css-values-3/#string-value . But this doesn't currently test double-quote characters.

Please add some entries to other_values with double-quotes, and some entries to invalid_values with unmatched quote characters (starting with single-quote and ending with double quote, & vice versa) (I assume that's invalid)

Also: invalid_values should include an otherwise-valid value, with a "trash token" that makes it invalid.

Also: other_values should include an entry with a newline character between quoted strings, to make sure we treat that as valid. (From the spec, it looks like that's expected to be a common use-case, so we should be extra-sure that it works & that we don't break it without noticing.)

Also: What's supposed to happen if we have "." adjacent to another character? (two examples: "a .. b" and "a. .b")  From looking at the spec, I think we're supposed to behave the same as if there were whitespace around each period, right? (i.e. both of the above values are equivalent to "a . . b") Or maybe it's invalid. Either way, could you include some values like that, in either other_values or invalid_values?

>+++ b/layout/style/test/test_initial_storage.html

As discussed earlier in #layout, I split out the changes to this file into a separate patch & pushed them as a "(no bug)" cset, since they're minimal & not related to the rest of this patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f80242a96ad2
Comment on attachment 8385450 [details] [diff] [review]
Patch 1 v6: Add the grid-template-{columns,rows} properties to the style system.

Sorry, one more thing I just noticed in Patch 1:

>+++ b/layout/style/nsCSSValue.cpp
>@@ -1002,16 +1002,22 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
>+    case eCSSProperty_grid_template_columns:
>+    case eCSSProperty_grid_template_rows:
>+      AppendASCIItoUTF16(nsCSSProps::LookupPropertyValue(aProperty, intValue),
>+                         aResult);
>+      break;

This looks the same as the existing "default" case:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp?rev=b3a9c9540bfc#1017

So, can we drop this chunk and just fall into the "default" case here?
Comment on attachment 8385456 [details] [diff] [review]
Patch 3 v2: Add the grid-auto-{columns,rows} properties to the style system.

>+++ b/layout/style/nsCSSValue.cpp
>@@ -1020,16 +1020,18 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
>+    case eCSSProperty_grid_auto_columns:
>+    case eCSSProperty_grid_auto_rows:
>     case eCSSProperty_grid_template_columns:
>     case eCSSProperty_grid_template_rows:
>       AppendASCIItoUTF16(nsCSSProps::LookupPropertyValue(aProperty, intValue),
>                          aResult);
>       break;

Per comment 31, I don't think we need this, since the existing default case does the same thing.

>+++ b/layout/style/nsRuleNode.cpp
>+SetGridTrackSize(const nsCSSValue& aValue,
>+                 nsStyleCoord& aResultMin,
>+                 nsStyleCoord& aResultMax,
>+                 nsStyleContext* aStyleContext,
>+                 nsPresContext* aPresContext,
>+                 bool& aCanStoreInRuleTree)
>+{
>+  if (aValue.GetUnit() == eCSSUnit_Function) {
>+    // A minmax() function.
>+    nsCSSValue::Array* func = aValue.GetArrayValue();
>+    NS_ASSERTION(func->Item(0).GetKeywordValue() == eCSSKeyword_minmax,
>+                 "Expected minmax(), got another function name.");

Nit: assertion messages shouldn't have a period at the end, per comment 9.

>+  } else if (aValue.GetUnit() == eCSSUnit_Auto) {
>+    // 'auto' computes to 'minmax(min-content, max-content)'
>+    aResultMin.SetIntValue(NS_STYLE_GRID_TRACK_BREADTH_MIN_CONTENT,
>+                           eStyleUnit_Enumerated);
>+    aResultMax.SetIntValue(NS_STYLE_GRID_TRACK_BREADTH_MAX_CONTENT,
>+                           eStyleUnit_Enumerated);

No need to change this, but [observation] it does feel a bit wasteful that we need two nsStyleCoords (min & max) for each of these properties, on every nsStylePosition, when the common case will just be "auto" when this is unset, or even a single value ("10px") when it's set.

Seems like explicit "minmax" (with 2 different values) is really the uncommon case, so I suspect it'd be better to use a single nsStyleCoord, which could support a new "MinMax" unit with some dynamic allocation (sort of like the existing Calc unit) when minmax is explicitly provided.  And we could optimize the extremely common case ("auto") to use eStyleUnit_Auto, so that it wouldn't need to use that new unit & dynamic allocation.

(We'd do something similar for grid-template-rows/grid-template-columns, for symmetry & to get the same space-savings there.)

I'm curious what you think of this suggestion.  But in any case, I think the structure you've got is good for now, and we could potentially do this fixup in a followup bug.

>+static void
>+SetGridAutoColumnsRows(const nsCSSValue& aValue,
>+                       nsStyleCoord& aResultMin,
>+                       nsStyleCoord& aResultMax,
>+                       const nsStyleCoord& aParentValueMin,
>+                       const nsStyleCoord& aParentValueMax,
>+                       nsStyleContext* aStyleContext,
>+                       nsPresContext* aPresContext,
>+                       bool& aCanStoreInRuleTree)
>+
>+{
>+  case eCSSUnit_Initial:
>+  case eCSSUnit_Unset:
>+    // 'auto' computes to 'minmax(min-content, max-content)'

This comment is a bit confusing, since it's talking about 'auto' but there's no mention of "auto" in the code here.

Extend the comment with something like:
    // Initial value is 'auto', which computes to [...]
    // (Explicitly-specified 'auto' values are handled below, in default case.)


>+++ b/layout/style/nsStyleStruct.cpp
>@@ -1223,16 +1223,25 @@ nsStylePosition::nsStylePosition(void)
>   mFlexBasis.SetAutoValue();
>+  // 'auto' computes to 'minmax(min-content, max-content)'
>+  mGridAutoColumnsMin.SetIntValue(NS_STYLE_GRID_TRACK_BREADTH_MIN_CONTENT,
>+                                  eStyleUnit_Enumerated);
>+  mGridAutoColumnsMax.SetIntValue(NS_STYLE_GRID_TRACK_BREADTH_MAX_CONTENT,
>+                                  eStyleUnit_Enumerated);
>+  mGridAutoRowsMin.SetIntValue(NS_STYLE_GRID_TRACK_BREADTH_MIN_CONTENT,
>+                               eStyleUnit_Enumerated);
>+  mGridAutoRowsMax.SetIntValue(NS_STYLE_GRID_TRACK_BREADTH_MAX_CONTENT,
>+                               eStyleUnit_Enumerated);

Similar to above, this comment is a bit confusing, since there's no "auto" in the code nearby. (Actually there is, but it's on the previous line and it's not the code that the comment is trying to explain).

Mention the property-names in the comment here, to make that clearer.

r=me with the above addressed.
Attachment #8385456 - Flags: review?(dholbert) → review+
Comment on attachment 8385458 [details] [diff] [review]
Patch 4 v2: Add the grid-auto-flow property to the style system.

>+++ b/layout/style/nsCSSParser.cpp
>@@ -6837,16 +6838,65 @@ CSSParserImpl::ParseFlexFlow()
>   return true;
> }
> 
>+
>+bool
>+CSSParserImpl::ParseGridAutoFlow()
>+{

Drop the extra blank line before this function.

>+  nsCSSValue value;
>+  if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
>+    AppendValue(eCSSProperty_grid_auto_flow, value);
>+    return true;
>+  }
>+
>+  bool dense = false;
>+  bool column = false;
>+  bool row = false;

Maybe name these gotDense, gotColumn, gotRow?

For me at least, that makes subsequent code like...
  } else if (keyword == eCSSKeyword_column && !gotColumn && !gotRow) {
    gotColumn = true
...easier to grok than the current 'column' / 'row' equivalent.

This is fine as-is too, though. (But, see below about bitfield stuff)

>+++ b/layout/style/nsCSSPropList.h
> CSS_PROP_POSITION(
>+    grid-auto-flow,
>+    grid_auto_flow,
>+    GridAutoFlow,
>+    CSS_PROPERTY_PARSE_FUNCTION |
>+        CSS_PROPERTY_STORES_CALC |
>+        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH,

As noted in comment 30, STORES_CALC and LAYOUT_FLUSH aren't needed here.

>+++ b/layout/style/nsCSSValue.cpp
>@@ -1020,16 +1020,30 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
> 
>     case eCSSProperty_font_variant_numeric:
>       nsStyleUtil::AppendBitmaskCSSValue(aProperty, intValue,
>                                          NS_FONT_VARIANT_NUMERIC_LINING,
>                                          NS_FONT_VARIANT_NUMERIC_ORDINAL,
>                                          aResult);
>       break;
> 
>+    case eCSSProperty_grid_auto_flow:
>+      if (intValue == NS_STYLE_GRID_AUTO_FLOW_COLUMN) {
>+        aResult.AppendLiteral("column");
>+      } else if (intValue == NS_STYLE_GRID_AUTO_FLOW_ROW) {
>+        aResult.AppendLiteral("row");
>+      } else if (intValue == NS_STYLE_GRID_AUTO_FLOW_COLUMN_DENSE) {
>+        aResult.AppendLiteral("column dense");
>+      } else if (intValue == NS_STYLE_GRID_AUTO_FLOW_ROW_DENSE) {
>+        aResult.AppendLiteral("row dense");
>+      } else {
>+        NS_ASSERTION(false, "Unexpected specified value for grid-auto-flow");
>+      }
>+      break;

Rather than explicitly enumerating all the possible values as different enums, I think this property should be encoded as a bitfield. That's what we seem to do for other properties, at least, and it lets you use AppendBitmaskCSSValue() to share code for converting to a string, both here and in nsComputeDOMStyle.

I'll jump to nsStyleConsts.h, since that's where you'd define the enum values:

>+++ b/layout/style/nsStyleConsts.h
>+// grid-auto-flow values (do not map not 1:1 to keywords)
>+#define NS_STYLE_GRID_AUTO_FLOW_NONE            0x00
>+#define NS_STYLE_GRID_AUTO_FLOW_COLUMN          0x01
>+#define NS_STYLE_GRID_AUTO_FLOW_ROW             0x02
>+#define NS_STYLE_GRID_AUTO_FLOW_COLUMN_DENSE    0x11
>+#define NS_STYLE_GRID_AUTO_FLOW_ROW_DENSE       0x12

Instead of the above, I think you want something like:

 // grid-auto-flow bitfield values
 // (Note: _NONE should only ever be set on its own,
 // and _COLUMN and _ROW should never be set simultaneously.)
 #define NS_STYLE_GRID_AUTO_FLOW_NONE             (1 << 0)
 #define NS_STYLE_GRID_AUTO_FLOW_COLUMN           (1 << 1)
 #define NS_STYLE_GRID_AUTO_FLOW_ROW              (1 << 2)
 #define NS_STYLE_GRID_AUTO_FLOW_DENSE            (1 << 3)

...and then you can logical-or those bits together as-appropriate in the parsing function to encode an integer in a nsCSSValue, and that encoded value will stringify nicely using AppendBitmaskCSSValue.  (For that to work, though, you'll need to provide a nsCSSProps keyword-table to map the bits to their corresponding eCSSKeywords, and you'll need to mention the keyword table's name in the CSS_PROP_POSITION() block of nsCSSPropList.h.)

(Note: of course in practice, not all of the possible bitfield values will be used, because we know that "none" will only appear alone, and  "row" and "column" are exclusive. We can enforce this at parse time, and then we can trust that it's true (with assertions anywhere we care to check) after that.)

>+++ b/layout/style/nsStyleStruct.h
>+  // XXX should mGridAutoFlow be an actual `enum` type?
>+  uint8_t       mGridAutoFlow;          // [reset] enumerated
>   uint8_t       mBoxSizing;             // [reset] see nsStyleConsts.h
>   uint8_t       mAlignContent;          // [reset] see nsStyleConsts.h

You can drop the XXX comment; we use uint8_t for enum values in nsStyleStruct.
(Probably should add a "see nsStyleConsts.h" comment, for consistency -- the idea being, see that file for the various values [bitfield values, now] that this member-var take on.)


>+++ b/layout/style/test/property_database.js
>+		invalid_values: [
>+			"",
>+			"column row",
>+			"dense row dense",
>+		]

Add "none row" and "none dense" as invalid_values, just to make sure we only accept "none" by itself.

Maybe throw "auto" and "10px" (or some other length) in there as invalid_values, too, for good measure.

> 	gCSSProperties["grid-auto-columns"] = {
[...]
>-	}
>+	};

This final change looks like it's fixing something that was broken in the previous ("grid-auto-columns") patch. Probably best to just fix that in the grid-auto-columns patch, and drop this chunk in this patch.
Comment on attachment 8385462 [details] [diff] [review]
Path 5: Add grid-{column,row}-{start,end} and grid-auto-position to the style system.

>+++ b/layout/style/nsCSSParser.cpp
>+CSSParserImpl::ParseGridLine(nsCSSValue& aValue)
>+{
>+  //  <grid-line> =
>+  //    auto |
>+  //    <custom-ident> |
>+  //    [ <integer> && <custom-ident>? ] |
>+  //    [ span && [ <integer> || <custom-ident> ] ]
>+  //
>+  // Syntaxically, this simplifies to:

s/Syntaxically/Syntactically/

>+  // A zero <integer> is invalid.
>+  // Given "span", a negative <integer> is invalid.
>+  // Require at least one of <ident> or <custom-ident>

I think you mean s/<ident>/<integer>/

>+  if (hasInteger ? (hasSpan ? (integer <= 0) : (integer == 0))
>+                 : !hasIdent) {
>+    return false;
>+  }

The nested ternary operator makes this unnecessarily complex & hard to read, IMHO.

Could you split this into normal "if" checks, with 3 separate return statements for the 3 conditions you describe in your 3-line comment above?
(This will make it more readable & also will make the control flow easier to follow in a debugger).

e.g.
     // Reject explicitly-invalid combinations:
     if (hasInteger) {
       // A zero <integer> is invalid.
       if (integer == 0) {
          return false;
       }
       // Given 'span', a [...etc...] 
       if (hasSpan && integer < 0) {
 etc. etc.

This might be slightly less efficient than what you've got (e.g. we might check hasSpan twice), but not by enough to make any noticeable difference.

>+bool
>+CSSParserImpl::ParseGridAutoPosition() {

Bump the "{" to next line

>+bool
>+CSSParserImpl::ParseGridColumnRowStartEnd(nsCSSProperty aPropID)

(Maybe this should have "Or" in the name, like "ParseGridColumnOrRowStartOrEnd"? Makes a bit more sense to me... This is fine, too, though.)
(If you do feel like changing it, the same applies to a similarly-named function in an earlier patch here.)

>+++ b/layout/style/nsCSSPropList.h
> CSS_PROP_POSITION(
>+    grid-auto-position,
>+    grid_auto_position,
>+    GridAutoPosition,
>+    CSS_PROPERTY_PARSE_FUNCTION |
>+        CSS_PROPERTY_STORES_CALC |
>+        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH,

(per comment 30): none of this patch's properties need _CALC or _LAYOUT_FLUSH here.

Also: this file is approximately kept in alphabetical order, but this ^ "grid-auto-position" property is currently separated a bit from the other "grid-auto-*" properties, which seems odd. Maybe bump it up next to them? (not necessarily to its exact alphabetical position, which would split up "grid-auto-columns" and "grid-auto-rows", but adjacent to them at least)

>+++ b/layout/style/nsCSSValue.cpp
>@@ -1042,16 +1042,24 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
>+    case eCSSProperty_grid_auto_position:
>+    case eCSSProperty_grid_column_start:
>+    case eCSSProperty_grid_column_end:
>+    case eCSSProperty_grid_row_start:
>+    case eCSSProperty_grid_row_end:
>+      aResult.AppendLiteral("span");
>+      break;

Add a brief comment here to explain that "span" is the *only* enumerated-unit value for these properties, which is why we can safely append it here.
(Maybe assert that our actual enumerated value is the dummy "1" value you put in, too? or maybe it doesn't matter.)

>+++ b/layout/style/nsComputedDOMStyle.cpp
> CSSValue*
>+nsComputedDOMStyle::GetGridLine(const nsStyleGridLine& aGridLine)
>+{
[...]
>+  if (aGridLine.mHasSpan) {
>+    nsROCSSPrimitiveValue* span = new nsROCSSPrimitiveValue;
>+    span->SetIdent(eCSSKeyword_span);
>+    valueList->AppendCSSValue(span);
>+  }
>+
>+  if (aGridLine.mInteger != 0) {
>+    nsROCSSPrimitiveValue* integer = new nsROCSSPrimitiveValue;
>+    integer->SetNumber(aGridLine.mInteger);
>+    valueList->AppendCSSValue(integer);
>+  }
>+
>+  if (!aGridLine.mLineName.IsEmpty()) {
>+    nsROCSSPrimitiveValue* lineName = new nsROCSSPrimitiveValue;
>+    nsString escapedLineName;
>+    nsStyleUtil::AppendEscapedCSSIdent(aGridLine.mLineName, escapedLineName);
>+    lineName->SetString(escapedLineName);
>+    valueList->AppendCSSValue(lineName);
>+  }
>+
>+  return valueList;

Before returning, assert that valueList.Length() != 0, to make it clear that we're expecting to enter at least *one* of the three optional clauses above (and put *something* in the list). 

>+++ b/layout/style/nsRuleNode.cpp
>+static void
>+SetGridLine(const nsCSSValue& aValue,
>+            nsStyleGridLine& aResult,
>+            const nsStyleGridLine& aParentValue,
>+            bool& aCanStoreInRuleTree)
>+
>+{
[...]
>+  case eCSSUnit_Initial:
>+  case eCSSUnit_Unset:
>+  case eCSSUnit_Auto:
>+    aResult.mIsAuto = true;
>+    // These should be ignored given mIsAuto == true,
>+    // reset them just to tidy up.
>+    aResult.mHasSpan = false;
>+    aResult.mInteger = 0;
>+    aResult.mLineName.Truncate();

Per my comment below about dropping mIsAuto, you should be able to replace this with aResult.SetAuto() (new function, suggested below).

>+  default:
>+    aResult.mIsAuto = false;
>+    aResult.mHasSpan = false;
>+    aResult.mInteger = 0;
>+    aResult.mLineName.Truncate();

Per my comment below about dropping mIsAuto, you sould be able to replace this with aResult.SetAuto() (not to actually create an auto value, but just to reset everything to the constructor-default values). And then at the very end of the 'default' case, add something like:
 MOZ_ASSERT(!aResult.IsAuto(), "we should have set something away from default value");
...to make it clear (and double-check) that we're not actually creating an implicitly "auto" value in this clause by leaving all the variables untouched.

(IsAuto(), used in that assertion, is another new function that I'm suggesting below; read on)

>+++ b/layout/style/nsStyleStruct.h
>+struct nsStyleGridLine {
>+  // http://dev.w3.org/csswg/css-grid/#typedef-grid-line
>+  bool mIsAuto;
>+  bool mHasSpan;
>+  int32_t mInteger;  // 0 means not provided
>+  nsString mLineName;  // Empty string means not provided.
>+
>+  nsStyleGridLine()
>+    : mIsAuto(true)  // The initial value is 'auto'
>+    , mHasSpan(false)
>+    , mInteger(0)
>+    // mLineName get its default constructor, the empty string

I don't think we actually need mIsAuto here. It looks like this is only "auto" IFF all the *other* variables are at their default values. So I think we can just rely on those other variables, and drop mIsAuto.
 I'd suggest addding a "bool IsAuto()" accessor which checks whether everything is at its default value, and a "void SetAuto()" setter which resets everything to its default value.

(Then you can call that instead of resetting all those things by hand, in the nsRuleNode.cpp code quoted above)

>   nsStyleCoord  mZIndex;                // [reset] integer, auto
>   // NOTE: Fields so far can be memcpy()'ed, while following fields
>   // need to have their copy constructor called when we're being copied.
>   // See nsStylePosition::nsStylePosition(const nsStylePosition& aSource)
>   // in nsStyleStruct.cpp
>   nsStyleGridTrackList mGridTemplateColumns;
>   nsStyleGridTrackList mGridTemplateRows;
>   nsCSSValueGridTemplateAreas mGridTemplateAreas;
>+  nsStyleGridLine mGridAutoPositionColumn;
>+  nsStyleGridLine mGridAutoPositionRow;
>+  nsStyleGridLine mGridColumnStart;
>+  nsStyleGridLine mGridColumnEnd;
>+  nsStyleGridLine mGridRowStart;
>+  nsStyleGridLine mGridRowEnd;

Add a comment next to or above the "mGridAutoPosition*" decls, to indicate that they're two parts of a single property.

Also, these new variables here (from other patches, too) technically should all have a "// [reset]" comment next to them, for consistency. (though I'm not sure how useful that is these days, given that each style struct must *entirely* be reset or *entirely* inherited, so they're clearly reset or they wouldn't be here... So maybe it doesn't matter too much.)

>+++ b/layout/style/test/property_database.js
>+	var gridLineOtherValues = [
>+		"foo",
>+		"2",
>+		"2 foo",
>+		"foo 2",
>+		"span 2",
>+		"2 span",
>+		"span foo",
>+		"foo span",
>+		"span 2 foo",
>+		"span foo 2",
>+		"2 foo span",
>+		"foo 2 span",
>+	];

Additional values to add here:
 - negative integers (without 'span'), which are valid

>+	var gridLineInvalidValues = [
>+		"",
>+		"4th",
>+		"span",
>+		"inherit 2",
>+		"20px",
>+		"2 3",
>+		"2 foo 3",
>+		"foo 2 foo",
>+		"span 2 span",
>+		"span foo span",
>+		"span 2 foo span",
>+	];

Additional invalid values to add here:
 - A value with 0 (spec says "A <integer> value of zero makes the declaration invalid.")

 - A value with "span" & negative integers (spec says this is invalid)

 - A value with 'span' occurring *between* the <integer> and <custom-ident>, i.e.:
   "2 span foo"
   "foo span 2"

 - A floating point values where we expect <integer>:
   2.5
   2.0 (I forget offhand whether that counts as an <integer> or not, but we should test it either way, in invalid_ or other_values.)

 - Inherit as a <custom-ident> -- e.g. "2 inherit".  (You do have a test for "inherit 2", but really that will only test whether we react correctly to junk after 'inherit'. It won't exercise our custom-ident-parsing code.)

I'm pretty sure your code already handles all of these invalid cases correctly, but it's worth double-checking.

>diff --git a/layout/style/test/test_garbage_at_end_of_declarations.html b/layout/style/test/test_garbage_at_end_of_declarations.html
>-    gElement.setAttribute("style", property + ": " + value + " blah");
>+    gElement.setAttribute("style", property + ": " + value + " +blah/");

Not sure what this change is here for - maybe this is left over from debugging test-failures or something?  Either revert it, or document it a bit better with a code-comment if it's an intentional change.
Re ParseCustomIdent: there is a WG resolution to only restrict <custom-ident> when ambiguous:

http://lists.w3.org/Archives/Public/www-style/2014Mar/0120.html

But the edits fantasai made are slightly different, CSS-wide keywords are still excluded when non-ambiguous: 

http://dev.w3.org/csswg/css-values/#custom-idents

I’ll wait until this is resolved at the spec level to make the corresponding changes in the patch. (That is, possibly moving the entire ParseCustomIdent thing to Patch 5, if <line-names> is unrestricted.)
Attachment #8385450 - Attachment is obsolete: true
Attachment #8385450 - Flags: review?(dholbert)
Attachment #8387042 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #30)
> >+  uint32_t column = 0;
> >+  nsCSSGridNamedArea* currentArea = nullptr;
> >+  while (scanner.Next(token)) {
> >+    if (token.isTrash) {
> >+      return false;
> >+    }
> >+    column++;  // Need to happen before any "continue".
> 
> It looks like this column++ makes all of the column-indexing 1-based, since
> we increment before we've ever used 'column').
> 
> Is that intentional?

Yes, track (column and row) indexing is 1-based.

In the definition of <grid-line>, the spec says than an integer N designates "the Nth grid line". The code initializing currentArea is such that track M is between lines M and M + 1.


> Probably best to just open the patch file itself and do a global
> search-and-replace. Best to do this for all of your patches, to avoid
> bitrotting them and in case they introduce any additional fancy-quote
> characters.

Well, I’m working with on a git repository locally, and only export patch files when it’s time to upload to Bugzilla. So any change I do in the patch file will be overwritten the next time I re-export it from git.


> NOTE: This also applies to the properties in Patch 4 and Patch 5, too --
> those currently have STORES_CALC and NEEDS_LAYOUT_FLUSH but don't need them.

Base on http://lists.w3.org/Archives/Public/www-style/2014Mar/0132.html , patch 3 needs STORES_CALC but not NEEDS_LAYOUT_FLUSH.


> Please add some entries to other_values with double-quotes, and some entries
> to invalid_values with unmatched quote characters (starting with
> single-quote and ending with double quote, & vice versa) (I assume that's
> invalid)

An unmatched quote is not invalid, it just doesn’t end the string. As shown in Comment 13, some of the existing tests construct a stylesheet by concatenating strings of CSS and break if we have this kind of things in other_values or invalid_values.


> Also: invalid_values should include an otherwise-valid value, with a "trash
> token" that makes it invalid.

The slash is a trash token.
Attachment #8385453 - Attachment is obsolete: true
Attachment #8385453 - Flags: review?(dholbert)
Attachment #8387043 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #32)
> No need to change this, but [observation] it does feel a bit wasteful that
> we need two nsStyleCoords (min & max) for each of these properties, on every
> nsStylePosition, when the common case will just be "auto" when this is
> unset, or even a single value ("10px") when it's set.

'auto' does not appear in computed values, it’s expanded into minmax(min-content, max-content); though I suppose we could store it as eStyleUnit_Auto and special-case serialization in nsComputedDOMStyle.cpp to emit "minmax(min-content, max-content)" for it.

It sounds like a good idea, but there is a lot aready in this bug so I’ll leave this for another bug.
Attachment #8385456 - Attachment is obsolete: true
Attachment #8387045 - Flags: review+
(In reply to Simon Sapin (:SimonSapin) from comment #36)
> > Probably best to just open the patch file itself and do a global
> > search-and-replace. Best to do this for all of your patches, to avoid
> > bitrotting them and in case they introduce any additional fancy-quote
> > characters.
> 
> Well, I’m working with on a git repository locally, and only export patch
> files when it’s time to upload to Bugzilla. So any change I do in the patch
> file will be overwritten the next time I re-export it from git.

Ah, ok -- sorry, that may be a little more work to fix, then. (I'm used to the patch queue model, where this sort of thing is trivial. Score one point for mercurial! ;))

> > NOTE: This also applies to the properties in Patch 4 and Patch 5, too --
> > those currently have STORES_CALC and NEEDS_LAYOUT_FLUSH but don't need them.
> 
> Base on http://lists.w3.org/Archives/Public/www-style/2014Mar/0132.html ,
> patch 3 needs STORES_CALC but not NEEDS_LAYOUT_FLUSH.

Nice, good point.
(In reply to Simon Sapin (:SimonSapin) from comment #37)
> 'auto' does not appear in computed values, it’s expanded into
> minmax(min-content, max-content); though I suppose we could store it as
> eStyleUnit_Auto and special-case serialization in nsComputedDOMStyle.cpp

That's what I had in mind, yeah. As you say, probably best to leave that for another bug.
Attachment #8385458 - Attachment is obsolete: true
Attachment #8385458 - Flags: review?(dholbert)
Attachment #8387070 - Flags: review?(dholbert)
Attachment #8387043 - Flags: review?(dholbert) → review+
Attachment #8387045 - Attachment description: Patch 3 v2: Add the grid-auto-{columns,rows} properties to the style system. r=dholbert → Patch 3 v3: Add the grid-auto-{columns,rows} properties to the style system. r=dholbert
Comment on attachment 8387070 [details] [diff] [review]
Patch 4 v3: Add the grid-auto-flow property to the style system.

>+++ b/layout/style/nsCSSParser.cpp
>+bool
>+CSSParserImpl::ParseGridAutoFlow()
>+{
[...]
>+  if (!(gotColumn || gotRow)) {
>+    return false;
>+  }
>+
>+  int32_t intValue = 0;
>+  if (gotColumn) {
>+    intValue |= NS_STYLE_GRID_AUTO_FLOW_COLUMN;
>+  }
>+  if (gotRow) {
>+    intValue |= NS_STYLE_GRID_AUTO_FLOW_ROW;
>+  }
>+  if (gotDense) {
>+    intValue |= NS_STYLE_GRID_AUTO_FLOW_DENSE;
>+  }

Two things:
 (1) s/intValue/bitfield/, maybe?
 (2) This can be a little cleaner & more efficient if you fold the "neither row nor column" early-return into the if-cascade below it, with some else clauses. I think this or something like it should do it:
=========
  int32_t bitfield = 0;
  if (gotColumn) {
    MOZ_ASSERT(!gotRow,
               "code above should've rejected values with both row and column");
    bitfield |= NS_STYLE_GRID_AUTO_FLOW_COLUMN;
  } else if (gotRow) {
    bitfield |= NS_STYLE_GRID_AUTO_FLOW_ROW;
  } else {
    // got neither row nor column; invalid.
    return false;
  }

  if (gotDense) {
    bitfield |= NS_STYLE_GRID_AUTO_FLOW_DENSE;
  }
=========

>+++ b/layout/style/nsStyleConsts.h
>+// grid-auto-flow values (do not map not 1:1 to keywords)
>+#define NS_STYLE_GRID_AUTO_FLOW_NONE            (1 << 0)
>+#define NS_STYLE_GRID_AUTO_FLOW_COLUMN          (1 << 1)
>+#define NS_STYLE_GRID_AUTO_FLOW_ROW             (1 << 2)
>+#define NS_STYLE_GRID_AUTO_FLOW_DENSE           (1 << 3)

Drop the "do not map 1:1 to keywords" part of the comment
(That's left over from when you had COLUMN_DENSE & ROW_DENSE as enums here)

r=me with that
Attachment #8387070 - Flags: review?(dholbert) → review+
(In reply to Simon Sapin (:SimonSapin) from comment #35)
> Created attachment 8387042 [details] [diff] [review]
> Patch 1 v7: Add the grid-template-{columns,rows} properties to the style
> system.
> 
> Re ParseCustomIdent: there is a WG resolution to only restrict
> <custom-ident> when ambiguous:
> 
> http://lists.w3.org/Archives/Public/www-style/2014Mar/0120.html
> 
> But the edits fantasai made are slightly different, CSS-wide keywords are
> still excluded when non-ambiguous: 
> 
> http://dev.w3.org/csswg/css-values/#custom-idents
> 
> I’ll wait until this is resolved at the spec level to make the corresponding
> changes in the patch. (That is, possibly moving the entire ParseCustomIdent
> thing to Patch 5, if <line-names> is unrestricted.)

The discussion in CSS WG is still ongoing. Rather than block this bug on that discussion, I will change Patch 1 to do as the spec currently says, and file another bug to check for spec changes later.

As of 2014-03-08, http://dev.w3.org/csswg/css-values/#custom-idents says:

> The CSS-wide keywords are not valid <custom-ident>s.
> The ‘default’ keyword is reserved and is also not a valid <custom-ident>.
>
> Specifications using ‘<custom-ident>’ should specify clearly what other
> keywords are excluded from <custom-ident>—for example by saying that any
> pre-defined keywords in that property's value definition are excluded.
> As a general rule, an identifier that could be interpreted as a pre-defined
> keyword in any position or multiplication of the <custom-ident> component
> value is excluded, and is invalid as a <custom-ident> matching to that
> component value even in positions where its use would be technically unambiguous.
> For example, if a keyword could be misparsed when specified as the first
> item of a ‘<custom-ident>+’ list, it is invalid when specified in any position
> in that list.
See Also: → 981300
Regarding the prefixing on min-content and max-content. More important than when I said so far, I think we’ve established (https://wiki.mozilla.org/WebAPI/ExposureGuidelines ) that prefixes hurt the Web more than they’re helpful. 

We only keep existing prefixes to avoid breaking deployed content that uses them without proper fallback on unprefixed syntax, but there is no such content is this case since CSS Grid properties have never been exposed to the web with a -moz- prefix anywhere. If any content uses these properties with a -moz- prefix (in anticipation this might we what we’ll implement), the prefix is most probably on the property name rather than on a keyword, and we’re not breaking this content since it has never worked.

So the only reason to support prefixed keywords here is to spare some confusion to authors trying to use them. But that confusion is only temporary, they’ll figure it out pretty quickly (or at least eventually.) On the other hand, if production content starts relying on prefixed keywords, we’ll be stuck with it for a very long time after the prefixes become obsolete. IMO, this is much worse than temporary author confusion.
(In reply to Simon Sapin (:SimonSapin) from comment #43)
> We only keep existing prefixes to avoid breaking deployed content that uses
> them without proper fallback on unprefixed syntax

Not quite, in this case.  Here, the prefixed keyword is the *only* version that we accept (for existing properties). It's not just a legacy alias -- not yet, at least. (Though I expect it'll become that, eventually.)

My concern here is about the consistency problems of having a keyword where we *only* accepting the unprefixed version for some properties, and *only* accept the prefixed version for other properties.

> if production content starts relying on prefixed keywords,
> we’ll be stuck with it for a very long time after the prefixes become
> obsolete. IMO, this is much worse than temporary author confusion.

I think I agree on this point, and this is probably compelling enough to proceed with the unprefixed keywords here, if dbaron is OK with it. (It sounded like he would be OK with it, from IRC conversation last week.)

(Before we un-pref grid support, though, we might want to consider adding unprefixed min-content/max-content support in other properties [making sure the spec & our implementation are stable enough], to resolve the consistency issue a bit.)
* Fix a segfault introduced in v7 when ParseCustomIdent’s aPropertyKTable was null
* Fix <line-names> parsing per Comment 42 (ie. only CSS-wide keywords are excluded)
Attachment #8387042 - Attachment is obsolete: true
Attachment #8387042 - Flags: review?(dholbert)
Attachment #8388165 - Flags: review?(dholbert)
Fix an off-by-one error introduced in v3 when checking that named areas are rectangular
Attachment #8387043 - Attachment is obsolete: true
Attachment #8388166 - Flags: review+
Add some missing semicolons in property_database.js, for consistency. (They are not strictly necessary.)
Attachment #8387045 - Attachment is obsolete: true
Attachment #8388167 - Flags: review+
* Fix some syntax errors. (Oops, I went a bit too fast with v3)
* Address review comments.
Attachment #8387070 - Attachment is obsolete: true
Attachment #8388168 - Flags: review+
Address review comments.
Attachment #8385462 - Attachment is obsolete: true
Attachment #8385462 - Flags: review?(dholbert)
Attachment #8388169 - Flags: review?(dholbert)
Pushed to try with patches 1 v8, 2 v4, 3 v4, 4 v4, and 5 v2:
https://tbpl.mozilla.org/?tree=Try&rev=e2869e82689c
Fix a build error in a NS_ASSERTION. (And adding --enable-debug to my local build now…)
Attachment #8388169 - Attachment is obsolete: true
Attachment #8388169 - Flags: review?(dholbert)
Attachment #8388447 - Flags: review?(dholbert)
More green: https://tbpl.mozilla.org/?tree=Try&rev=25ed1fe770db (patches 1 v8, 2 v4, 3 v4, 4 v4, and 5 v3)
Blocks: 981752
Blocks: 981754
Comment on attachment 8388447 [details] [diff] [review]
Patch 5 v3: Add grid-{column,row}-{start,end} and grid-auto-position to the style system.

>+++ b/layout/style/nsCSSParser.cpp
>+  nsCSSValueList* item = aValue.SetListValue();
>+  if (hasSpan) {
>+    // Given "span", a negative <integer> is invalid.
>+    if (hasInteger && integer <= 0) {

It's impossible for 'integer' to be 0 here, because you already screened out 0 values higher up. (in your do{}while loop)

So, let's make this check < 0 instead of <= 0.

(If you want to make it clearer that we know 'integer' can't be 0 here, add an assertion about that somewhere.)

>+++ b/layout/style/nsStyleStruct.h
>+  void SetAuto()
>+  {
>+    mHasSpan = false;
>+    mInteger = 0;
>+    mLineName.Truncate();
>+  }
>+
>+  bool IsAuto() const
>+  {
>+    return mInteger == 0 && mLineName.IsEmpty();
>+  }

mHasSpan is notably missing from the checks in IsAuto().

Is that because we should never set mHasSpan to true, while the other vars are at their defaults?

If so, add an assertion to that effect, e.g.:
  bool haveInitialValues =  mInteger == 0 && mLineName.IsEmpty();
  MOZ_ASSERT(!haveInitialValues || !mHasSpan,
             "should not have 'span' when other components are "
             "at their initial values");
  return haveInitialValues;


> struct nsStylePosition {
[...]
>+  // grid-auto-position
>+  nsStyleGridLine mGridAutoPositionColumn;
>+  nsStyleGridLine mGridAutoPositionRow;
>+
>+  nsStyleGridLine mGridColumnStart;

It's not clear what the comment is saying there. Replace with something like:
  // We represent the "grid-auto-position" property in two parts:

r=me with the above.
Attachment #8388447 - Flags: review?(dholbert) → review+
Address review comments, and simplify tests to have O(N) cases instead of O(N^2) for grid-auto-position, where N is the number of tests for grid-column-start. We don’t need to test all combinations. (In bug 981752, O(N^4) for grid-area got unreasonably large.)
Attachment #8388447 - Attachment is obsolete: true
Attachment #8388813 - Flags: review+
Comment on attachment 8388165 [details] [diff] [review]
Patch 1 v8: Add the grid-template-{columns,rows} properties to the style system.

>+++ b/layout/style/nsCSSKeywordList.h
>+CSS_KEY(subgrid, subgrid)

This patch shouldn't add the subgrid keyword anymore, since it doesn't use it.

(If we end up needing to change <custom-ident> parsing to exclude it (which it sounds like we won't?), we can add the keyword at that point; otherwise, we'll add the keyword when we add real support for "subgrid" parsing.)

>+++ b/layout/style/nsCSSParser.cpp
[...]
>+  bool ParseCustomIdent(nsCSSValue& aValue,
>+                        const nsAutoString& aIdentValue,
>+                        const nsCSSKeyword aPropertyKeywords[] = nullptr,
>+                        const nsCSSProps::KTableValue aPropertyKTable[] = nullptr);
[...]
>+// http://dev.w3.org/csswg/css-values/#custom-idents
>+// Parse an identifier that is not a CSS-wide keyword, nor "default",
>+// nor one of the other keywords that can appear in the value of the property.
>+bool
>+CSSParserImpl::ParseCustomIdent(nsCSSValue& aValue,
>+                                const nsAutoString& aIdentValue,
>+                                const nsCSSKeyword aPropertyKeywords[],
>+                                const nsCSSProps::KTableValue aPropertyKTable[])
>+{

Maybe rename aPropertyKeywords to aReservedKeywords or aExcludedKeywords, to make its purpose clearer?

Also, add some documentation to explain the expected contents & structure of that array. In particular: it's an array containing the nsCSSKeywords (beyond css-wide) that are to be excluded from this <custom-ident>, with a eCSSKeyword_UNKNOWN terminator.  Also, if aPropertyKTable is provided, then its keywords will be excluded, so they don't need to be redundantly listed in aPropertyKeywords.

>+  if (aPropertyKeywords) {
>+    for (uint32_t i = 0;; i++) {
>+      nsCSSKeyword key = nsCSSKeyword(aPropertyKeywords[i]);
>+      if (key == eCSSKeyword_UNKNOWN) {
>+        break;
>+      }
>+      if (keyword == key) {
>+        return false;

s/key/reservedKeyword/, to avoid ambiguity between "key" & "keyword". (Right now, there's no intuitive way to figure out which is which, without scrolling up to look at the variable declarations).

And flip the handedness of the "key == eCSSKeyword_UNKNOWN" check (so that eCSSKeyword_UNKNOWN comes before the "=="), to make it consistent with the check right after it (against 'keyword').

r=me with the above.
Attachment #8388165 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #18)
> [needinfo=dbaron on the min-content vs. -moz-min-content thing at the
> beginning of comment 17.]

Canceling needinfo, given that we discussed this in IRC and dbaron didn't have strong feelings.
Flags: needinfo?(dbaron)
Blocks: 981300
See Also: 981300
Blocks: 983175
Blocks: 984241
Blocks: 984760
Blocks: 984728
Depends on: 985955
Blocks: 994592
Blocks: 1005567
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: