Mats Palmgren (inactive) Assignee
	Description • 9 years ago

The CSS Grid spec has changed since we implemented repeat():
http://dev.w3.org/csswg/css-grid/#funcdef-repeat

	yisibl
	Comment 1 • 9 years ago

It renamed to 'auto-fill' and 'auto-fit'

	Daniel Holbert [:dholbert]
	Comment 2 • 9 years ago

Thanks, Percy. Spec link, for reference: https://drafts.csswg.org/css-grid/#typedef-auto-repeat

	yisibl
	Comment 3 • 9 years ago

@Daniel Holbert I hope to give priority to this function, which is very common layout. 

https://twitter.com/AlanHogan/status/519256635911307265/photo/1

And do you think you should add the grid-rule(like column-rule) property in spec?

	Mats Palmgren (inactive) Assignee
	Comment 4 • 9 years ago

(In reply to percyley from comment #3)
> And do you think you should add the grid-rule(like column-rule) property in spec?

Bugzilla is not the right forum for discussing new CSS features.
Please post to www-style instead:
https://lists.w3.org/Archives/Public/www-style/

	Daniel Holbert [:dholbert]
	Comment 5 • 9 years ago

Clarifying the situation, for myself at least, since I managed to confuse myself about repeat(...auto...)'s supportedness, after misunderstanding the original bug summary here.

 - We *do* support the "auto" keyword in repeat() expressions, as a track-size.
 - We do *not yet* support "auto-fill"/"auto-fit" as a repeat-count.  That's what this bug covers.

(Please correct me if I'm misunderstanding.)

	Mats Palmgren (inactive) Assignee
	Comment 6 • 9 years ago

Simon, I guess you're the best reviewer for this patch since you wrote
this code originally.

Note that the syntax now only allows one track size in the repeat():
https://drafts.csswg.org/css-grid/#valdef-repeat-auto-fill

I made the parser create a Pair type for a repeat(auto-fill/fit,...),
with the "X" value being the enum value (auto-fill/fit) and "Y"
being a list with three items:
1. a name list containing the names before the track size
2. the track size
3. a name list containing the names after the track size

(for subgrid, the "Y" value is simply a list of line names)

The line names associated with a repeat() are NOT merged with the name
lists before/after the repeat() as we do for repeat(<integer>,...).

In the StyleStruct data, the repeat track size goes into the array of
track sizing functions as usual.  The associated line name lists are
stored separately, in mRepeatAutoLineNameListBefore/After.

(The line name lookup code in layout will then sort out which names
goes where dynamically, and how many track sizes that repeat track
really represents (zero or more))

	Mats Palmgren (inactive) Assignee
	Comment 7 • 9 years ago

I've split up the layout implementation into a series of smaller patches
for easier reviewing of each step.  As a general overview, I'm adding
a new class LineNameMap that provides a line name lookup function that
knows where to look in the style data for line names in the presence
of a dynamic number of repeat() track(s).  I'm extending
TrackSizingFunctions to do the corresponding lookup for track sizes.
Then there is a patch for the function that calculates how many tracks
to repeat for a given set of [min/max/]size, and one for computing those
sizes, and lastly, a patch that implements removing empty 'auto-fit' tracks.
I'll provide a rollup patch at the end.

So here's the first part, it implements a basic LineNameMap class.
I'll add the lookup functions next...

	Mats Palmgren (inactive) Assignee
	Comment 8 • 9 years ago

This just moves the existing static FindLine/RFindLine/FindNamedLine functions
into the LineNameMap class.  I think you can rubberstamp this patch.
The next patch will modify them to work with the mapping...

	Mats Palmgren (inactive) Assignee
	Comment 9 • 9 years ago

This makes them into methods and now they also find the repeat() lines.

	Mats Palmgren (inactive) Assignee
	Comment 10 • 9 years ago

Nothing much to see here.

	Mats Palmgren (inactive) Assignee
	Comment 11 • 9 years ago

This patch adds similar code to TrackSizingFunctions to lookup track sizes
with repeat() taken into account.

	Mats Palmgren (inactive) Assignee
	Comment 12 • 9 years ago

There are some XXX comments here about spec issues.
I'll remove those, and possibly update the code, once the spec editors
answer my questions here:
https://lists.w3.org/Archives/Public/www-style/2015Dec/0034.html
https://lists.w3.org/Archives/Public/www-style/2015Dec/0042.html

	Mats Palmgren (inactive) Assignee
	Comment 13 • 9 years ago

This patch determines the sizes to use for calculating the number of
repeat tracks.  The Size and MaxSize seems straightforward - just use
the Computed[Max]Size from the reflow state.  For MinSize though,
I don't think we can do that.  We need to know if the computed style
was indefinite, so we can tell that apart from a specified min-width:0.

	Mats Palmgren (inactive) Assignee
	Comment 15 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bec5f6e3893f

	Mats Palmgren (inactive) Assignee
	Comment 16 • 9 years ago

(let me know if you want a wdiff too)

	yisibl
	Comment 17 • 9 years ago

It looks like well be able to use it in nightly, exciting!

	Mats Palmgren (inactive) Assignee
	Comment 18 • 9 years ago

Comment on attachment 8696830 [details] [diff] [review]
part 5 - [css-grid] Remove any empty 'repeat(auto-fit)' tracks at the end of its range and adjust affected grid area line numbers accordingly.

fantasai clarified that they did mean remove all empty tracks:
https://lists.w3.org/Archives/Public/www-style/2015Dec/0155.html
and added:
"We don't have a particularly strong opinion on this, though,
so if you thinkg dropping only tracks at the end would be
more useful to authors, we're okay with changing it."

So let's hold the review on this part for now.

	Mats Palmgren (inactive) Assignee
	Comment 19 • 9 years ago

Comment on attachment 8696828 [details] [diff] [review]
part 3b - [css-grid] Implement the CalculateRepeatFillCount method that calculates the number of 'repeat(auto-fill/auto-fit)' tracks to use for the given sizes.

This part might need minor modifications too per:
https://lists.w3.org/Archives/Public/www-style/2015Dec/0157.html
Feel free to skip this review too if you think that's best.

	Daniel Holbert [:dholbert]
	Comment 20 • 9 years ago

(Sorry for delay here -- Mozlando ate up a lot of time, and then my laptop refused to boot starting on Friday, so I couldn't get much done on the flight back. I only just got it working again yesterday. Reviewing this now.)

	Daniel Holbert [:dholbert]
	Comment 21 • 9 years ago

Comment on attachment 8696821 [details] [diff] [review]
part 2a - [css-grid] Add a LineNameMap class that let's us lookup line names with a dynamic number of 'repeat(auto-fill/auto-fit)' tracks taken into account.

Review of attachment 8696821 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit: s/let's/lets/

This class looks sane to me, modulo one minor nit below; I may circle back to some aspect as I better-grok how it's used later in the patch stack.

::: layout/generic/nsGridContainerFrame.cpp
@@ +215,5 @@
> +    , mRepeatAutoStart(aGridTemplate.HasRepeatAuto() ?
> +                         aGridTemplate.mRepeatAutoIndex : 0)
> +    , mRepeatAutoEnd(mRepeatAutoStart + aNumRepeatTracks)
> +    , mRepeatEndDelta(aGridTemplate.HasRepeatAuto() ?
> +                        int32_t(mRepeatAutoEnd - mRepeatAutoStart) - 1 :

Nit: Couldn't the "mRepeatAutoEnd - mRepeatAutoStart" expression be simplified to just "aNumRepeatTracks" here? (since mRepeatAutoEnd was just assigned to be mRepeatAutoStart + aNumRepeatTracks)

Seems simpler to just use the single value instead of unnecessary arithmetic.  But maybe the arithmetic makes this clearer for some reason (not sure), in which case this is fine. (This might make more sense after I grok how this structure is used, in subsequent patches.)  So, up to you if this is worth simplifying.

	Daniel Holbert [:dholbert]
	Comment 22 • 9 years ago

Comment on attachment 8696823 [details] [diff] [review]
part 2b - [css-grid] Move the static functions FindLine/RFindLine/FindNamedLine into the LineNameMap class (idempotent patch).

Review of attachment 8696823 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Daniel Holbert [:dholbert]
	Comment 23 • 9 years ago

Comment on attachment 8696821 [details] [diff] [review]
part 2a - [css-grid] Add a LineNameMap class that let's us lookup line names with a dynamic number of 'repeat(auto-fill/auto-fit)' tracks taken into account.

Review of attachment 8696821 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +262,5 @@
> +  int32_t        mRepeatEndDelta;
> +  // The end of the line name lists with repeat(auto-fill/fit) tracks accounted
> +  // for.  It is equal to mLineNameLists.Length() when a repeat() track
> +  // generates one track (making mRepeatEndDelta == 0).
> +  uint32_t       mTemplateLinesEnd;

Brief diversion back to "part 2a" -- could you make some or all of these uint/int LineNameMap member-variables 'const'?

They receive their values in the init list, and it looks like they never change after that point. If they were declared as 'const', we could make that fact more discoverable and enforceable.

	Daniel Holbert [:dholbert]
	Comment 24 • 9 years ago

Comment on attachment 8696824 [details] [diff] [review]
part 2c - [css-grid] Modify the LineNameMap::FindLine/RFindLine/FindNamedLine methods to take line names associated with 'repeat(auto-fill/auto-fit)' tracks into account.

Review of attachment 8696824 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this part doesn't build successfully on its own (it modifies an API without updating the callers of that API), so it probably shouldn't land as-is, as a patch -- or else it'll create a failure point for 'hg bisect' where the tree won't build.

I see two decent ways of resolving this:
(1) The simplest way to resolve this would be to just fold this together with part 2d (which updates the callsites). That's clumsy because part 2d has some unrelated-ish mechanical changes, but that'd be OK with me if you like.
(2) A better way to resolve this (IMO) would be to swap the order of Part 2d and this patch, and move Part 2d's FindLine argument-tweaks into this patch.
  - So the "new" part 2c (formerly 2d) would instantiate and pass around a LineNameMap object, and would make FindNamedLine an instance-method, but *would not change its args* (or anything about its implementation).
  - And the "new" part 2d (formerly 2c) would drop the aNameList arg, in the function impl *and at the callsites*, and would change the function impl to use Contains() and mTemplateLinesEnd instead of aNameList.
(emphasis added with "*" for the pieces that are different from how the patches are currently structured.)

I think this would mean we could build successfully (and stuff would basically work) at each intermediate point.

Having said that: I've got one minor nit on 2c-as-it-stands-right-now:

::: layout/generic/nsGridContainerFrame.cpp
@@ +264,4 @@
>    {
>      MOZ_ASSERT(aNth && *aNth > 0);
>      int32_t nth = *aNth;
> +    const uint32_t len = mTemplateLinesEnd;

Perhaps better to just drop 'len' and use mTemplateLinesEnd directly wherever 'len' is used?

(In the old code, 'len' was a nice alias for aNameList.Length().  Now, though, it's not exactly a length [based on the mTemplateLinesEnd documentation], so "len" might be a misleading name. Also, using a local-variable alias makes it a bit harder to find out what a comparison like "i < len" actually means -- it adds one unnecessary "hop" in a code-reader's search for documentation/meaning.)

@@ +300,5 @@
>      --aFromIndex;
>      int32_t nth = *aNth;
> +    // The implicit line may be beyond the explicit grid so we match
> +    // this line first if it's within the mTemplateLinesEnd..aFromIndex range.
> +    const uint32_t len = mTemplateLinesEnd;

Same here, RE "len".

	Daniel Holbert [:dholbert]
	Comment 25 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #24)
> (2) A better way to resolve this (IMO) would be to swap the order of Part 2d
> and this patch, and move Part 2d's FindLine argument-tweaks into this patch.
>   - So the "new" part 2c (formerly 2d) would instantiate and pass around a
> LineNameMap object, and would make FindNamedLine an instance-method, but
> *would not change its args* (or anything about its implementation).

Oh, now that I look more closely at current-part-2d, my "better way" (labeled (2)) clearly won't work, because we won't have |aLineNameList| in-scope anymore at the callsites to FindNamedLine.

Here's an amended "better way", which doesn't have that problem:
  - So the "new" part 2c (formerly 2d) would instantiate and pass around a LineNameMap object, and would make FindNamedLine an instance-method, *and would drop the aNameList arg at callsites & in the function-impl, and trivially adjust the impl to simply use mLineNameLists* instead.
  - And the "new" part 2d (formerly 2c) would update the function impl to use Contains() and mTemplateLinesEnd instead.

	Daniel Holbert [:dholbert]
	Comment 26 • 9 years ago

Comment on attachment 8696825 [details] [diff] [review]
part 2d - [css-grid] Instantiate and pass around a LineNameMap object instead of an array of line name arrays.

Review of attachment 8696825 [details] [diff] [review]:
-----------------------------------------------------------------

part 2d looks good, modulo nits below about keeping function documentation up to date, and modulo comment 24 - comment 25 concerns about keeping things buildable at intermediate commits.

::: layout/generic/nsGridContainerFrame.h
@@ +322,5 @@
>     */
>    int32_t ResolveLine(const nsStyleGridLine& aLine,
>                        int32_t aNth,
>                        uint32_t aFromIndex,
> +                      const LineNameMap& aNameMap,

The documentation above this function needs updating. It still has:
   * @param aLineNameList the explicit named lines
but you're replacing that arg with a different one here.

@@ +345,5 @@
>     * @param aStyle the StylePosition() for the grid container
>     */
>    LineRange ResolveLineRange(const nsStyleGridLine& aStart,
>                               const nsStyleGridLine& aEnd,
> +                             const LineNameMap& aNameMap,

Same here.

@@ +378,5 @@
>     */
> +  GridArea PlaceDefinite(nsIFrame* aChild,
> +                         const LineNameMap& aColLineNameMap,
> +                         const LineNameMap& aRowLineNameMap,
> +                         const nsStylePosition* aStyle);

Consider adding a trivial @param entry for the new params here.  (The @param listing for this function was complete before this patch; this patch makes it incomplete.)

@@ +444,5 @@
>     */
> +  GridArea PlaceAbsPos(nsIFrame* aChild,
> +                       const LineNameMap& aColLineNameMap,
> +                       const LineNameMap& aRowLineNameMap,
> +                       const nsStylePosition* aStyle);

Same here.

	Daniel Holbert [:dholbert]
	Comment 27 • 9 years ago

Comment on attachment 8696827 [details] [diff] [review]
part 3a - [css-grid] Modify TrackSizingFunctions to take a dynamic number of 'repeat(auto-fill/auto-fit)' tracks taking into account.

Review of attachment 8696827 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +369,5 @@
> +    : mMinSizingFunctions(aGridTemplate.mMinTrackSizingFunctions)
> +    , mMaxSizingFunctions(aGridTemplate.mMaxTrackSizingFunctions)
> +    , mAutoMinSizing(aAutoMinSizing)
> +    , mAutoMaxSizing(aAutoMaxSizing)
> +    , mExplicitGridOffset(0)

Looking at this patch as well as the rollup patch, it seems this mExplicitGridOffset member-variable is always 0 -- it never gets reassigned to anything else.  That seems like a bug, one way or another... (Either it should be reassigned to something nonzero in some scenario, or it shouldn't exist, right?)

Apologies if I'm just missing a case where it's reassigned...

@@ +376,5 @@
> +    , mRepeatAutoEnd(mRepeatAutoStart)
> +    , mRepeatEndDelta(0)
> +    , mHasRepeatAuto(aGridTemplate.HasRepeatAuto())
> +  {
> +    MOZ_ASSERT(mRepeatAutoStart <= mMinSizingFunctions.Length());

Two things:
 (1) Please assert that mMinSizingFunctions and mMaxSizingFunctions have the same length here. That validates that your other assertions here (about mMinSizingFunctions's length) are also true of mMaxSizingFunction, and that we can safely index into it in the same way.  (We had an assertion to this effect in Tracks::Initialize, but you're removing that assertion elsewhere in this patch; and this seems like a good place for that assertion to live now.)

 (2) For nontrivial cases, the assertion quoted above should really be "<", not "<=", right? I ask because further down in this patch, in MinSizingFor(), we have the following expression:
  return mMinSizingFunctions[mRepeatAutoStart];
If mRepeatAutoStart were equal to the array-length (as the assertion allows it to be), then this expression would be an out-of-bounds array access. That seems like something this assertion should catch.

(I suspect we do allow mRepeatAutoStart to be equal to the array-length *only* in trivial cases where the repeated section is empty, perhaps?  In which case we never visit that 'return statement & trigger an out-of-bounds array access.  If this is the case, then please adjust the assertion to express this subtlety, and to enforce that otherwise mRepeatAutoStart must be strictly *less than* the length.)

@@ +394,5 @@
> +    end = std::max(end, aGridTemplateAreasEnd);
> +    end = std::min(end, uint32_t(nsStyleGridLine::kMaxLine));
> +    *aExplicitGridEnd = end;
> +    *aGridEnd = end;
> +    return repeatTracks;

This function seems to be doing too many things at once, and ends up feeling messy. In particular, I have the following concerns about this function, in its current state:
 - Its name ("Bounds") is plural & implies 2D bounds, but the function only cares about in a single dimension.
 - It returns 3 values, which is a lot for an "Initialize" function.
 - 2 of its 3 return values are actually always set to the same value (its 2 outparams), which is a bit odd.
 - The direct return-value (repeatTracks) has nothing to do with the function's name (InitializeGridBounds).
 - The function takes a whole bunch of args, half of which will be used in the first part (once the XXX comment is expanded in a later patch), and half of which are for the actual grid bound computation.
 - The first part, about computing repeat information, isn't directly related to the function's nominal purpose, initializing the grid bounds.

Seems like things would be a lot better if we simply split this into two functions.  I'd suggest something like the following:
 - Split the repeatTracks/XXX/SetNumRepeatTracks() first part into its own dedicated TrackSizingFunctions member-function (with a name that makes sense w.r.t. returning repeatTracks), and invoke that function first. Strawman name: "InitRepeatTracks()".
 - In the code that remains here, collapse the two-identical-outparams to just be the (single) direct return value.
 - Maybe rename InitializeGridBounds to ComputeExplicitGridEnd, to better-describe the code that remains (since it wouldn't be doing any initialization anymore; it'd just do some computation and return a value).

So the callsite would end up looking something like this:
  uint32_t numRepeatCols = aState.mColFunctions.InitRepeatTracks(...);
  mGridColEnd = mExplicitGridColEnd =
    aState.mColFunctions.ComputeExplicitGridEnd(...);

Does that make sense?  Maybe there's another way of restructuring this that'd be even better to avoid my concerns above, too.

@@ +440,5 @@
> +  {
> +    MOZ_ASSERT(mHasRepeatAuto || aNumRepeatTracks == 0);
> +    mRepeatAutoEnd = mRepeatAutoStart + aNumRepeatTracks;
> +    mRepeatEndDelta = mHasRepeatAuto ?
> +                        int32_t(mRepeatAutoEnd - mRepeatAutoStart) - 1 :

Just like the code mentioned in comment 21 -- this "mRepeatAutoEnd - mRepeatAutoStart" expression could just be simplified to aNumRepeatTracks.

@@ +452,5 @@
>    const nsStyleCoord& mAutoMaxSizing;
> +  // Offset from the start of the implicit grid to the first explicit track.
> +  uint32_t   mExplicitGridOffset;
> +  // The index of the repeat(auto-fill/fit) track, or zero if there is none.
> +  uint32_t   mRepeatAutoStart;

I think mRepeatAutoStart here can be "const uint32_t", since this is never reassigned after the constructor's init-list (unlike mRepeatAutoEnd and mRepeatEndDelta which are set in SetNumRepeatTracks).

@@ +3150,5 @@
>    const nsStylePosition* stylePos = aReflowState.mStylePosition;
>    InitImplicitNamedAreas(stylePos);
>    GridReflowState gridReflowState(this, aReflowState);
>    mIsNormalFlowInCSSOrder = gridReflowState.mIter.ItemsAreAlreadyInOrder();
> +  LogicalSize indefinite(gridReflowState.mWM, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);

This line's too long -- please wrap it somewhere.

	Daniel Holbert [:dholbert]
	Comment 28 • 9 years ago

Comment on attachment 8696828 [details] [diff] [review]
part 3b - [css-grid] Implement the CalculateRepeatFillCount method that calculates the number of 'repeat(auto-fill/auto-fit)' tracks to use for the given sizes.

Review of attachment 8696828 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +409,5 @@
> +    }
> +    // Spec quotes are from https://drafts.csswg.org/css-grid/#repeat-notation
> +    const uint32_t numTracks = mMinSizingFunctions.Length();
> +    MOZ_ASSERT(numTracks >= 1, "expected at least the repeat() track");
> +    nscoord maxFill = aSize != NS_UNCONSTRAINEDSIZE ? aSize : aMaxSize;

Before this line (probably at the beginning of this function), please assert that:
 - aMinSize <= aSize, if neither is unconstrained
 - aSize <= aMaxSize, if neither is unconstrained

(This should presumably be enforced by nsHTMLReflowState, at some level up the stack, but there's room for us to get args confused or resolve some auto-value without clamping along the way.)

Given this assertion, this |maxFill| assignment is more clearly-correct. (If the assertion didn't hold, we could end up with maxFill being larger than aMaxSize which would be odd.)

@@ +419,5 @@
> +    nscoord sum = 0;
> +    for (uint32_t i = 0; i < numTracks; ++i) {
> +      // "The <auto-repeat> variant ... requires definite minimum track sizes ..."
> +      // "treating each track as its max track sizing function if that is definite
> +      //  or as its minimum track sizing function otherwise"

Nit: first two lines of the comment here are too long (82 characters).

(Possible tweak: just drop the space before each "..." on the 1st line, and wrap "definite" from the 2nd line to the 3rd line. Or just rewrap.)

@@ +426,5 @@
> +      const auto* coord = &maxCoord;
> +      if (!coord->IsCoordPercentCalcUnit()) {
> +        coord = &mMinSizingFunctions[i];
> +        if (!coord->IsCoordPercentCalcUnit()) {
> +          return 1; // XXX spec issue

This is a bit vague - if there's still a spec issue here, please include some hint at what the issue is. (Or a link or bug number.)

(I think you're interpreting "requires definite minimum track sizes" as requiring that of *all* tracks in the grid, which makes sense to me.  I think the #valdef-repeat-auto-fill section doesn't explicitly cover this, though, which is probably why you've got this tagged as a spec issue.  Please ask the CSSWG to clarify this, if you haven't already -- I don't immediately see this in your "few questions on auto-fill / auto-fit" thread.)

@@ +441,5 @@
> +      return 0; // XXX spec issue: repeat track-size == 0
> +    }
> +    nscoord available = maxFill != NS_UNCONSTRAINEDSIZE ? maxFill : aMinSize;
> +    if (available <= 0) {
> +      return 0; // XXX spec issue: definite max- or min-size == 0

Sounds like this should be |return 1;| per fantasai's response:
  https://lists.w3.org/Archives/Public/www-style/2015Dec/0157.html

(There will be overflowing content, and that's OK.)

I'm also not sure we want a special case for this here.  Possibly better to drop this case, & let this scenario fail the "spaceToFill >= 0" check further down, since it's really just a version of that outcome.

@@ +460,5 @@
> +        ++numRepeatTracks; // one more to ensure the grid is at least min-size
> +      }
> +    } else {
> +      // Even with just one repeat() track it doesn't fit.
> +      numRepeatTracks = maxFill != NS_UNCONSTRAINEDSIZE ? 0 : 1;

Like the case above, I'm pretty sure this should be |return 1;| per fantasai's response.

(This is the "else" for an "if (spaceToFill >= 0)" check.)

	Daniel Holbert [:dholbert]
	Comment 29 • 9 years ago

Comment on attachment 8696829 [details] [diff] [review]
part 4 - [css-grid] Provide the sizes to use for CalculateRepeatFillCount.

Review of attachment 8696829 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with the following addressed:

::: layout/generic/nsGridContainerFrame.cpp
@@ +3230,5 @@
> +  // ComputedMinSize is zero rather than NS_UNCONSTRAINEDSIZE when indefinite
> +  // (unfortunately) so we have to check the style data and parent reflow state
> +  // to determine if it's indefinite.
> +  LogicalSize computedMinSize(aReflowState.ComputedMinSize());
> +  const nsHTMLReflowState* cbState = aReflowState.parentReflowState;

This should be .mCBReflowState, not .parentReflowState, since we're really looking for a size to use for percent resolution.  (And mCBReflowState is supposed to be used for that, per http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.h?rev=fd03238ebdb5#280 )

They might be the same -- but if they're not, mCBReflowState is what we should be using.  (And I'd expect that they'd differ for an inline-grid inside of a <span> -- the grid's .parentReflowState would be for the span, I think, but its mCBReflowState would be for the span's block-container.)

@@ +3235,5 @@
> +  if (!stylePos->MinISize(wm).IsCoordPercentCalcUnit() ||
> +      (stylePos->MinISize(wm).HasPercent() && cbState &&
> +       cbState->ComputedMinSize(wm).ISize(wm) == NS_UNCONSTRAINEDSIZE)) {
> +    computedMinSize.ISize(wm) = NS_UNCONSTRAINEDSIZE;
> +  }

Why are we checking cbState->ComputedMinSize here? (emphasis on "Min")  Shouldn't we be checking its *size* (not its min-size.)

(Percent "min-width" values resolve against the size of the element's containing block:
 http://www.w3.org/TR/CSS21/visudet.html#propdef-min-width
not against the min-size of its containing block.)

@@ +3239,5 @@
> +  }
> +  if (!stylePos->MinBSize(wm).IsCoordPercentCalcUnit() ||
> +      (stylePos->MinBSize(wm).HasPercent() && cbState &&
> +       cbState->ComputedMinSize(wm).BSize(wm) == NS_UNCONSTRAINEDSIZE)) {
> +    computedMinSize.BSize(wm) = NS_UNCONSTRAINEDSIZE;

Same here, RE cbState->ComputedMinSize.

	Mats Palmgren (inactive) Assignee
	Comment 30 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #21)
> Commit message nit: s/let's/lets/

Fixed.

> Nit: Couldn't the "mRepeatAutoEnd - mRepeatAutoStart" expression be
> simplified to just "aNumRepeatTracks" here? (since mRepeatAutoEnd was just
> assigned to be mRepeatAutoStart + aNumRepeatTracks)

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #23)
> Brief diversion back to "part 2a" -- could you make some or all of these
> uint/int LineNameMap member-variables 'const'?

Sure, I made all of them const.

	Mats Palmgren (inactive) Assignee
	Comment 31 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #24)
> Looks like this part doesn't build successfully on its own

Yeah, I'll fold some of these patches before landing and
make sure they each build.

> Perhaps better to just drop 'len' and use mTemplateLinesEnd directly
> wherever 'len' is used?

mTemplateLinesEnd is short for this->mTemplateLinesEnd so it needs
to be fetched indirectly via 'this' on every iteration.
'len' OTOH can be put in a register so I prefer that.

> (In the old code, 'len' was a nice alias for aNameList.Length().  Now,
> though, it's not exactly a length [based on the mTemplateLinesEnd
> documentation], so "len" might be a misleading name.

OK, I renamed it 'end'.

	Mats Palmgren (inactive) Assignee
	Comment 32 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #26)
> The documentation above this function needs updating. It still has:
>    * @param aLineNameList the explicit named lines
> but you're replacing that arg with a different one here.

Fixed.

> Consider adding a trivial @param entry for the new params here.

Fixed.

	Mats Palmgren (inactive) Assignee
	Comment 33 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #27)
> Looking at this patch as well as the rollup patch, it seems this
> mExplicitGridOffset member-variable is always 0 -- it never gets reassigned
> to anything else.  That seems like a bug, one way or another...

mExplicitGridOffset existed before this patch series.
It's updated when we translate the grid in PlaceGridItems.

> > +    MOZ_ASSERT(mRepeatAutoStart <= mMinSizingFunctions.Length());
> 
> Two things:
>  (1) Please assert that mMinSizingFunctions and mMaxSizingFunctions have the
> same length here. 

The style system already asserts that here (at least):
http://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf9/layout/style/nsRuleNode.cpp#l7836
so I consider it an invariant that the style system guarranties.
Also, nsTArray asserts on any out-of-bounds operation.

>  (2) For nontrivial cases, the assertion quoted above should really be "<",
> not "<=", right? I ask because further down in this patch, in
> MinSizingFor(), we have the following expression:
>   return mMinSizingFunctions[mRepeatAutoStart];

Using "<" would fail when mMinSizingFunctions.Length() is zero.
mRepeatAutoStart and mRepeatAutoEnd are both zero when there is no
repeat() track (and in that case mHasRepeatAuto is false).

> If mRepeatAutoStart were equal to the array-length (as the assertion allows
> it to be), then this expression would be an out-of-bounds array access. That
> seems like something this assertion should catch.

We don't reach that code when mHasRepeatAuto is false, and the
second assertion covers that case:
    MOZ_ASSERT(!mHasRepeatAuto || mMinSizingFunctions.Length() >= 1);

> (I suspect we do allow mRepeatAutoStart to be equal to the array-length
> *only* in trivial cases where the repeated section is empty, perhaps?  In
> which case we never visit that 'return statement & trigger an out-of-bounds
> array access.  If this is the case, then please adjust the assertion to
> express this subtlety, and to enforce that otherwise mRepeatAutoStart must
> be strictly *less than* the length.)

I guess I could replace both assertions with:
MOZ_ASSERT(!mHasRepeatAuto || (mMinSizingFunctions.Length() >= 1 &&
                               mRepeatAutoStart < mMinSizingFunctions.Length());

Not sure if it's better though...

> Seems like things would be a lot better if we simply split this into two
> functions...

I agree.  Fixed as suggested.

> Just like the code mentioned in comment 21 -- this "mRepeatAutoEnd -
> mRepeatAutoStart" expression could just be simplified to aNumRepeatTracks.

Fixed.

> I think mRepeatAutoStart here can be "const uint32_t", since this is never
> reassigned after the constructor's init-list (unlike mRepeatAutoEnd and
> mRepeatEndDelta which are set in SetNumRepeatTracks).

Fixed.

> This line's too long -- please wrap it somewhere.

That line is removed in part 4.

	Mats Palmgren (inactive) Assignee
	Comment 34 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #28)
> > +    nscoord maxFill = aSize != NS_UNCONSTRAINEDSIZE ? aSize : aMaxSize;
> 
> Before this line (probably at the beginning of this function), please assert
> that:
>  - aMinSize <= aSize, if neither is unconstrained
>  - aSize <= aMaxSize, if neither is unconstrained

In an ideal world you're right of course, but nscoord values doesn't
live in that world because every now and then they overflow due to
insanely specified values.  So I'd rather not assert anything about
them.  This method was written with that in mind - it cannot possibly
return an invalid value for any given set of input values.

> Nit: first two lines of the comment here are too long (82 characters).

Fixed.

> > +          return 1; // XXX spec issue
> 
> This is a bit vague - if there's still a spec issue here, please include
> some hint at what the issue is. (Or a link or bug number.)

I now think they likely want 1, so I removed the comment.

> (I think you're interpreting "requires definite minimum track sizes" as
> requiring that of *all* tracks in the grid, which makes sense to me.  I
> think the #valdef-repeat-auto-fill section doesn't explicitly cover this,
> though, which is probably why you've got this tagged as a spec issue. 
> Please ask the CSSWG to clarify this, if you haven't already -- I don't
> immediately see this in your "few questions on auto-fill / auto-fit" thread.)

The "but requires definite minimum track sizes" under <auto-repeat> here:
https://drafts.csswg.org/css-grid/#repeat-notation
contradicts "treating each track as its max track sizing function if that
is definite or as its minimum track sizing function otherwise" here
https://drafts.csswg.org/css-grid/#valdef-repeat-auto-fill
or at least it's worded sufficiently different to introduce confusion.

I posted about that here:
https://lists.w3.org/Archives/Public/www-style/2015Dec/0033.html
Tab responded that "definite minimum track sizes" should be read
as the latter ("use the max-sizing function if that's definite
or the min-sizing functions otherwise") but it isn't defined
anywhere in the spec, one should apparently just understand this
anyway.  Later he also says "All tracks are required to have a
definite min sizing function." which makes no sense to me.
So I gave up trying to improve the spec in this area.

> > +      return 0; // XXX spec issue: repeat track-size == 0
> > +    }
> > +    nscoord available = maxFill != NS_UNCONSTRAINEDSIZE ? maxFill : aMinSize;
> > +    if (available <= 0) {
> > +      return 0; // XXX spec issue: definite max- or min-size == 0
> 
> Sounds like this should be |return 1;| per fantasai's response:
>   https://lists.w3.org/Archives/Public/www-style/2015Dec/0157.html

Yes, fixed.

> I'm also not sure we want a special case for this here.  Possibly better to
> drop this case, & let this scenario fail the "spaceToFill >= 0" check
> further down, since it's really just a version of that outcome.

Yeah, I reversed the "spaceToFill >= 0" test to return 1 if
"spaceToFill <= 0" instead; it should catch the "available <= 0" case.

> > +      numRepeatTracks = maxFill != NS_UNCONSTRAINEDSIZE ? 0 : 1;
> 
> Like the case above, I'm pretty sure this should be |return 1;| per
> fantasai's response.

I agree, fixed.


NOTE: there is a remaining uncertainty about what "floor at 1px" really
means (does it affect Track Sizing?).  I posted about that here:
https://lists.w3.org/Archives/Public/www-style/2015Dec/0243.html

We can tweak that later if needed though...

	Mats Palmgren (inactive) Assignee
	Comment 35 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #29)
> Comment on attachment 8696829 [details] [diff] [review]
> part 4 - [css-grid] Provide the sizes to use for CalculateRepeatFillCount.

Good catches, thanks!

	Mats Palmgren (inactive) Assignee
	Comment 36 • 9 years ago

Comment on attachment 8696830 [details] [diff] [review]
part 5 - [css-grid] Remove any empty 'repeat(auto-fit)' tracks at the end of its range and adjust affected grid area line numbers accordingly.

(In reply to Mats Palmgren (:mats) from comment #18)
> Comment on attachment 8696830 [details] [diff] [review]
> part 5 - [css-grid] Remove any empty 'repeat(auto-fit)' tracks at the end of
> its range and adjust affected grid area line numbers accordingly.
> 
> fantasai clarified that they did mean remove all empty tracks:
> https://lists.w3.org/Archives/Public/www-style/2015Dec/0155.html
> and added:
> "We don't have a particularly strong opinion on this, though,
> so if you thinkg dropping only tracks at the end would be
> more useful to authors, we're okay with changing it."
> 
> So let's hold the review on this part for now.

I honestly don't know what would be more useful to authors,
so lets review and land what we have and see if we can get some
feedback on that.  (It's a minor change to make it remove all
empty tracks vs. just at the end.)

	Daniel Holbert [:dholbert]
	Comment 37 • 9 years ago

Comment on attachment 8699666 [details] [diff] [review]
part 2c - [css-grid] Modify the LineNameMap::FindLine/RFindLine/FindNamedLine methods to take line names associated with 'repeat(auto-fill/auto-fit)' tracks into account.

Review of attachment 8699666 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mats Palmgren (:mats) from comment #31)
> Yeah, I'll fold some of these patches before landing and
> make sure they each build.

OK, thanks. (Wasn't sure if that was your intent, since each individual part had a nice meaningful commit message.)

r=me on this updated 2c, in that case. Thanks!

	Daniel Holbert [:dholbert]
	Comment 38 • 9 years ago

(In reply to Mats Palmgren (:mats) from comment #33)
> Created attachment 8699675 [details] [diff] [review]
> part 3a - [css-grid] Modify TrackSizingFunctions to take a dynamic number of
> 'repeat(auto-fill/auto-fit)' tracks taking into account.
[...]
> mExplicitGridOffset existed before this patch series.
> It's updated when we translate the grid in PlaceGridItems.

Ah, thanks -- I got confused and convinced myself it was new (and hence any modification would have to be happening as part of this bug), since the constructor is new & its init-list-line for the constructor is new.

Never mind about that, then.

> >  (1) Please assert that mMinSizingFunctions and mMaxSizingFunctions have the
> > same length here. 
> 
> The style system already asserts that here (at least):
> http://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf9/layout/style/
> nsRuleNode.cpp#l7836
> so I consider it an invariant that the style system guarranties.

But people reading nsGridContainerFrame.cpp* may not have any idea about that invariant.  Since we strongly depend on it here, it helps to assert about it here (and mention the style system in the assertion-message).

* (maybe not you or me, but e.g. someone diagnosing a crash in this code and reading through it for the first time, and wondering what guarantees we have about these array sizes)

> Also, nsTArray asserts on any out-of-bounds operation.

Sure, but those abstracted-away assertions are insufficient.  Asserts aren't just to catch issues in debug builds (if they were, I'd agree with you).  They're also about explicitly declaring the assumptions/invariants that the nearby code depends on, for the benefit of human readers.  In this case, the nearby code here *absolutely* depends on these two caller-provided arrays having the same length; so it's helpful to assert about that somewhere here.

> I guess I could replace both assertions with:
> MOZ_ASSERT(!mHasRepeatAuto || (mMinSizingFunctions.Length() >= 1 &&
>                                mRepeatAutoStart < mMinSizingFunctions.Length());
> 
> Not sure if it's better though...

I think that'd be better, yeah.

At least, it makes the "mMinSizingFunctions[mRepeatAutoStart]" usages look less suspiciously like out-of-bounds array accesses. (which they do look like, with the "<= Length()" assertion in the current patch.)

> > This line's too long -- please wrap it somewhere.
> 
> That line is removed in part 4.

Ah, never mind then -- thanks.

	Daniel Holbert [:dholbert]
	Comment 39 • 9 years ago

Comment on attachment 8699675 [details] [diff] [review]
part 3a - [css-grid] Modify TrackSizingFunctions to take a dynamic number of 'repeat(auto-fill/auto-fit)' tracks taking into account.

Review of attachment 8699675 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3a, with strong encouragement to...
 - add an assertion about array-lengths being equal
 - combine the mHasRepeatAuto / mRepeatAutoStart assertions in the way that you suggested
...as discussed in comment 38.

	Mats Palmgren (inactive) Assignee
	Comment 40 • 9 years ago

Comment on attachment 8694739 [details] [diff] [review]
part 1 (style system part) - [css-grid] Implement the 'auto-fill' and 'auto-fit' keywords in the repeat() function.

It appears Simon isn't available for reviews, so can you take this one too please?
There's an overview of the patch in comment 6.

	Daniel Holbert [:dholbert]
	Comment 41 • 9 years ago

Comment on attachment 8699684 [details] [diff] [review]
part 3b - [css-grid] Implement the CalculateRepeatFillCount method that calculates the number of 'repeat(auto-fill/auto-fit)' tracks to use for the given sizes.

Review of attachment 8699684 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on updated part 3b; just two nits:

::: layout/generic/nsGridContainerFrame.cpp
@@ +431,5 @@
> +      nscoord trackSize = nsRuleNode::ComputeCoordPercentCalc(*coord, aSize);
> +      if (i == mRepeatAutoStart) {
> +        // Use a minimum 1px for the repeat() track-size.
> +        if (trackSize < nsPresContext::AppUnitsPerCSSPixel()) {
> +          trackSize = nsPresContext::AppUnitsPerCSSPixel();

Drop the nsPresContext:: prefix here.

As of bug 908726, AppUnitsPerCSSPixel() is a function in the mozilla namespace. The nsPresContext static method is just a stale/deprecated alias for that, IIUC. We should probably get rid of its usages, and we should use the mozilla:: version in new code (and we can drop the mozilla:: prefix here since we have 'using namespace mozilla;')

@@ +445,5 @@
> +    nscoord available = maxFill != NS_UNCONSTRAINEDSIZE ? maxFill : aMinSize;
> +    nscoord spaceToFill = available - sum;
> +    if (spaceToFill <= 0) {
> +      // Even with just one repeat() track it doesn't fit.
> +      return 1;

The comment doesn't quite match the code here. The comment suggests that we can't even use one repeat() track, but then we return 1.  (The comment is from the earlier version of this patch, when you had "return 0" here.)

Please adjust, e.g. adding:
 // In this case, "... the specified track list repeats only once."

(This quote is from the end of https://drafts.csswg.org/css-grid/#valdef-repeat-auto-fill , and I think it's the phrase that Tab says he added to cover this case in https://lists.w3.org/Archives/Public/www-style/2015Dec/0244.html )

	Mats Palmgren (inactive) Assignee
	Comment 42 • 9 years ago

> As of bug 908726, AppUnitsPerCSSPixel() is a function in the mozilla namespace.

Cool. I've not noticed that change for two years. :-)

> +    if (spaceToFill <= 0) {
> +      // Even with just one repeat() track it doesn't fit.
> +      return 1;

The comment is still true actually, but it's more a comment on the "spaceToFill <= 0"
than what we return so I probably should have said "didn't".

> // In this case, "... the specified track list repeats only once."

That's not the right part of the spec that applies here.  The last sentence in the
auto-fill paragraph only applies to the [min-|max-|]width:auto case, like so:
"if the grid container has a definite size or max size in the relevant axis then
  ...
Otherwise, if the grid container has a definite min size in the relevant axis then
  ...
Otherwise, the specified track list repeats only once."

The "return 1" above is now covered by "if any number of repetitions would overflow,
then 1 repetition" which is the new spec text that Tab added (it was undefined
before that addition).  I'll add that quote instead.

Once again, one can conclude that the Grid spec is severely lacking in readability
and clarity.  It's sad that the editors can't take a hint about that.  :-(

	Daniel Holbert [:dholbert]
	Comment 43 • 9 years ago

Comment on attachment 8696830 [details] [diff] [review]
part 5 - [css-grid] Remove any empty 'repeat(auto-fit)' tracks at the end of its range and adjust affected grid area line numbers accordingly.

Review of attachment 8696830 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, nits below.

::: layout/generic/nsGridContainerFrame.cpp
@@ +2158,5 @@
> +         --row) {
> +      ++numEmptyRows;
> +    }
> +  }
> +  // Remove any empty tracks.

s/any empty tracks/any empty 'auto-fit' tracks/

(We don't "remove any empty tracks", in general.)

::: layout/generic/nsGridContainerFrame.h
@@ +176,5 @@
>          mEnd = translatedMax;
>        }
>      }
>      /**
> +     * Translate the lines to account for removed tracks.  This method is only

Consider:  s/removed tracks/removed (empty) tracks/

This makes it clearer that we don't need to worry about items *in* those removed tracks, because there are no such items.

@@ +185,5 @@
> +    {
> +      MOZ_ASSERT(mStart != kAutoLine, "invalid resolved line for a grid item");
> +      MOZ_ASSERT(mEnd != kAutoLine, "invalid resolved line for a grid item");
> +      if (mStart >= aFirstRemovedTrack) {
> +        MOZ_ASSERT(mStart >= aNumRemovedTracks);

So: I think your assertion here is insufficiently strong. I think it really should be:
  MOZ_ASSERT(mStart >= aFirstRemovedTrack + aNumRemovedTracks,
            "can't start in a removed range of tracks - those tracks are "
            "supposed to be empty");

(message-text partially cribbed from your next assert in this patch.)

Right now, this assert doesn't make any sense to me, aside from that it will make us abort before subtracting below 0 in the arithmetic below it (which is nice but isn't really meaningful).

Concrete example: suppose we're removing 5 empty tracks, starting at track 20, and we find a grid item that starts after track 20. This assertion will check that it *also* starts after line 5 (clearly it does).  Why do we care about line 5 at all? It's not a meaningful line for us to care about in this operation.  The meaningful line to assert about is line 25, i.e. aFirstRemovedTrack + aNumRemovedTracks.  Our item better start after *that* line, if we're going to adjust it to account for the removed range -- *that* seems assert-worthy.

@@ +194,5 @@
> +                   "range of tracks - those tracks are supposed to be empty");
> +      }
> +    }
> +    /**
> +     * Translate the lines to account for removed tracks.  This method is only

As above, consider s/removed/removed (empty)/

@@ +204,5 @@
> +                                      uint32_t aNumRemovedTracks)
> +    {
> +      if (mStart != nsGridContainerFrame::kAutoLine &&
> +          mStart >= aFirstRemovedTrack) {
> +        MOZ_ASSERT(mStart >= aNumRemovedTracks);

Above comment applies to this assertion, too:
 - I think this wants to be asserting about aFirstRemovedTrack+aNumRemovedTracks
 - And ideally this should have an explanatory assertion message, since it's non-obvious why this must hold, at first glance -- this isn't just like a null check.

@@ +209,5 @@
> +        mStart -= aNumRemovedTracks;
> +      }
> +      if (mEnd != nsGridContainerFrame::kAutoLine &&
> +          mEnd >= aFirstRemovedTrack) {
> +        MOZ_ASSERT(mEnd >= aNumRemovedTracks);

This should have an assertion message -- please copypaste the message from this same assertion in the other function up above.

	Daniel Holbert [:dholbert]
	Comment 44 • 9 years ago

Comment on attachment 8694739 [details] [diff] [review]
part 1 (style system part) - [css-grid] Implement the 'auto-fill' and 'auto-fit' keywords in the repeat() function.

Review of attachment 8694739 [details] [diff] [review]:
-----------------------------------------------------------------

I'm working my way through the style-system patch -- here's my first wave of notes on that:

::: layout/style/nsCSSParser.cpp
@@ +929,5 @@
>    bool ParseOptionalLineNameListAfterSubgrid(nsCSSValue& aValue);
> +
> +  enum GridTrackSizeFlags {
> +    eDefaultTrackSize = 0x0,
> +    eFixedTrackSize   = 0x1, // parse a <fixed-size> instead of <track-size>

This be "enum class", for better type-safety.

@@ +8702,5 @@
> +               mToken.mIdent.LowerCaseEqualsLiteral("auto-fit")) {
> +      aRepeatAuto->emplace(NS_STYLE_GRID_REPEAT_AUTO_FIT);
> +    } else {
> +      SkipUntil(')');
> +      return false;

I'd like to push *all* of these SkipUntil(')') error-handling cases up 2 levels, to be right next to the code that actually checks for the "repeat(" token. I'd like those two callsites to each look something like this:

    if (mToken.mType == eCSSToken_Function &&
        mToken.mIdent.LowerCaseEqualsLiteral("repeat")) {
      if (!ParseGridTrackListRepeat(&item)) {
        SkipUntil(')');
        return false;
      }
...and then we can do away with all of the SkipUntil calls inside of the helper-functions, because they can trust this caller to do the skipping.

I'd prefer that because:
 - It makes it much easier to see why we need it & what its purpose is (we're parsing a close-paren right next to where we parsed the open-paren)
 - It makes it much easier to be sure about whether or not we need it in a particular place (Did our helper-function do it for us already? and/or will our caller do it if we return failure?)  Rule of thumb is to trust that whoever parsed the "(" will also perform the skip-to-")" on failure.
 - It makes it much easier to see that we *are* doing it reliably (not accidentally skipping one error case), and doing it the right number of times (from accidentally doing it twice for some error case).
 - It will make the code shorter -- right now we have 4 of these SkipUntil calls in ParseGridTrackRepeatIntro, and another 4-5 in each of its callers (ParseGridTrackListRepeat & ParseGridNameListRepeat). Those can all go away.

It probably makes sense to do this change as an idempotent helper-patch, probably layering on top of your work here to avoid bitrotting yourself.  (But if you want to do the helper-patch first, that works for me too.)

::: layout/style/test/property_database.js
@@ +5956,5 @@
>        "[a] 2.5fr [z] Repeat(4, [a] 20px [] auto) [d]",
>        "[a] 2.5fr [z] Repeat(4, 20px [b c] auto [b c]) [d]",
> +      "[a] 2.5fr [z] Repeat(4, 20px auto) [d]",
> +      "repeat(Auto-fill, 0)",
> +      "[a] repeat( Auto-fill,1%)",

Nit: you never actually test that "auto-fill" (the standard lowercase spelling) is valid.

Maybe change this second usage here to use "auto-fill" instead of "Auto-fill"?

	Daniel Holbert [:dholbert]
	Comment 45 • 9 years ago

Comment on attachment 8694739 [details] [diff] [review]
part 1 (style system part) - [css-grid] Implement the 'auto-fill' and 'auto-fit' keywords in the repeat() function.

Review of attachment 8694739 [details] [diff] [review]:
-----------------------------------------------------------------

Second (& final) wave of notes on the style-system patch -- r=me with this & previous comment addressed.

Firstly, remember to update commit message to have "r=dholbert" (instead of simon.sapin)

::: dom/locales/en-US/chrome/layout/css.properties
@@ +173,5 @@
>  PEExpectedVariableCommaOrCloseParen=Expected ',' or ')' after variable name in variable reference but found '%1$S'.
>  PESubgridNotSupported=Support for the 'subgrid' keyword of CSS Grid is not enabled.
> +PEMoreThanOneGridRepeatAutoFillInNameList=Only one repeat(auto-fill, ...) is allowed in a name list for a subgrid.
> +PEMoreThanOneGridRepeatAutoFillFitInTrackList=Only one repeat(auto-fill, ...) or repeat(auto-fit, ...) is allowed in a track list.
> +PEMoreThanOneGridRepeatTrackSize=Only one track size is allowed inside repeat().

This last message isn't true of repeat() in general -- it's only true of repeat(auto-fill, ...) or repeat(auto-fit, ...), I think.

Please update the message to make it more specific.

::: layout/style/nsCSSParser.cpp
@@ +8403,5 @@
> +      SkipUntil(')');
> +      return false;
> +    }
> +    if (!ExpectSymbol(')', true)) {
> +      return false;

I'm pretty sure we're missing a SkipUntil(')') here.  We're inside of a repeat() expression, and we just discovered we failed to parse (because there's unexpected junk when we were expecting the close-paren).  At that point we're supposed to seek to the next close-paren -- to close out the invalid repeat()-expression -- before we allow ourselves to try to resume parsing.

Really, I'd like for all error-handling SkipUntil calls to happen up one level from here, as noted in comment 44. BUT, until we've done that refactoring (probably in a followup patch), I'm pretty sure this error-case needs its own SkipUntil. (though, do correct me if I'm misunderstanding)

@@ +8406,5 @@
> +    if (!ExpectSymbol(')', true)) {
> +      return false;
> +    }
> +    // Instead of hooking up this list into the flat name list as usual
> +    // we create a pair(Int, List) where the first value is the auto-fill

Add comma after "as usual" (at end of 1st line).

@@ +8410,5 @@
> +    // we create a pair(Int, List) where the first value is the auto-fill
> +    // keyword and the second is the name list to repeat.
> +    *aTailPtr = (*aTailPtr)->mNext = new nsCSSValueList;
> +    nsCSSValue kwd;
> +    kwd.SetIntValue(repeatAuto.value(), eCSSUnit_Enumerated);

IMO this would be clearer if this line...
 *aTailPtr = (*aTailPtr)->mNext = new nsCSSValueList;
...were bumped down 2 lines, to be after we set up |kwd|.

Right now its placement seems kind of arbitrary.  If it moved down, then the ordering would be more logically-grouped -- we'd have these logical groupings of things:
 1) stuff dealing with |listValue| (2nd pair value)
 2) stuff dealing with |kwd| (1st pair value)
 3) stuff dealing with |aTailPtr| (extending it with a new list entry & populating that entry with the pair).

(Right now, the patch modifies aTailPtr in between step 1 and step 2, for no obvious reason, since we don't actually work with aTailPtr until later on.)

@@ +8463,2 @@
>    for (;;) {
> +    // First try to parse repeat(<positive-integer> | auto-fill, <line-names>+)

Please mention <name-repeat> here (the spec's shorthand for this expression).

The longhand version is nice to have handy too -- maybe mention both? e.g.
 // First try to parse <name-repeat>, i.e.
 // repeat(<positive-integer> | auto-fill, <line-names>+)

@@ +8682,5 @@
>  
>  // Assuming the 'repeat(' function token has already been consumed,
> +// parse "repeat( <positive-integer> | auto-fill | auto-fit ,"
> +// (or "repeat( <positive-integer> | auto-fill ," when aForSubgrid is true)
> +// but stop after the comma.  Return true when parsing succeeds,

s/but stop/and stop/?  (The "but" term throws me off slightly, because it implies we're doing something unexpected -- but really, we're just parsing exactly what you just said we're going to parse, and nothing more.)

@@ +8731,3 @@
>  //         [ <line-names>? <track-size> ]+ <line-names>? )
>  // Append to the linked list whose end is given by |aTailPtr|,
>  // and updated |aTailPtr| to point to the new end of the list.

s/and updated/and update/

(You fixed one of these in this patch; this is another one that needs fixing.)

@@ +8736,5 @@
>  CSSParserImpl::ParseGridTrackListRepeat(nsCSSValueList** aTailPtr)
>  {
> +  int32_t repetitions;
> +  Maybe<int32_t> repeatAuto;
> +  if (!ParseGridTrackRepeatIntro(false, &repetitions, &repeatAuto)) {

Nit: The variable "Maybe<int32_t> repeatAuto" superficially looks like it could be a repeat-count of some sort. (but it's not -- really, it's an enum)

Please name it "repeatAutoEnum" or "repeatAutoType" or something like that which is more clearly enum-flavored. (This applies to several places -- there are multiple |repeatAuto| local vars & |aRepeatAuto| args in this patch which IMO would all benefit from a rename along these lines.)

@@ +8778,5 @@
>  
> +    // <auto-repeat> only accepts a single track size:
> +    // <line-names>? <fixed-size> <line-names>?
> +    if (repeatAuto.isSome()) {
> +      REPORT_UNEXPECTED(PEMoreThanOneGridRepeatTrackSize);

(For reference, here's the one usage of the css.properties entry that I've got a nit about elsewhere in this bug-comment.   As you can see from this code, this warning message needs to be specifically about <auto-repeat>, i.e. repeat(auto-fill|fit, ...) -- not about generic "repeat()".)

@@ +8810,5 @@
>    // * lastLineNames: the last <line-names>
>  
> +  if (repeatAuto.isSome()) {
> +    // Instead of hooking up this list into the flat track/name list as usual
> +    // we create a pair(Int, List) where the first value is the auto-fill/fit

Please add comma after "as usual" (end of 1st line)

@@ +8825,5 @@
> +    list = list->mNext = new nsCSSValueList;
> +    list->mValue = lastLineNames;
> +    *aTailPtr = (*aTailPtr)->mNext = new nsCSSValueList;
> +    nsCSSValue kwd;
> +    kwd.SetIntValue(repeatAuto.value(), eCSSUnit_Enumerated);

As noted for another function above,  I think the *aTailPtr assignment line should get bumped down 2 lines, to be after the |kwd| manipulation & grouped together with the other aTailPtr manipulation.

::: layout/style/nsComputedDOMStyle.cpp
@@ +2425,5 @@
>  
>      for (uint32_t i = 0; i < aTrackList.mLineNameLists.Length(); i++) {
> +      if (MOZ_UNLIKELY(aTrackList.IsRepeatAutoIndex(i))) {
> +        MOZ_ASSERT(aTrackList.mIsAutoFill, "subgrid can only have 'auto-fill'");
> +        MOZ_ASSERT(aTrackList.mRepeatAutoLineNameListAfter.IsEmpty());

Can you include a message for this second assertion? This stuff is complex enough (with these variables being used/unused/used-differently under different circumstances) that it's non-obvious what this assertion is about & why it should hold.

(I think the reason is "a subgrid's <name-repeat> expression only has one run of <line-names>, all of which should be stored in mRepeatAutoLineNameListBefore."  That's a bit long; maybe it can be expressed more compactly, or maybe it's OK if this message is a bit long.)

@@ +2479,5 @@
> +      nsROCSSPrimitiveValue* end = new nsROCSSPrimitiveValue;
> +      end->SetString(NS_LITERAL_STRING(")"));
> +      valueList->AppendCSSValue(end);
> +      continue;
> +    }

Consider just using "} else {", instead of "continue" here.  There's only one statement after this clause, and "continue" to skip that one statement seems like overkill.  (IMO the logic is easier to follow with "if { ... } else { ... }")

(Also, this chunk doesn't apply cleanly to my tree -- I think this needs some unbitrotting.)

::: layout/style/nsRuleNode.cpp
@@ +7794,5 @@
>        aResult.mIsSubgrid = true;
>        item = item->mNext;
> +      for (int32_t i = 0; item && i < nsStyleGridLine::kMaxLine; ++i) {
> +        if (item->mValue.GetUnit() == eCSSUnit_Pair) {
> +          // This is a 'auto-fill' repeat line name list.

Consider calling this a "'auto-fill' <name-repeat> expression" (or mentioning that term in a parenthetical here), to be precise & share the spec's terminology.

@@ +7797,5 @@
> +        if (item->mValue.GetUnit() == eCSSUnit_Pair) {
> +          // This is a 'auto-fill' repeat line name list.
> +          const nsCSSValuePair& pair = item->mValue.GetPairValue();
> +          MOZ_ASSERT(aResult.mRepeatAutoIndex == -1,
> +                     "can only have one <auto-repeat>");

I'm pretty sure "<auto-repeat>" is a copypaste typo here -- I think you mean "<name-repeat> with auto-fill".  (We're dealing with a subgrid here, and that's the version that comes with subgrid -- not <auto-repeat>, IIUC.)

@@ +7826,5 @@
>            break;
>          }
>  
> +        if (item->mValue.GetUnit() == eCSSUnit_Pair) {
> +          // This is a 'auto-fill' / 'auto-fit' repeat track.

Consider mentioning <auto-repeat> here to match the spec terminology. (similar to above)

::: layout/style/nsStyleStruct.h
@@ +1295,5 @@
> +// If mRepeatAutoIndex != -1 then that index is an <auto-repeat> and
> +// mIsAutoFill == true means it's an 'auto-fill', otherwise 'auto-fit'.
> +// mRepeatAutoLineNameListBefore is the list of line names before the track
> +// size, mRepeatAutoLineNameListAfter the names after.  (They are empty
> +// when there is no repeat() track, i.e. when mRepeatAutoIndex == -1).

I think this last line should say "when there is no <auto-repeat> track", or "when there is no repeat(auto-*,...) track", or something like that, instead of the current text about "no repeat() track".

The current text, "no repeat() track", is too vague; the variables you're discussing here are only related to repeat(auto-[fill|fit]), and aren't used at all for other repeat() expressions IIUC.  So, while the current text is *technically* correct (yes, they will be empty when there are no repeat() tracks), it's confusing because it makes it sound like this stuff is tied to repeat() in general, and really it's not.

@@ +1332,5 @@
> +    return mRepeatAutoIndex != -1;
> +  }
> +
> +  bool IsRepeatAutoIndex(uint32_t aIndex) const {
> +    return int32_t(aIndex) == mRepeatAutoIndex;

Hmm. I'm wary about the ability to get false positives here.

Just looking at this function in isolation, it looks like someone could call this function with a UINT32_MAX, which will produce -1 when converted to int32_t here, and that'll sporadically return "true" in cases where mRepeatAutoIndex is -1 (i.e. when there is no <auto-repeat> track at all).

* If you're confident that this cannot happen at the callsites, please add an appropriate assertion here. (e.g. I'll bet we know that the passed-in track number will already be clamped to be less than some maximum value, maybe kMaxLine or nsGridContainerFrame::kTranslatedMaxLine? Please assert that, if so.)

* Otherwise, if you think this sort of false positive *can* happen, then please adjust this function so that it doesn't happen -- e.g. check HasRepeatAuto() first, and fail if that check fails. That way, we won't get spurious matches against mRepeatAutoIndex values of -1.

	Mats Palmgren (inactive) Assignee
	Comment 46 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #43)
> > +        MOZ_ASSERT(mStart >= aNumRemovedTracks);
> 
> Above comment applies to this assertion, too:
>  - I think this wants to be asserting about
> aFirstRemovedTrack+aNumRemovedTracks

No, abs.pos. lines can definitely be inside the removed range
(they don't count as occupying cells so the tracks they span are
still empty unless there's a real grid item in it).

Hmm, I see now that this code doesn't handle that very well actually.
If we have a grid like this (with . denoting empty auto-fit tracks):
"a b c . . . . d"
        |-------| <-- abs.pos. grid-column:5/-1;
and an abs.pos. spanning into the removed range as above then
we shouldn't subtract 4... I can actually make this assert
go off with a large number of removed tracks. :-(

So, how I want this to work is that an abs.pos. line that
is inside the removed range should snap to the edge of the
removed range, for example:
"a b c . . . . d"
        |-------| (grid-column: 5/-1)
|-------|         (grid-column: 1/5)
        |---|     (grid-column: 5/7)
after the empty tracks are removed:
"a b c d"
      |-|
|-----|
      |-|

in the last case, we first end up with mStart == mEnd, but that
would be illegal, so we have two alternatives: treat it as span 1
or set the end line to 'auto'.  The argument for span 1 is that
the author did in fact place this area at definite lines to begin
with.  The argument for 'auto' is that it's more consistent with
the usual conflict handling in 9.3.1:
"If the start line is equal to the end line, remove the end line."
(and for abs.pos. "remove the end line" means set it to auto)

The spec doesn't say anything about how to handle abs.pos. lines
at all AFAICT, so I think we should propose the above, but I'm
not quite sure what is best in the last case.

What do you think?

	Daniel Holbert [:dholbert]
	Comment 47 • 9 years ago

I don't have a strong preference. I'd say to pick whichever one makes the most sense to you and/or is easiest to implement for now, and ask for clarification on the list.

(One note: the spec does say that this empty-track-dropping "can result in all tracks being dropped", so it's entirely possible that we'll end up with 0 tracks -- that's something we have to allow for when handling these abspos children.)

	Mats Palmgren (inactive) Assignee
	Comment 48 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #44)
> > +  enum GridTrackSizeFlags {
> > +    eDefaultTrackSize = 0x0,
> > +    eFixedTrackSize   = 0x1, // parse a <fixed-size> instead of <track-size>
> 
> This be "enum class", for better type-safety.

Fixed.

> I'd like to push *all* of these SkipUntil(')') error-handling cases up 2
> levels, to be right next to the code that actually checks for the "repeat("
> token.

Yep, makes sense. Fixed.

> ::: layout/style/test/property_database.js
> Nit: you never actually test that "auto-fill" (the standard lowercase
> spelling) is valid.

Fixed.


(In reply to Daniel Holbert [:dholbert] from comment #45)
> ::: dom/locales/en-US/chrome/layout/css.properties
> > +PEMoreThanOneGridRepeatTrackSize=Only one track size is allowed inside repeat().
> 
> This last message isn't true of repeat() in general -- it's only true of
> repeat(auto-fill, ...) or repeat(auto-fit, ...), I think.

Fixed.

> ::: layout/style/nsCSSParser.cpp
> @@ +8403,5 @@
> > +      SkipUntil(')');
> > +      return false;
> > +    }
> > +    if (!ExpectSymbol(')', true)) {
> > +      return false;
> 
> I'm pretty sure we're missing a SkipUntil(')') here.

Right.  This is now fixed by moving the SkipUntil up two
levels as you suggested.  Good catch!

> Add comma after "as usual" (at end of 1st line).

Fixed.

> > +    *aTailPtr = (*aTailPtr)->mNext = new nsCSSValueList;
> > +    nsCSSValue kwd;
> > +    kwd.SetIntValue(repeatAuto.value(), eCSSUnit_Enumerated);
> 
> IMO this would be clearer if this line...
>  *aTailPtr = (*aTailPtr)->mNext = new nsCSSValueList;
> ...were bumped down 2 lines, to be after we set up |kwd|.

OK, fixed.

> The longhand version is nice to have handy too -- maybe mention both? e.g.
>  // First try to parse <name-repeat>, i.e.
>  // repeat(<positive-integer> | auto-fill, <line-names>+)

Sure.

> s/but stop/and stop/? 

Fixed.

> s/and updated/and update/

Fixed.

> >  CSSParserImpl::ParseGridTrackListRepeat(nsCSSValueList** aTailPtr)
> >  {
> > +  int32_t repetitions;
> > +  Maybe<int32_t> repeatAuto;
> > +  if (!ParseGridTrackRepeatIntro(false, &repetitions, &repeatAuto)) {
> 
> Nit: The variable "Maybe<int32_t> repeatAuto" superficially looks like it
> could be a repeat-count of some sort. (but it's not -- really, it's an enum)

Well, the "Auto" implies it's auto-fill/-fit and nothing else IMO.
But sure, I don't mind appending "Enum".  Fixed.

> Please add comma after "as usual" (end of 1st line)

Fixed.

> As noted for another function above,  I think the *aTailPtr assignment line
> should get bumped down 2 lines, to be after the |kwd| manipulation & grouped
> together with the other aTailPtr manipulation.

Fixed.

> ::: layout/style/nsComputedDOMStyle.cpp
> > +        MOZ_ASSERT(aTrackList.mRepeatAutoLineNameListAfter.IsEmpty());
> 
> Can you include a message for this second assertion?

"mRepeatAutoLineNameListAfter isn't used for subgrid"

> Consider just using "} else {", instead of "continue" here.

Sure, fixed.

> ::: layout/style/nsRuleNode.cpp
> > +          // This is a 'auto-fill' repeat line name list.
> 
> Consider calling this a "'auto-fill' <name-repeat> expression"

OK.

> > +                     "can only have one <auto-repeat>");
> 
> I'm pretty sure "<auto-repeat>" is a copypaste typo here -- I think you mean
> "<name-repeat> with auto-fill".

Yep, fixed.

> > +        if (item->mValue.GetUnit() == eCSSUnit_Pair) {
> > +          // This is a 'auto-fill' / 'auto-fit' repeat track.
> 
> Consider mentioning <auto-repeat> here to match the spec terminology.
> (similar to above)

Fixed.

> ::: layout/style/nsStyleStruct.h
> > +// If mRepeatAutoIndex != -1 then that index is an <auto-repeat> and
> > +// mIsAutoFill == true means it's an 'auto-fill', otherwise 'auto-fit'.
> > +// mRepeatAutoLineNameListBefore is the list of line names before the track
> > +// size, mRepeatAutoLineNameListAfter the names after.  (They are empty
> > +// when there is no repeat() track, i.e. when mRepeatAutoIndex == -1).
> 
> I think this last line should say "when there is no <auto-repeat> track", or
> "when there is no repeat(auto-*,...) track", or something like that, instead
> of the current text about "no repeat() track".

OK. (FYI, repeat(<positive-integer>) is expanded and disappear already
in the parser so it's not really a thing here.)

> > +  bool IsRepeatAutoIndex(uint32_t aIndex) const {
> > +    return int32_t(aIndex) == mRepeatAutoIndex;
> 
> Hmm. I'm wary about the ability to get false positives here.
> 
> Just looking at this function in isolation, it looks like someone could call
> this function with a UINT32_MAX, which will produce -1 when converted to
> int32_t here, and that'll sporadically return "true" in cases where
> mRepeatAutoIndex is -1 (i.e. when there is no <auto-repeat> track at all).

I added an assertion that it's less than 2*kMaxLine there.

	Mats Palmgren (inactive) Assignee
	Comment 49 • 9 years ago

After rebasing we need to support auto-fill/fit in
nsComputedDOMStyle::GetGridTemplateColumnsRows so I added that.
You only need to re-review this method.

(I punted on the 'subgrid' part though since it's not clear to me
how that should work.)

	Mats Palmgren (inactive) Assignee
	Comment 50 • 9 years ago

This patch implements what I suggested above, with the end = 'auto'
alternative for the case where the grid area is completely inside
the removed range.
(You only need to re-review the AdjustAbsPosForRemovedTracks method)


(In reply to Daniel Holbert [:dholbert] from comment #43)
> s/any empty tracks/any empty 'auto-fit' tracks/

It sort of follows from the earlier comment "Count empty
'auto-fit' tracks..." that it's those are the track we're
removing. :-)   But sure, it doesn't hurt to spell it out.

> ::: layout/generic/nsGridContainerFrame.h
> > +     * Translate the lines to account for removed tracks.  This method is only
> 
> Consider:  s/removed tracks/removed (empty) tracks/

Fixed.

> > +        MOZ_ASSERT(mStart >= aNumRemovedTracks);
> 
> So: I think your assertion here is insufficiently strong. I think it really
> should be:
>   MOZ_ASSERT(mStart >= aFirstRemovedTrack + aNumRemovedTracks,
>             "can't start in a removed range of tracks - those tracks are "
>             "supposed to be empty");

Fixed.

> As above, consider s/removed/removed (empty)/

Fixed.

	Daniel Holbert [:dholbert]
	Comment 53 • 9 years ago

(In reply to Mats Palmgren (:mats) from comment #49)
> (I punted on the 'subgrid' part though since it's not clear to me
> how that should work.)

OK -- could you file a followup for that, and include a bug number in the XXX comment (in nsComputedDOMStyle::GetGridTemplateColumnsRows), so that's tracked somewhere?

	Daniel Holbert [:dholbert]
	Comment 54 • 9 years ago

Comment on attachment 8700709 [details] [diff] [review]
part 1 (style system part) - [css-grid] Implement the 'auto-fill' and 'auto-fit' keywords in the repeat() function.

Review of attachment 8700709 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsComputedDOMStyle.cpp
@@ +2463,5 @@
> +                   "mRepeatAutoLineNameListAfter isn't used for subgrid");
> +        nsROCSSPrimitiveValue* start = new nsROCSSPrimitiveValue;
> +        start->SetString(NS_LITERAL_STRING("repeat(auto-fill,"));
> +        valueList->AppendCSSValue(start);
> +        AppendGridLineNames(valueList, aTrackList.mRepeatAutoLineNameListBefore);

Could we assert that aTrackList.mRepeatAutoLineNameListBefore is non-empty here, before we make this call? (Maybe we could assert this right after we assert that mRepeatAutoLineNameListAfter *is* empty, as a counterpoint to that assertion?)

Reason for asking: if mRepeatAutoLineNameListBefore were empty here, we'd end up producing "repeat(auto-fill, )" here, which would be invalid -- so it's not something we should've been able to parse, and not something we want to produce via this serialization code.

(In the CSS grammar, the thing we're serializing here is...
 <name-repeat> = repeat( [ <positive-integer> | auto-fill ], <line-names>+)
...and the "+" there means one-or-more, which means our name-list must be nonempty here.)

@@ +2466,5 @@
> +        valueList->AppendCSSValue(start);
> +        AppendGridLineNames(valueList, aTrackList.mRepeatAutoLineNameListBefore);
> +        nsROCSSPrimitiveValue* end = new nsROCSSPrimitiveValue;
> +        end->SetString(NS_LITERAL_STRING(")"));
> +        valueList->AppendCSSValue(end);

(Since valueList is space-delimited, I think we'll end up with a trailing space before this close-paren, right? That seems slightly undesirable, but maybe not a big deal.)

@@ +2494,5 @@
> +    int32_t endOfRepeat = 0;  // first index after any repeat() tracks
> +    int32_t offsetToLastRepeat = 0;
> +    if (aTrackList.HasRepeatAuto()) {
> +      // offsetToLastRepeat is -1 if all repeat(auto-fit) tracks are empty
> +      offsetToLastRepeat = numTracks + 1 - aTrackList.mLineNameLists.Length();

Did you mean for this comment to be "auto-fit" specific? (and exclude "auto-fill"?)

If you wanted to cover both possibilities, maybe generalize by replacing "auto-fit" with "auto-*", or "auto-fit|fill" (barely fits in 80 chars)

@@ +2504,5 @@
> +          const nsTArray<nsString>& lineNames = aTrackList.mLineNameLists[i];
> +          if (i == endOfRepeat) {
> +            // all auto-fit tracks are empty, so "[a] repeat(...) [b]"
> +            // becomes "[a b]"
> +            AppendGridLineNames(valueList, lineNames,

As above, is this really "auto-fit" specific, or does it cover auto-fill as well?

Also: This whole section seems to be expanding the repeat(auto-*, ...) expression to the actual line-names / line-sizes list.  So, the outputted string won't have a "repeat" expression at all.  Are you sure we want the expanded version here?

I was expecting that we'd just append the repeat() expression itself, not its expanded version. The expansion (with these "auto-*" keywords) will be different depending on the grid's exact layout/sizing, of course... I'm not sure we want getComputedStyle() to vary based on that. (It clearly does for e.g. "width" and "height", but I thought those were special cases and not how getComputedStyle is supposed to work in general.)

(Note that for integer-valued "repeat" expressions, we'll output the expanded version; that's different, because the expanded version is unambiguously equivalent to the repeat() expression.  Here, with the "auto" keywords, they aren't unambiguously equivalent, since the expansion will change depending on sizing.)

	Daniel Holbert [:dholbert]
	Comment 55 • 9 years ago

(Oh, never mind about that last "Also:" RE expanding the repeat(auto-*) expression -- it looks like that's required by https://drafts.csswg.org/css-grid/#resolved-track-list / bug 978212.)

	Daniel Holbert [:dholbert]
	Comment 56 • 9 years ago

Comment on attachment 8700709 [details] [diff] [review]
part 1 (style system part) - [css-grid] Implement the 'auto-fill' and 'auto-fit' keywords in the repeat() function.

Review of attachment 8700709 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsComputedDOMStyle.cpp
@@ +2488,5 @@
>    // Delimiting 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");
>    if (aTrackSizes) {

This function is a bit long now, with several nesting levels of logic. So, some comments for this top-level "if" clause & its "else" clause would be helpful for anchoring what's going on. (I didn't initially grok what "if (aTrackSizes)" was checking, since it's not clear from context whether this passed-in variable is from the style system or from layout or from something else.)

Maybe add something like this, just inside the clause for the "if" check:
  // We've done layout on the grid and have resolved the sizes of its tracks,
  // so we'll return those sizes here.  We'll also expand repeat(auto-*,...)
  // expressions, because their repeats might have different resolved sizes.

...and this just inside its "else" clause:
  // We haven't laid out the grid yet, or at least we don't have resolved
  // track-sizes. So, we'll just return a serialization of the tracks from
  // the style system (without resolved sizes).

@@ +2534,1 @@
>          break;

Maybe worth asserting that numTracks is >0 (higher up near where numTracks is declared), so it's easier to infer that this loop is definitely valid?

Right now it's structured basically like this:
 numTracks = array length;
 [...]
 for (int32_t i = 0;; i++) {
   // use "i" as an index into array
   [...]
   if (uint32_t(i) == numTracks) {
     break;
   }
 }

And clearly this loop structure is only valid if we're really sure that numTracks is nonzero (or else our array indexing would be bogus). So, worth stating that nonzero-length as an invariant up-front.

	Daniel Holbert [:dholbert]
	Comment 57 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #54)
> @@ +2466,5 @@
> > +        valueList->AppendCSSValue(start);
> > +        AppendGridLineNames(valueList, aTrackList.mRepeatAutoLineNameListBefore);
> > +        nsROCSSPrimitiveValue* end = new nsROCSSPrimitiveValue;
> > +        end->SetString(NS_LITERAL_STRING(")"));
> > +        valueList->AppendCSSValue(end);
> 
> (Since valueList is space-delimited, I think we'll end up with a trailing
> space before this close-paren, right? That seems slightly undesirable, but
> maybe not a big deal.)

To be clear, the trailing space I'm talking about comes from the fact that:
 * You're adding ")" as a distinct list-entry in this nsDOMCSSValueList
 * When nsDOMCSSValueList converts to a string, it inserts a separator (a space in this case) between each value in the list:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSValueList.cpp?rev=0b0c492a33b5&mark=61-61,84-84#49

The best way to avoid this, I think, is to build up the whole repeat(...) expression as a *single nsAutoString*, and create a *single nsROCSSPrimitiveValue* for that string, and put that one nsROCSSPrimitiveValue into our valueList. I think that makes more sense than having distinct list-entries for "repeat(..." and for ")".  But it'd would require refactoring your helper-functions a bit here.  I'll leave it up to you whether you think it's worth it.

	Daniel Holbert [:dholbert]
	Comment 58 • 9 years ago

Comment on attachment 8700712 [details] [diff] [review]
part 5 - [css-grid] Remove any empty 'repeat(auto-fit)' tracks at the end of its range and adjust affected grid area line numbers accordingly.

Review of attachment 8700712 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 5

::: layout/generic/nsGridContainerFrame.h
@@ +214,5 @@
> +    /**
> +     * Translate the lines to account for (empty) removed tracks.  This method
> +     * is only for abs.pos. children and should only be called after placement.
> +     * Same as for in-flow items, but we don't touch 'auto' lines here and we
> +     * also need to adjust areas that spans into the removed range.

Nit: s/spans/span/

(should say "areas that span")

	Pulsebot
	Comment 59 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0876a7e83d
https://hg.mozilla.org/integration/mozilla-inbound/rev/468e226d8dc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6c3bf97a9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2325b96d317
https://hg.mozilla.org/integration/mozilla-inbound/rev/c79ced4b22c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c159150b39f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80b38f71990
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e56586df39f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1c24beaa5b

	Mats Palmgren (inactive) Assignee
	Comment 60 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #54)
> Could we assert that aTrackList.mRepeatAutoLineNameListBefore is non-empty
> here, before we make this call? (Maybe we could assert this right after we
> assert that mRepeatAutoLineNameListAfter *is* empty, as a counterpoint to
> that assertion?)

Sure.

> (In the CSS grammar, the thing we're serializing here is...
>  <name-repeat> = repeat( [ <positive-integer> | auto-fill ], <line-names>+)
> ...and the "+" there means one-or-more, which means our name-list must be
> nonempty here.)

Correct.

> > +        end->SetString(NS_LITERAL_STRING(")"));
> > +        valueList->AppendCSSValue(end);
> 
> (Since valueList is space-delimited, I think we'll end up with a trailing
> space before this close-paren, right? That seems slightly undesirable, but
> maybe not a big deal.)

It doesn't seem worth fixing IMO.  The same problem might occur in
the non-subgrid case too, so we'd also need to rewrite GetGridTrackSize.
Feel free to file a bug if you think it's worth fixing.

> > +    int32_t endOfRepeat = 0;  // first index after any repeat() tracks
> > +    int32_t offsetToLastRepeat = 0;
> > +    if (aTrackList.HasRepeatAuto()) {
> > +      // offsetToLastRepeat is -1 if all repeat(auto-fit) tracks are empty
> > +      offsetToLastRepeat = numTracks + 1 - aTrackList.mLineNameLists.Length();
> 
> Did you mean for this comment to be "auto-fit" specific? (and exclude
> "auto-fill"?)

Yes, "auto-fill" always results in one or more tracks per spec.

> Also: This whole section seems to be expanding the repeat(auto-*, ...)
> expression to the actual line-names / line-sizes list.  So, the outputted
> string won't have a "repeat" expression at all.  Are you sure we want the
> expanded version here?

Yes, because the track sizes can differ.  It's only one of minmax(x,y)
that needs to be definite (which is used for determining the number of
tracks), but the other sizing function may be something that is
affected by the items in the track which may cause repeated tracks
to have different sizes.

> I was expecting that we'd just append the repeat() expression itself,

Note that we MUST output the actual "px" values here.

> (Note that for integer-valued "repeat" expressions, we'll output the
> expanded version; that's different, because the expanded version is
> unambiguously equivalent to the repeat() expression.

We have the same problem here with different sizes.

Overall, this doesn't seem worth doing.  I suspect that web
developers will strongly prefer the expanded version anyway,
over having to parse the repeat() notation themselves.


(In reply to Daniel Holbert [:dholbert] from comment #56)
> This function is a bit long now, with several nesting levels of logic. So,
> some comments for this top-level "if" clause & its "else" clause would be
> helpful for anchoring what's going on.

OK, I added a couple of comments there.

> Maybe worth asserting that numTracks is >0 (higher up near where numTracks
> is declared), so it's easier to infer that this loop is definitely valid?

numTracks can be zero here (for a single repeat(auto-fit)
track that resulted in zero tracks).

Hmm, now that I think about it, we should probably return 'none' in
that case, so I added an early return and assertion like so:

    MOZ_ASSERT(numTracks > 0 ||
               (aTrackList.HasRepeatAuto() && !aTrackList.mIsAutoFill),
               "only 'auto-fit' can result in zero tracks");
    if (numTracks == 0) {
      nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
      val->SetIdent(eCSSKeyword_none);
      return val;
    }

(trivial change so I don't think it needs another review)

> And clearly this loop structure is only valid if we're really sure that
> numTracks is nonzero (or else our array indexing would be bogus). So, worth
> stating that nonzero-length as an invariant up-front.

No, I think you're mistaken.  We index mLineNameLists which is guaranteed
to have at least two entries here.  ("grid-template-rows:1px" is
implicitly the same as "grid-template-rows:[] 1px []" so when there is
one *specified* track, then there will be two line lists in mLineNameLists.
mLineNameLists is empty only for 'none' which is checked earlier.)

	Mats Palmgren (inactive) Assignee
	Comment 61 • 9 years ago

(In reply to Daniel Holbert [:dholbert] from comment #58)
> Nit: s/spans/span/

Fixed.

	Pulsebot
	Comment 62 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c9b8523293

	yisibl
	Comment 63 • 9 years ago

@Mats Palmgren We not support " <flex> | min-content | max-content | auto" width auto-fill/fit?

e.g.

grid-template-columns: repeat(auto-fit, minmax(min-content, auto));

Test case: http://jsbin.com/colibiduju/edit?html,output

	Carsten Book [:Tomcat]
	Comment 64 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1f0876a7e83d
https://hg.mozilla.org/mozilla-central/rev/468e226d8dc8
https://hg.mozilla.org/mozilla-central/rev/bb6c3bf97a9c
https://hg.mozilla.org/mozilla-central/rev/f2325b96d317
https://hg.mozilla.org/mozilla-central/rev/c79ced4b22c8
https://hg.mozilla.org/mozilla-central/rev/c159150b39f5
https://hg.mozilla.org/mozilla-central/rev/d80b38f71990
https://hg.mozilla.org/mozilla-central/rev/1e56586df39f
https://hg.mozilla.org/mozilla-central/rev/5e1c24beaa5b
https://hg.mozilla.org/mozilla-central/rev/b6c9b8523293

	Mats Palmgren (inactive) Assignee
	Comment 65 • 8 years ago

(In reply to percyley from comment #63)
> @Mats Palmgren We not support " <flex> | min-content | max-content | auto"
> width auto-fill/fit?

No, you need to have a <fixed-size> there:
https://drafts.csswg.org/css-grid/#repeat-notation

	yisibl
	Comment 66 • 8 years ago

<auto-repeat>  = repeat( [ auto-fill | auto-fit ],   <line-names>? <fixed-size>    <line-names>? )
<fixed-size>       = <fixed-breadth> | minmax( <fixed-breadth> , <track-breadth> )
<track-breadth>    = <length> | <percentage> | <flex> | min-content | max-content | auto

So we should support more <track-breadth> features.

And in the Demo, if only the percentage and the fixed width are supported, it is too limited.

	Francesco Lodolo [:flod]
	Comment 67 • 8 years ago

PEMoreThanOneGridRepeatAutoFillInNameList=Only one repeat(auto-fill, ...) is allowed in a name list for a subgrid.
PEMoreThanOneGridRepeatAutoFillFitInTrackList=Only one repeat(auto-fill, ...) or repeat(auto-fit, ...) is allowed in a track list.
PEMoreThanOneGridRepeatTrackSize=Only one track size is allowed inside repeat(auto-fit/auto-fill, ...).

I suppose '...' must be intended as a generic ellipsis and not part of the syntax: in that case it would be useful to use … (one Unicode character) for consistency with the other strings in Mozilla products. Also no need to use new string IDs for the change.

	yisibl
	Comment 68 • 8 years ago

Chrome(Blink) adds the parsing support. https://code.google.com/p/chromium/issues/detail?id=579104#c2