[css-grid] Simplify grid track sizes parsing

[css-grid] Simplify grid track sizes parsing

Previously once we saw an auto-repeat function,
we passed the "FixedSizeOnly" restriction to the rest of methods.
That way we were sure that all the tracks after the auto-repeat
had fixed sizes.
But we needed to call allTracksAreFixedSized() to be sure that
the tracks before the auto-repeat had fixed sizes too.

Now we're introducing a new boolean allTracksAreFixedSized,
to check in advance if the declaration contains any track not fixed.
If that's the case and we found an auto-repeat method,
we consider it invalid.
With this approach we avoid the loop to verify
that all the tracks (before and after the auto-repeat) are fixed.
It also allows us to simplify the code and avoid passing
the restriction to all the methods parsing the track size.

No new tests, no change of behavior.

Committed: https://crrev.com/004c4fbec738cd7f8445fc5b8a349aea89630298
Cr-Commit-Position: refs/heads/master@{#395187}

[Manuel Rego, 2016-05-20 15:43:20]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, rob.buis@samsung.com, svillar@igalia.com,
timloh@chromium.org

[rwlbuis, 2016-05-20 16:59:21]

Seems like this is more readable, nice.

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

https://codereview.chromium.org/1998033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3453:
allTracksAreFixedSized &= isGridTrackFixedSized(*trackSize);
Seems like you can potentially do less calls if you do:
if (allTracksAreFixedSized)
    allTracksAreFixedSized = isGridTrackFixedSized(*trackSize);

https://codereview.chromium.org/1998033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3496:
allTracksAreFixedSized &= isGridTrackFixedSized(*value);
Ditto.

[Manuel Rego, 2016-05-20 18:44:40]

Thanks for the quick review, new version uploaded.

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

https://codereview.chromium.org/1998033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3453:
allTracksAreFixedSized &= isGridTrackFixedSized(*trackSize);
On 2016/05/20 16:59:21, rwlbuis wrote:
> Seems like you can potentially do less calls if you do:
> if (allTracksAreFixedSized)
>     allTracksAreFixedSized = isGridTrackFixedSized(*trackSize);

Done.

https://codereview.chromium.org/1998033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3496:
allTracksAreFixedSized &= isGridTrackFixedSized(*value);
On 2016/05/20 16:59:20, rwlbuis wrote:
> Ditto.

Done.

[rwlbuis, 2016-05-20 20:06:36]

lgtm.

PS: I left two grid TODOs in case you are interested:
// TODO(rob.buis): This needs a bool parameter so we can disallow
<auto-track-list> for the grid shorthand.
// TODO(rob.buis): <line-names> should not be able to directly precede
<auto-repeat>.

[Manuel Rego, 2016-05-20 20:20:54]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-05-20 20:21:22]

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

[commit-bot: I haz the power, 2016-05-20 22:26:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-20 22:30:01]

Description was changed from

==========
[css-grid] Simplify grid track sizes parsing

Previously once we saw an auto-repeat function,
we passed the "FixedSizeOnly" restriction to the rest of methods.
That way we were sure that all the tracks after the auto-repeat
had fixed sizes.
But we needed to call allTracksAreFixedSized() to be sure that
the tracks before the auto-repeat had fixed sizes too.

Now we're introducing a new boolean allTracksAreFixedSized,
to check in advance if the declaration contains any track not fixed.
If that's the case and we found an auto-repeat method,
we consider it invalid.
With this approach we avoid the loop to verify
that all the tracks (before and after the auto-repeat) are fixed.
It also allows us to simplify the code and avoid passing
the restriction to all the methods parsing the track size.

No new tests, no change of behavior.
==========

to

==========
[css-grid] Simplify grid track sizes parsing

Previously once we saw an auto-repeat function,
we passed the "FixedSizeOnly" restriction to the rest of methods.
That way we were sure that all the tracks after the auto-repeat
had fixed sizes.
But we needed to call allTracksAreFixedSized() to be sure that
the tracks before the auto-repeat had fixed sizes too.

Now we're introducing a new boolean allTracksAreFixedSized,
to check in advance if the declaration contains any track not fixed.
If that's the case and we found an auto-repeat method,
we consider it invalid.
With this approach we avoid the loop to verify
that all the tracks (before and after the auto-repeat) are fixed.
It also allows us to simplify the code and avoid passing
the restriction to all the methods parsing the track size.

No new tests, no change of behavior.

Committed: https://crrev.com/004c4fbec738cd7f8445fc5b8a349aea89630298
Cr-Commit-Position: refs/heads/master@{#395187}
==========

[commit-bot: I haz the power, 2016-05-20 22:30:02]

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

[Manuel Rego, 2016-05-23 09:46:30]

On 2016/05/20 20:06:36, rwlbuis wrote:
> PS: I left two grid TODOs in case you are interested:
> // TODO(rob.buis): This needs a bool parameter so we can disallow
> <auto-track-list> for the grid shorthand.
> // TODO(rob.buis): <line-names> should not be able to directly precede
> <auto-repeat>.

I'm not sure if these issues are right or not on the spec.
So I've send some mails to www-style trying to clarify it:
https://lists.w3.org/Archives/Public/www-style/2016May/0189.html
https://lists.w3.org/Archives/Public/www-style/2016May/0191.html

Once I got answers I'll:
1) Get rid of the TODOs or
2) Report the bugs and associate them with grid layout meta-bug.

[Manuel Rego, 2016-05-24 10:19:47]

On 2016/05/20 20:06:36, rwlbuis wrote:
> // TODO(rob.buis): This needs a bool parameter so we can disallow
> <auto-track-list> for the grid shorthand.

This will be addressed as part of bug #614314.

> // TODO(rob.buis): <line-names> should not be able to directly precede
> <auto-repeat>.

This won't be needed and it'll be removed in the following CL:
https://codereview.chromium.org/2003223003/