[css-grid] Implement auto-repeat computation

[css-grid] Implement auto-repeat computation

We added support for parsing and style for the new auto-repeat syntax. This
CL actually implements the feature by computing the number of auto-repeat
tracks that will be part of the explicit grid depending on the available
size on the corresponding axis.

Note that this CL does not drop the empty tracks, i.e., auto-fit will work
exactly as auto-fill until the empty track removal is implemented.

Test results are not totally correct yet because we need to add the line
names to the computed style. As that requires a rather large piece of code
it will be done in a follow up patch. Track sizes are correct though.

BUG=589460
Committed: https://crrev.com/7b1b0f17c7a1eac0283fc9daa519328a42e0139f
Cr-Commit-Position: refs/heads/master@{#389456}

[svillar, 2016-04-18 07:51:46]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, rego@igalia.com, rune@opera.com, timloh@chromium.org

[svillar, 2016-04-18 07:51:47]

Sending out for review

[Manuel Rego, 2016-04-18 21:31:42]

This is a first review with several comments.

I'm missing more tests (we're only testing the computed style),
as the basics of this feature are already working with this patch.
For example, I miss a simple test checking that items are layout
properly using repeat.
Also we've a lot of code to deal with minimum and maximum sizes
that we're not testing at all.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt:7:
FAIL getComputedStyle(element, '').getPropertyValue('grid-template-columns')
should be [foo bar] 200px [foo bar] 200px [foo bar] 200px [foo bar] 200px. Was
200px 200px 200px 200px.
I'm not sure if it's a good idea to add the name lines
if they're not supported yet.
Wouldn't be better to have the test without name lines,
until we support them?

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt:19:
FAIL getComputedStyle(element, '').getPropertyValue('grid-template-columns')
should be none. Was 150px 150px 150px 150px 150px.
Like in the previous case, I'm not sure if we want to expect "none"
in this test.
I understand that would be the result once we implement "auto-fit",
but right now I'd prefer to have a PASS with the wrong result and
a TODO, that the "none" (it's pretty confusing if you read the test).

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:2:
<link href="resources/grid.css" rel="stylesheet">
This is unrelated to this patch, but it seems we don't use grid.css in this
test.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:12:
testGridDefinitionsSetJSValues("repeat(auto-fill, [foo bar] 200px)",
"repeat(auto-fill, 20em [foo bar])", "[foo bar] 200px [foo bar] 200px [foo bar]
200px [foo bar] 200px", "200px [foo bar] 200px [foo bar] 200px [foo bar]");
Generic comment, I'm not sure if using "em" unit is a good idea,
isn't the size of 1em relative to the font size?

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:305: || (diff.needsLayout()
&& (styleRef().gridAutoRepeatColumns().size() ||
styleRef().gridAutoRepeatRows().size())))
We don't have a test for this.

BTW, why you need the diff.needsLayout()?

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:316: ||
oldStyle.gridAutoRepeatRows().size() != styleRef().gridAutoRepeatRows().size();
We don't have a test for this either.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:700: const GridTrackSize&
LayoutGrid::unresolvedGridTrackSize(GridTrackSizingDirection direction, int i)
const
We could find a better name than "i" for the variable. :-)

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:702: bool isForColumns =
direction == ForColumns;
Probably better use "isRowAxis" as in most of the code.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:711: // grid-template-areas
is specified for example).
But explicitGridXXXCount() now already manages
the grid-template-areas issues.
I'm not sure if I understand completely this comment,
or it's an outdated comment
(before http://crrev.com/1885913002).

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:717: if
(LIKELY(!repetitions) || i < insertionPoint)
Wow, first time I see LIKELY in a patch. :-)

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:728: int translatedIndex =
i + (direction == ForColumns ? m_smallestColumnStart : m_smallestRowStart);
You could do this inside unresolvedGridTrackSize() as it's only used there.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1219:
DCHECK(autoTrackSize.minTrackBreadth().isLength() &&
!autoTrackSize.minTrackBreadth().isContentSized());
Usually it's preferred to split this in 2 lines,
to get the right error directly if some of the conditions fail.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1221: const Length& minSize
= isRowAxis ? styleRef().logicalMinWidth() : styleRef().logicalMinHeight();
This should be moved below where it's first used.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1224: bool
needsToFulfillMinimumSize = false;
Ditto.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1232: availableSize =
availableSize != -1 ? std::min(availableSize, maxSize) : maxSize;
"availableSize" should be -1 if we are in this part of the if.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1275: size_t repetitions =
1 + (freeSpace / (autoRepeatTrackSize + gapSize)).toInt();
I'm not sure if I get this, if you've for example
freeSpace = 600px and autoRepeatTrackSize = 200px (without gaps)
I guess the number of repetitions should be 3,
but this calculation will be 1 + 600/200 = 4.

What I'm missing here?

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1281: ++repetitions;
Again, I don't get why in this case we've to increment
the number of repetitions.
If the grid is using min-width: 600px and width: min-content,
I guess this "needsToFulfillMinimumSize" will be true
and we'll be incrementing the number of repetitions
calculated above.

Also I'm not sure if the comment is 100% accurate,
it seems that the grid container needs to be sized
under a min-content constraint (or something like that)
and not just have a definite min size.
If it has width: 800px and min-width: 600px,
I understand it should enter in this if.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1283: return
std::min<size_t>(repetitions, kGridMaxTracks);
I guess the limit for kGridMaxTracks should be
repetitions + trackSizes.size().
Not sure if really important anyway.

[svillar, 2016-04-19 08:09:36]

Thanks for the nice review.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt:7:
FAIL getComputedStyle(element, '').getPropertyValue('grid-template-columns')
should be [foo bar] 200px [foo bar] 200px [foo bar] 200px [foo bar] 200px. Was
200px 200px 200px 200px.
On 2016/04/18 21:31:41, Manuel Rego wrote:
> I'm not sure if it's a good idea to add the name lines
> if they're not supported yet.
> Wouldn't be better to have the test without name lines,
> until we support them?

I think both are valid alternatives. I don't strongly feel about any.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:2:
<link href="resources/grid.css" rel="stylesheet">
On 2016/04/18 21:31:41, Manuel Rego wrote:
> This is unrelated to this patch, but it seems we don't use grid.css in this
> test.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:12:
testGridDefinitionsSetJSValues("repeat(auto-fill, [foo bar] 200px)",
"repeat(auto-fill, 20em [foo bar])", "[foo bar] 200px [foo bar] 200px [foo bar]
200px [foo bar] 200px", "200px [foo bar] 200px [foo bar] 200px [foo bar]");
On 2016/04/18 21:31:41, Manuel Rego wrote:
> Generic comment, I'm not sure if using "em" unit is a good idea,
> isn't the size of 1em relative to the font size?

It is, and we use it in many cases. What I could do is to explicitly define the
size of the font.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:305: || (diff.needsLayout()
&& (styleRef().gridAutoRepeatColumns().size() ||
styleRef().gridAutoRepeatRows().size())))
On 2016/04/18 21:31:42, Manuel Rego wrote:
> We don't have a test for this.

ACK

> BTW, why you need the diff.needsLayout()?

Because we don't want to dirty the grid every time there is a style change when
we have autorepeat tracks _unless_ the size have changed.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:316: ||
oldStyle.gridAutoRepeatRows().size() != styleRef().gridAutoRepeatRows().size();
On 2016/04/18 21:31:41, Manuel Rego wrote:
> We don't have a test for this either.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:700: const GridTrackSize&
LayoutGrid::unresolvedGridTrackSize(GridTrackSizingDirection direction, int i)
const
On 2016/04/18 21:31:42, Manuel Rego wrote:
> We could find a better name than "i" for the variable. :-)

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:702: bool isForColumns =
direction == ForColumns;
On 2016/04/18 21:31:42, Manuel Rego wrote:
> Probably better use "isRowAxis" as in most of the code.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:711: // grid-template-areas
is specified for example).
On 2016/04/18 21:31:42, Manuel Rego wrote:
> But explicitGridXXXCount() now already manages
> the grid-template-areas issues.
> I'm not sure if I understand completely this comment,
> or it's an outdated comment
> (before http://crrev.com/1885913002).

It's perfectly up to date :). So the thing is that we don't want the number of
explicit tracks but just the size of the trackStyles vector because what we want
is to know where to find the track size. Example:

grid-template-areas: "a b c"
grid-template-columns: 20px;
grid-auto-columns: 10%;

If we ask for the 2nd column (index 1), we should return the auto track because
1>=trackStyles.size(). If we had used the number of explicit tracks we could end
up trying to access trackStyles[1] -> segmentation fault.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:717: if
(LIKELY(!repetitions) || i < insertionPoint)
On 2016/04/18 21:31:42, Manuel Rego wrote:
> Wow, first time I see LIKELY in a patch. :-)

Not sure if this is a complain or what :). There are not a lot of them but it's
used sometimes.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:728: int translatedIndex =
i + (direction == ForColumns ? m_smallestColumnStart : m_smallestRowStart);
On 2016/04/18 21:31:42, Manuel Rego wrote:
> You could do this inside unresolvedGridTrackSize() as it's only used there.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1219:
DCHECK(autoTrackSize.minTrackBreadth().isLength() &&
!autoTrackSize.minTrackBreadth().isContentSized());
On 2016/04/18 21:31:42, Manuel Rego wrote:
> Usually it's preferred to split this in 2 lines,
> to get the right error directly if some of the conditions fail.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1221: const Length& minSize
= isRowAxis ? styleRef().logicalMinWidth() : styleRef().logicalMinHeight();
On 2016/04/18 21:31:42, Manuel Rego wrote:
> This should be moved below where it's first used.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1224: bool
needsToFulfillMinimumSize = false;
On 2016/04/18 21:31:42, Manuel Rego wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1232: availableSize =
availableSize != -1 ? std::min(availableSize, maxSize) : maxSize;
On 2016/04/18 21:31:42, Manuel Rego wrote:
> "availableSize" should be -1 if we are in this part of the if.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1275: size_t repetitions =
1 + (freeSpace / (autoRepeatTrackSize + gapSize)).toInt();
On 2016/04/18 21:31:42, Manuel Rego wrote:
> I'm not sure if I get this, if you've for example
> freeSpace = 600px and autoRepeatTrackSize = 200px (without gaps)
> I guess the number of repetitions should be 3,
> but this calculation will be 1 + 600/200 = 4.
> 
> What I'm missing here?

You're missing the freeSpace computation just a couple of lines above. As
tracksSize is at least the size of 1 of the autorepeat tracks, the freeSpace
would be 600-200 = 400.

Then the value of repetitions is 1 + (400/200) = 3.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1281: ++repetitions;
On 2016/04/18 21:31:42, Manuel Rego wrote:
> Again, I don't get why in this case we've to increment
> the number of repetitions.
> If the grid is using min-width: 600px and width: min-content,
> I guess this "needsToFulfillMinimumSize" will be true
> and we'll be incrementing the number of repetitions
> calculated above.

Because in the above code we compute the maximum number of repetitions that does
not overflow the container. This means that 1 more repetition will be enough to
fulfill the minimum size requirement.

> Also I'm not sure if the comment is 100% accurate,
> it seems that the grid container needs to be sized
> under a min-content constraint (or something like that)
> and not just have a definite min size.
> If it has width: 800px and min-width: 600px,
> I understand it should enter in this if.

Well the comment is a literal transcription from the specs, so if you think it
is not accurate you should send an editorial comment :). What the specs say is:

1- if you have definite size or max size -> compute the largest number that does
not overflow
2- otherwise (and it explicitly says otherwise) if the min size is definite then
compute the minimum value that fulfills that requirement.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1283: return
std::min<size_t>(repetitions, kGridMaxTracks);
On 2016/04/18 21:31:42, Manuel Rego wrote:
> I guess the limit for kGridMaxTracks should be
> repetitions + trackSizes.size().
> Not sure if really important anyway.

Yeah good catch.

[svillar, 2016-04-19 08:19:23]

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt:19:
FAIL getComputedStyle(element, '').getPropertyValue('grid-template-columns')
should be none. Was 150px 150px 150px 150px 150px.
On 2016/04/18 21:31:41, Manuel Rego wrote:
> Like in the previous case, I'm not sure if we want to expect "none"
> in this test.
> I understand that would be the result once we implement "auto-fit",
> but right now I'd prefer to have a PASS with the wrong result and
> a TODO, that the "none" (it's pretty confusing if you read the test).

BTW I have to disagree with this. A FAIL is pretty explicit. A PASS with a TODO
in another file is not at all.

[Manuel Rego, 2016-04-19 11:37:30]

Thanks for the explanations, now I got things better.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt:19:
FAIL getComputedStyle(element, '').getPropertyValue('grid-template-columns')
should be none. Was 150px 150px 150px 150px 150px.
On 2016/04/19 08:19:23, svillar wrote:
> On 2016/04/18 21:31:41, Manuel Rego wrote:
> > Like in the previous case, I'm not sure if we want to expect "none"
> > in this test.
> > I understand that would be the result once we implement "auto-fit",
> > but right now I'd prefer to have a PASS with the wrong result and
> > a TODO, that the "none" (it's pretty confusing if you read the test).
> 
> BTW I have to disagree with this. A FAIL is pretty explicit. A PASS with a
TODO
> in another file is not at all.

Of course it's a personal preference, the TODO can even be in the test. :-)

In my case, if I read a test, I expect everything to be "PASS"
unless I find some special comment like a TODO.

Anyway, in this case it's a temporal situation, that hopefully won't last
for a long time (next patches will be coming soon),
so I don't have a strong opinion either.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html
(right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:12:
testGridDefinitionsSetJSValues("repeat(auto-fill, [foo bar] 200px)",
"repeat(auto-fill, 20em [foo bar])", "[foo bar] 200px [foo bar] 200px [foo bar]
200px [foo bar] 200px", "200px [foo bar] 200px [foo bar] 200px [foo bar]");
On 2016/04/19 08:09:35, svillar wrote:
> On 2016/04/18 21:31:41, Manuel Rego wrote:
> > Generic comment, I'm not sure if using "em" unit is a good idea,
> > isn't the size of 1em relative to the font size?
> 
> It is, and we use it in many cases. What I could do is to explicitly define
the
> size of the font.

Ok, that would be nice.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:711: // grid-template-areas
is specified for example).
On 2016/04/19 08:09:35, svillar wrote:
> On 2016/04/18 21:31:42, Manuel Rego wrote:
> > But explicitGridXXXCount() now already manages
> > the grid-template-areas issues.
> > I'm not sure if I understand completely this comment,
> > or it's an outdated comment
> > (before http://crrev.com/1885913002).
> 
> It's perfectly up to date :). So the thing is that we don't want the number of
> explicit tracks but just the size of the trackStyles vector because what we
want
> is to know where to find the track size. Example:
> 
> grid-template-areas: "a b c"
> grid-template-columns: 20px;
> grid-auto-columns: 10%;
> 
> If we ask for the 2nd column (index 1), we should return the auto track
because
> 1>=trackStyles.size(). If we had used the number of explicit tracks we could
end
> up trying to access trackStyles[1] -> segmentation fault.

Yeah I read the comment wrong, I got it now. Thanks.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:717: if
(LIKELY(!repetitions) || i < insertionPoint)
On 2016/04/19 08:09:36, svillar wrote:
> On 2016/04/18 21:31:42, Manuel Rego wrote:
> > Wow, first time I see LIKELY in a patch. :-)
> 
> Not sure if this is a complain or what :). There are not a lot of them but
it's
> used sometimes.

Just a funny comment, sorry. :-)

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1275: size_t repetitions =
1 + (freeSpace / (autoRepeatTrackSize + gapSize)).toInt();
On 2016/04/19 08:09:36, svillar wrote:
> On 2016/04/18 21:31:42, Manuel Rego wrote:
> > I'm not sure if I get this, if you've for example
> > freeSpace = 600px and autoRepeatTrackSize = 200px (without gaps)
> > I guess the number of repetitions should be 3,
> > but this calculation will be 1 + 600/200 = 4.
> > 
> > What I'm missing here?
> 
> You're missing the freeSpace computation just a couple of lines above. As
> tracksSize is at least the size of 1 of the autorepeat tracks, the freeSpace
> would be 600-200 = 400.
> 
> Then the value of repetitions is 1 + (400/200) = 3.

Thanks again.

https://codereview.chromium.org/1895443002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1281: ++repetitions;
On 2016/04/19 08:09:36, svillar wrote:
> On 2016/04/18 21:31:42, Manuel Rego wrote:
> > Again, I don't get why in this case we've to increment
> > the number of repetitions.
> > If the grid is using min-width: 600px and width: min-content,
> > I guess this "needsToFulfillMinimumSize" will be true
> > and we'll be incrementing the number of repetitions
> > calculated above.
> 
> Because in the above code we compute the maximum number of repetitions that
does
> not overflow the container. This means that 1 more repetition will be enough
to
> fulfill the minimum size requirement.

And thanks again, I was not getting it.

> > Also I'm not sure if the comment is 100% accurate,
> > it seems that the grid container needs to be sized
> > under a min-content constraint (or something like that)
> > and not just have a definite min size.
> > If it has width: 800px and min-width: 600px,
> > I understand it should enter in this if.
> 
> Well the comment is a literal transcription from the specs, so if you think it
> is not accurate you should send an editorial comment :). What the specs say
is:
> 
> 1- if you have definite size or max size -> compute the largest number that
does
> not overflow
> 2- otherwise (and it explicitly says otherwise) if the min size is definite
then
> compute the minimum value that fulfills that requirement.

The thing is that the comment on the spec has both parts 1 and 2.
Here we only have the "Otherwise, ..." so it's harder to read,
as we don't have the first part explaining that if the size or max-size
is definite we use that one.

[svillar, 2016-04-19 13:58:08]

BTW, regarding having more tests, I've just realized that I haven't included all
the mozilla tests I'm importing plus one more test I have developed for this
bug. Will upload them all along with the suggested changes.

[Manuel Rego, 2016-04-19 17:22:16]

A few more comments.

BTW, apart from the warnings in the compiler 
(as you can see in the trybot),
in my machine 3 tests are not passing:
  fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-002.html [ Failure ]
  fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003.html [ Failure ]
  fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-004.html [ Failure ]

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:17:
.gridMultipleNames { grid-template: 25px / [start] 20px [foo] 50%
repeat(auto-fill, [bar] 20px [start foo]) [foo] 10% [end bar]; }
This is a super-nit but these lines are quite long.
I'd prefer to add new lines :-).

Also "grid-template" is about to die, I suggest to change it for "grid".

Last, there're no tests for "auto-fill" in the column axis,
shouldn't we add some cases too?

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:21:
min-height: 100%;
Do we really need this min-height: 100%?

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:38:
<p>This test checks that repeat(auto-fill, ) syntax works as expected</p>
Nit: Missing dot at the end.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:45: <div
class="item" style="grid-column: 1 / span 6 autobar" data-offset-x="0"
data-offset-y="0" data-expected-width="180" data-expected-height="25"></div>
This will have the same result even if we use an unknown line,
or lines were not properly resolved.
E.g.: grid-column: 1 / span 6 foobar;

I'd set "grid-auto-columns", so we'll be sure that
the lines are being resolved.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:49: <div
class="item" style="grid-column: 1 / span 5" data-offset-x="0" data-offset-y="0"
data-expected-width="200" data-expected-height="25"></div>
Do we actually have 5 auto-repeat columns?

I think we only have 4 columns:
30 + 20 + 30 + 20 + 30 + 20 + 30 = 180

If we'd have 5, the result would be:
30 + 20 + 30 + 20 + 30 + 20 + 30 + 20 + 30 = 230

This seems to be working fine because of we don't have
"grid-auto-columns", otherwise the result would be different.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:53: <div
class="item" style="grid-column: autobar 2 / span 3" data-offset-x="100"
data-offset-y="0" data-expected-width="100" data-expected-height="25"></div>
I'd use span 2, as we only have 4 auto-repeat columns.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:66:
</div>
Again this won't fail if we use "5 myfoobarline".

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:69: <div
class="item" style="grid-column: 1 / span 5" data-offset-x="0" data-offset-y="0"
data-expected-width="200" data-expected-height="25"></div>
Again, I believe we only have 4 columns in total.
I'll stop here reviewing the test until you add "grid-auto-columns". :-)

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html:125:
<!-- <fit>
Maybe add a TODO here,
and in the rest of tests where you disable auto-fit.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003-expected.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003-expected.html:27:
/* Original Mozilla tests set a fixed width for their grids, that's likely due
to some bug they have */
Nit: Missing dot at the end of the sentence.

BTW, have you checked if it passes without fixed widths
in Firefox? Maybe it's not needed for them either.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-004-expected.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-004-expected.html:27:
/* Original Mozilla tests set a fixed width for their grids, that's likely due
to some bug they have */
Ditto.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:305: || (diff.needsLayout()
&& (styleRef().gridAutoRepeatColumns().size() ||
styleRef().gridAutoRepeatRows().size())))
We still don't have tests for this.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:316: ||
oldStyle.gridAutoRepeatRows().size() != styleRef().gridAutoRepeatRows().size();
Ditto.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:700: const GridTrackSize&
LayoutGrid::unresolvedGridTrackSize(GridTrackSizingDirection direction, size_t
untranslatedIndex) const
I've just realized that "untranslatedIndex" should be an int.
Until we translate them we cannot be sure they're >= 0.

Nit: I'm not sure about the name of the method,
as we usually use the term "resolved" related to positions.
In this case it's related to sizes.
However, I don't have a better proposal. :-(

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1222: LayoutUnit
availableSize = isRowAxis ? availableLogicalWidth() :
computeContentLogicalHeight(MainOrPreferredSize, styleRef().logicalHeight(),
LayoutUnit(-1));
Couldn't you use availableLogicalHeight()?

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1246: availableSize =
valueForLength(minSize, containingBlockAvailableSize);
Why the difference to get the max vs min size.

A few lines before you use a different method to
compute "maxLength", I'm wondering why you use
something different to compute "minSize".

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1256: // There will be
always at least 1 auto-repeat track, so take it already into account when
computing the total track size
Nit: Missing dot at the end.

[svillar, 2016-04-20 08:24:18]

Regarding the tests for the rows, the mozilla tests are full of them, do you
think we should have more?

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:17:
.gridMultipleNames { grid-template: 25px / [start] 20px [foo] 50%
repeat(auto-fill, [bar] 20px [start foo]) [foo] 10% [end bar]; }
On 2016/04/19 17:22:15, Manuel Rego wrote:
> This is a super-nit but these lines are quite long.
> I'd prefer to add new lines :-).
> 
> Also "grid-template" is about to die, I suggest to change it for "grid".
> 
> Last, there're no tests for "auto-fill" in the column axis,
> shouldn't we add some cases too?

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:21:
min-height: 100%;
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Do we really need this min-height: 100%?

It's just to stretch the items in the column axis but it will be likely done
anyway yes.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:45: <div
class="item" style="grid-column: 1 / span 6 autobar" data-offset-x="0"
data-offset-y="0" data-expected-width="180" data-expected-height="25"></div>
On 2016/04/19 17:22:15, Manuel Rego wrote:
> This will have the same result even if we use an unknown line,
> or lines were not properly resolved.
> E.g.: grid-column: 1 / span 6 foobar;
> 
> I'd set "grid-auto-columns", so we'll be sure that
> the lines are being resolved.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:49: <div
class="item" style="grid-column: 1 / span 5" data-offset-x="0" data-offset-y="0"
data-expected-width="200" data-expected-height="25"></div>
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Do we actually have 5 auto-repeat columns?
> 
> I think we only have 4 columns:
> 30 + 20 + 30 + 20 + 30 + 20 + 30 = 180
> 
> If we'd have 5, the result would be:
> 30 + 20 + 30 + 20 + 30 + 20 + 30 + 20 + 30 = 230
> 
> This seems to be working fine because of we don't have
> "grid-auto-columns", otherwise the result would be different.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:66:
</div>
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Again this won't fail if we use "5 myfoobarline".

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill.html:69: <div
class="item" style="grid-column: 1 / span 5" data-offset-x="0" data-offset-y="0"
data-expected-width="200" data-expected-height="25"></div>
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Again, I believe we only have 4 columns in total.
> I'll stop here reviewing the test until you add "grid-auto-columns". :-)

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html:125:
<!-- <fit>
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Maybe add a TODO here,
> and in the rest of tests where you disable auto-fit.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003-expected.html
(right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003-expected.html:27:
/* Original Mozilla tests set a fixed width for their grids, that's likely due
to some bug they have */
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Nit: Missing dot at the end of the sentence.
> 
> BTW, have you checked if it passes without fixed widths
> in Firefox? Maybe it's not needed for them either.

Yes they need it because they don't compute the auto-repeat tracks correctly. I
double-checked this particular example with fantasai and her answer was that
auto-fill should be the same that repeat with the computed number of
repetitions.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:305: || (diff.needsLayout()
&& (styleRef().gridAutoRepeatColumns().size() ||
styleRef().gridAutoRepeatRows().size())))
On 2016/04/19 17:22:15, Manuel Rego wrote:
> We still don't have tests for this.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:316: ||
oldStyle.gridAutoRepeatRows().size() != styleRef().gridAutoRepeatRows().size();
On 2016/04/19 17:22:15, Manuel Rego wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:700: const GridTrackSize&
LayoutGrid::unresolvedGridTrackSize(GridTrackSizingDirection direction, size_t
untranslatedIndex) const
On 2016/04/19 17:22:16, Manuel Rego wrote:
> I've just realized that "untranslatedIndex" should be an int.
> Until we translate them we cannot be sure they're >= 0.

Perhaps the name is not good enough but we're passing a size_t from
gridTrackSize() directly, so we are sure that it's an unsigned.

> Nit: I'm not sure about the name of the method,
> as we usually use the term "resolved" related to positions.
> In this case it's related to sizes.
> However, I don't have a better proposal. :-(

I don't like the name either.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1222: LayoutUnit
availableSize = isRowAxis ? availableLogicalWidth() :
computeContentLogicalHeight(MainOrPreferredSize, styleRef().logicalHeight(),
LayoutUnit(-1));
On 2016/04/19 17:22:16, Manuel Rego wrote:
> Couldn't you use availableLogicalHeight()?

No because it does not return -1 for indefinite heights, it instead returns the
block size of the container in those cases (check availableLogicalHeightUsing())

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1246: availableSize =
valueForLength(minSize, containingBlockAvailableSize);
On 2016/04/19 17:22:16, Manuel Rego wrote:
> Why the difference to get the max vs min size.
> 
> A few lines before you use a different method to
> compute "maxLength", I'm wondering why you use
> something different to compute "minSize".

I'm mostly doing the same. I could have used computeLogicalXXX but since I'm
pretty sure the minSize is specified (otherwise I would have returned above) I
can directly call valueForLength() which is what computeLogical() would do.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1256: // There will be
always at least 1 auto-repeat track, so take it already into account when
computing the total track size
On 2016/04/19 17:22:16, Manuel Rego wrote:
> Nit: Missing dot at the end.

Acknowledged.

[Manuel Rego, 2016-04-20 09:57:44]

On 2016/04/20 08:24:18, svillar wrote:
> Regarding the tests for the rows, the mozilla tests are full of them, do you
> think we should have more?

Then, why are we adding a new test custom test?
fast/css-grid-layout/grid-auto-fill.html

If we add it, I'd prefer to check both columns and rows
than only columns.

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:700: const GridTrackSize&
LayoutGrid::unresolvedGridTrackSize(GridTrackSizingDirection direction, size_t
untranslatedIndex) const
On 2016/04/20 08:24:18, svillar wrote:
> On 2016/04/19 17:22:16, Manuel Rego wrote:
> > I've just realized that "untranslatedIndex" should be an int.
> > Until we translate them we cannot be sure they're >= 0.
> 
> Perhaps the name is not good enough but we're passing a size_t from
> gridTrackSize() directly, so we are sure that it's an unsigned.

Yeah, so I got confused because the names are (and were) wrong.
I know I added this code in the past so sorry about that.

The thing is that gridTrackSize() always receive a translatedIndex,
so a size_t.
And here we need the untranslatedIndex back (an int),
so we do the computation.

So we should rename them. The param received by both
gridTrackSize and unresolvedGridTrackSize() (a size_t)
should be called "translatedIndex".
And the thing we calculate below, should be called
"untranslatedIndex" (an int).

> > Nit: I'm not sure about the name of the method,
> > as we usually use the term "resolved" related to positions.
> > In this case it's related to sizes.
> > However, I don't have a better proposal. :-(
> 
> I don't like the name either.

Just a quick idea uncomputedGridTrackSize(),
as we calculate track sizes in computeUsedBreadthOfGridTracks().

[svillar, 2016-04-21 14:13:59]

Patchset #5 (id:80001) has been deleted

[svillar, 2016-04-21 14:16:29]

Patch should be ready for review now. I've uploaded all the missing
tests&expectations and fixed also the compile warnings that were making the
trybots unhappy.

[Manuel Rego, 2016-04-22 12:57:05]

Non-owner LGTM.

I've some minor comments (mostly on the tests).

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html
(right):

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html:17:
.gridAutoRepeatAndFixedAfter { grid-template-columns: repeat(auto-fill, [first]
30px [last]) [foo] minmax(60px, 80px) [bar] minmax(45px, max-content); }
We don't have any content, so this "max-contet" is not really useful.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html:23:
min-height: 100%;
Again I believe we don't need this.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html:28:
grid-row-gap: 15px;
grid-row-gap is not needed with only 1 row.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html:43:
<div class="item" style="grid-column: 1 / span 6" data-offset-x="0"
data-offset-y="0" data-expected-width="180" data-expected-height="25"></div>
Super-nit: In the first test of each case you're testing
from the first to the last line.
What about using: "grid-column: 1 / -1"
(so we don't have to count)?

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html:79:
<div class="item" style="grid-column: notPresent / 3 autofoo"
data-offset-x="377" data-offset-y="0" data-expected-width="157"
data-expected-height="25"></div>
Dunno if we really need such complex cases.
Here both "notPresent" and "3 autofoo" resolve to the same line 6.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-columns.html:91:
<div class="item" style="grid-column: last 2 / foo" data-offset-x="60"
data-offset-y="0" data-expected-width="80" data-expected-height="25"></div>
Again this is pretty confusing "last 2" anf "foo" are the same line 3.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html
(right):

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fill-rows.html:1:
<!DOCTYPE html>
I like the idea that you just use the same values than
in the columns test but switching the axis.

So, the same comments than the ones in the previous test
apply here. :-)

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-auto-repeat-tracks.html
(right):

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-auto-repeat-tracks.html:26:
background-color: red;
I'd use a different color and not red
as it's not an error.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-auto-repeat-tracks.html:41:
<!-- Explicitly not using layout-th because it does not allow multiple
checkLayout(). -->
Not sure if this comment is needed, but ok.
Weird that you cannot call several times checkLayout() now.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-auto-repeat-tracks.html:89:
setGridTemplate('grid1', '', 'none');
Nit: '' and 'none' are the same,
maybe just use one of them all over the place.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-auto-repeat-tracks.html:102:
testGridDefinitions({ 'id': 'i21', 'width': '25', 'height': '45', 'x': '0', 'y':
'0' }, { 'id': 'i22', 'width': '25', 'height': '45', 'x': '25', 'y': '0' }, {
'id': 'i23', 'width': '25', 'height': '45', 'x': '50', 'y': '0' });
Just being picky, in all the cases you're actually changing
the number of tracks defined by grid-template-columns|rows.

I miss one test just changing the size of the auto-repeat track.
For example:
 repeat(auto-fill, 25px) => 2 columns
and then:
 repeat(auto-fill, 20px) => 2 columns but different sizes
and:
  repeat(auto-fill, 10px) => 5 columns

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html
(right):

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html:125:
<!-- <fit>
You forgot about adding a TODO here
(and in the rest of the tests too). :-)

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:719: if (untranslatedIndex
>= explicitTracksCount)
You can do merge this if with the one above.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:731: GridTrackSize
LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size_t i) const
Maybe we can take advantage of this patch
to rename this "i" to "translatedIndex" too.

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1281: // minimum
requirement.
Nit: I still think it'd be nice to explain the part
before the "Otherwise".

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/1895443002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.h:159: const GridTrackSize&
rawGridTrackSize(GridTrackSizingDirection, size_t) const;
Nice name. :-)

[svillar, 2016-04-25 08:57:45]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-04-25 08:57:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com
Link to the patchset: https://codereview.chromium.org/1895443002/#ps120001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-04-25 08:57:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895443002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895443002/120001

[commit-bot: I haz the power, 2016-04-25 12:33:04]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-04-25 12:34:48]

Description was changed from

==========
[css-grid] Implement auto-repeat computation

We added support for parsing and style for the new auto-repeat syntax. This
CL actually implements the feature by computing the number of auto-repeat
tracks that will be part of the explicit grid depending on the available
size on the corresponding axis.

Note that this CL does not drop the empty tracks, i.e., auto-fit will work
exactly as auto-fill until the empty track removal is implemented.

Test results are not totally correct yet because we need to add the line
names to the computed style. As that requires a rather large piece of code
it will be done in a follow up patch. Track sizes are correct though.

BUG=589460
==========

to

==========
[css-grid] Implement auto-repeat computation

We added support for parsing and style for the new auto-repeat syntax. This
CL actually implements the feature by computing the number of auto-repeat
tracks that will be part of the explicit grid depending on the available
size on the corresponding axis.

Note that this CL does not drop the empty tracks, i.e., auto-fit will work
exactly as auto-fill until the empty track removal is implemented.

Test results are not totally correct yet because we need to add the line
names to the computed style. As that requires a rather large piece of code
it will be done in a follow up patch. Track sizes are correct though.

BUG=589460

Committed: https://crrev.com/7b1b0f17c7a1eac0283fc9daa519328a42e0139f
Cr-Commit-Position: refs/heads/master@{#389456}
==========

[commit-bot: I haz the power, 2016-04-25 12:34:50]

Patchset 6 (id:??) landed as
https://crrev.com/7b1b0f17c7a1eac0283fc9daa519328a42e0139f
Cr-Commit-Position: refs/heads/master@{#389456}

[Charlie Reis, 2016-04-25 16:49:25]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1920953002/ by creis@chromium.org.

The reason for reverting is: Appeared to cause lots of fast/css-grid-layout test
failures on WebKit Linux MSAN:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b....