[css-grid] Add parsing support for <auto-repeat> syntax

[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).

BUG=579104
Committed: https://crrev.com/7647a0d4b25582406b03216f63f4906bd3c46d22
Cr-Commit-Position: refs/heads/master@{#370951}

[svillar, 2016-01-13 11:48:03]

svillar@igalia.com changed reviewers:
+ eae@chromium.org, kouhei@chromium.org, skobes@chromium.org

[svillar, 2016-01-13 11:48:04]

Sending out for review

[svillar, 2016-01-13 14:56:21]

Description was changed from

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).
==========

to

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).
==========

[svillar, 2016-01-13 14:56:21]

svillar@igalia.com changed reviewers:
+ timloh@chromium.org

[Manuel Rego, 2016-01-19 16:37:47]

rego@igalia.com changed reviewers:
+ rego@igalia.com

[Manuel Rego, 2016-01-19 16:37:47]

Non-owner LGTM.

I've created a bug to track the implementation of auto repeat,
please link it in the description: http://crbug.com/579104

https://codereview.chromium.org/1582793002/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/1582793002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:9:
description('Test that setting and getting grid-template-columns and
grid-template-rows with repeat() works as expected');
Nit: I'd mention "auto repeat" here.

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2355: //
minimum track sizes in order to compute the number of repetitions.
Nit: The 2nd sentence doesn't apply here.

The comment that it can only appear once seems enough.

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2422: if
(isAutoRepeat && numberOfTracks)
Maybe a comment explaining that <auto-repeat>
can only define 1 track.

[eae, 2016-01-19 22:47:48]

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2385: :
*toCSSPrimitiveValue(toCSSFunctionValue(*value).item(0));
Is this guaranteed to be non-null? Mixing references and pointers like this
makes me nervous.

[svillar, 2016-01-20 14:38:34]

Description was changed from

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).
==========

to

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).

BUG=579104
==========

[svillar, 2016-01-21 12:25:15]

https://codereview.chromium.org/1582793002/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/1582793002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html:9:
description('Test that setting and getting grid-template-columns and
grid-template-rows with repeat() works as expected');
On 2016/01/19 16:37:47, Manuel Rego wrote:
> Nit: I'd mention "auto repeat" here.

Yeah, the description was likely copypasted :)

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2355: //
minimum track sizes in order to compute the number of repetitions.
On 2016/01/19 16:37:47, Manuel Rego wrote:
> Nit: The 2nd sentence doesn't apply here.
> 
> The comment that it can only appear once seems enough.

Acknowledged.

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2385: :
*toCSSPrimitiveValue(toCSSFunctionValue(*value).item(0));
On 2016/01/19 22:47:48, eae wrote:
> Is this guaranteed to be non-null? Mixing references and pointers like this
> makes me nervous.

For the if branch we have an assertion just above the statement. For the else
part perhaps I could assert on the existance of item(0) (which is guaranteed,
there are no functions without at least 1 item).

I'll rewrite it anyway.

https://codereview.chromium.org/1582793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:2422: if
(isAutoRepeat && numberOfTracks)
On 2016/01/19 16:37:47, Manuel Rego wrote:
> Maybe a comment explaining that <auto-repeat>
> can only define 1 track.

If you don't mind, in this case I think the code is pretty clearly saying it. I
don't think we need to justify every line of code citing the specs. Maintaining
comments is hard so let's try to avoid adding them if the code is
self-explanatory

[svillar, 2016-01-21 12:45:54]

Removed comments and cleaned up a bit the mix of pointers and references

[eae, 2016-01-22 00:42:09]

Thank you, LGTM

[svillar, 2016-01-22 08:39:44]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-01-22 08:39:44]

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/1582793002/#ps20001
(title: "Removed comments and cleaned up a bit the mix of pointers and
references")

[commit-bot: I haz the power, 2016-01-22 08:39:59]

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

[commit-bot: I haz the power, 2016-01-22 09:42:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-22 09:42:33]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[svillar, 2016-01-22 11:33:35]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-01-22 11:33:35]

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

[commit-bot: I haz the power, 2016-01-22 11:33:49]

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

[commit-bot: I haz the power, 2016-01-22 12:41:25]

Description was changed from

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).

BUG=579104
==========

to

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).

BUG=579104
==========

[commit-bot: I haz the power, 2016-01-22 12:41:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-22 12:42:34]

Description was changed from

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).

BUG=579104
==========

to

==========
[css-grid] Add parsing support for <auto-repeat> syntax

The repeat() notation allows now to specify auto-fill or auto-fit instead of
a fixed number of repetitions meaning that it will be automatically computed
depending on the available space.

This CL just adds the parsing support, the expansion of the repeat notation
will be implemented in a follow up patch because it cannot be done at
parsing level (since it requires knowing about the available space).

BUG=579104

Committed: https://crrev.com/7647a0d4b25582406b03216f63f4906bd3c46d22
Cr-Commit-Position: refs/heads/master@{#370951}
==========

[commit-bot: I haz the power, 2016-01-22 12:42:34]

Patchset 3 (id:??) landed as
https://crrev.com/7647a0d4b25582406b03216f63f4906bd3c46d22
Cr-Commit-Position: refs/heads/master@{#370951}