[CSSGridLayout] Switch from parentheses to brackets for grid line names

[CSSGridLayout] Switch from parentheses to brackets for grid line names

Grid line names are now wrapped in square brackets instead of
parentheses as agreed in the latest F2F meeting of the CSSWG, see https://lists.w3.org/Archives/Public/www-style/2015May/0175.html. Apparently
they're easier to read and compatible with SASS.


BUG=487995
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196568

[svillar, 2015-06-02 09:04:15]

svillar@igalia.com changed reviewers:
+ dsinclair@chromium.org, eae@chromium.org, jchaffraix@chromium.org

[svillar, 2015-06-02 09:04:16]

Sending for review. Should be easy.

[Manuel Rego, 2015-06-04 11:38:38]

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

[Manuel Rego, 2015-06-04 11:38:39]

The patch seems good to me. Just some small suggestions in the tests.

Regarding the description, you're missing the spaces in the subject
[CSSGridLayout] vs [CSS Grid Layout] :-)
Also you could add a link to the CSSWG resolution for future reference:
https://lists.w3.org/Archives/Public/www-style/2015May/0280.html

https://codereview.chromium.org/1168453002/diff/1/LayoutTests/fast/css-grid-l...
File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):

https://codereview.chromium.org/1168453002/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:68: }
Would it make sense to keep a few of the old syntax examples,
considering them as as invalid values?

https://codereview.chromium.org/1168453002/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:139:
testGridDefinitionsSetBadJSValues("foo bar 10px", "50% baz bar foo 2em");
Here, you're updating some examples but not other.
Maybe it'd be better to change everything to brackets, and add a few
cases for the old syntax with parenthesis.

[svillar, 2015-06-04 14:28:11]

On 2015/06/04 11:38:39, Manuel Rego wrote:
> The patch seems good to me. Just some small suggestions in the tests.
> 
> Regarding the description, you're missing the spaces in the subject
> [CSSGridLayout] vs [CSS Grid Layout] :-)
> Also you could add a link to the CSSWG resolution for future reference:
> https://lists.w3.org/Archives/Public/www-style/2015May/0280.html

Heh :) that was done on purpose to keep the title shorter than the recommended
70-something columns. Mentioning the resolution might be a good idea indeed.

>
https://codereview.chromium.org/1168453002/diff/1/LayoutTests/fast/css-grid-l...
> File LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html (right):
> 
>
https://codereview.chromium.org/1168453002/diff/1/LayoutTests/fast/css-grid-l...
> LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:68: }
> Would it make sense to keep a few of the old syntax examples,
> considering them as as invalid values?

I thought about that but decided not to do it because using parentheses would be
the same as using any other character. I verified that all of them were failing
before doing the replacement BTW.

>
https://codereview.chromium.org/1168453002/diff/1/LayoutTests/fast/css-grid-l...
> LayoutTests/fast/css-grid-layout/named-grid-line-get-set.html:139:
> testGridDefinitionsSetBadJSValues("foo bar 10px", "50% baz bar foo 2em");
> Here, you're updating some examples but not other.
> Maybe it'd be better to change everything to brackets, and add a few
> cases for the old syntax with parenthesis.

Oh that's indeed a mistake, I'll fix that.

[svillar, 2015-06-04 16:30:52]

Apart from the test I also had to update the parsing code, as with the new
parser we need to verify the existence of opening and closing brackets because
we no longer get a list of idents

[jfernandez, 2015-06-04 22:49:55]

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

[jfernandez, 2015-06-04 22:49:57]

lgtm

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3231: 
Unneeded white line.

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3442: static inline bool
isClosingBracket(CSSParserValue* value)
Shouldn't value argument be 'const' ?

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3449: if (!inputList.current() ||
inputList.current()->unit != CSSParserValue::Operator ||
inputList.current()->iValue != '[')
Wouldn't be a good idea to define  a 'isOpenBracket' inline function as well ?

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3493: // Handle leading  <ident>*.
Shouldn't be <custom-ident> ?

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3537: // Handle leading <ident>*.
Ditto.

[Julien - ping for review, 2015-06-04 23:17:58]

lgtm, the link in the description is wrong and should be:
https://lists.w3.org/Archives/Public/www-style/2015May/0175.html

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3233: return false;
Aren't you tightening our parsing here and thus this should get some extra test
coverage?

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3442: static inline bool
isClosingBracket(CSSParserValue* value)
On 2015/06/04 at 22:49:56, jfernandez wrote:
> Shouldn't value argument be 'const' ?

Even better a constant reference.

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3551: if
(!maybeParseGridLineNames(*arguments, *repeatedValues))
This function is not 'maybe' parsing anymore if we make all the caller reject
the value list when it returns false. It should be rename to parseGridLineNames.

I am concerned that we implemented too complicated a logic due to those maybe
function.

[svillar, 2015-06-05 07:25:43]

Thanks for the reviews!

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3233: return false;
On 2015/06/04 23:17:58, Julien Chaffraix - PST wrote:
> Aren't you tightening our parsing here and thus this should get some extra
test
> coverage?

Actually I acknowledge that it looks like that we tighten our parsing, but in
fact we aren't. I'll explain why:

* with the old bison-based parser, the grammar allowed us to create a
CSSParserValue::ValueList object for each ident list. That simplified a lot the
property parsing code as we didn't have to verify the existence of
opening/closing parentheses, nested parentheses etc... the grammar was just
rejecting those rules before our property parsing code was executed.

* with the new one, we just get a flow of tokens that need to be parsed, and
thus, we're the ones who need to do all the verifications.

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3442: static inline bool
isClosingBracket(CSSParserValue* value)
On 2015/06/04 23:17:58, Julien Chaffraix - PST wrote:
> On 2015/06/04 at 22:49:56, jfernandez wrote:
> > Shouldn't value argument be 'const' ?
> 
> Even better a constant reference.

Acknowledged.

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3449: if (!inputList.current() ||
inputList.current()->unit != CSSParserValue::Operator ||
inputList.current()->iValue != '[')
On 2015/06/04 22:49:57, jfernandez wrote:
> Wouldn't be a good idea to define  a 'isOpenBracket' inline function as well ?

I don't think so, it'd be just used here.

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3493: // Handle leading  <ident>*.
On 2015/06/04 22:49:56, jfernandez wrote:
> Shouldn't be <custom-ident> ?

Yes, that's a legacy comment.

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3551: if
(!maybeParseGridLineNames(*arguments, *repeatedValues))
On 2015/06/04 23:17:58, Julien Chaffraix - PST wrote:
> This function is not 'maybe' parsing anymore if we make all the caller reject
> the value list when it returns false. It should be rename to
parseGridLineNames.

Sure, I also thought about that.

> I am concerned that we implemented too complicated a logic due to those maybe
> function.

Not sure what you exactly mean. With the old parser it made total sense, because
we were getting a proper CSSParserValueList from the parser in the event of
having a valid ident list. Right now that has totally changed and we have to do
that parsing on our own.

[svillar, 2015-06-05 08:38:44]

Applied the suggested changes. I also took the chance to use an already existing
function we have to verify that the idents are valid. As this tightens our
parsing code, the appropiate test cases were added

[svillar, 2015-06-05 08:50:29]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-06-05 08:50:29]

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

[commit-bot: I haz the power, 2015-06-05 08:50:37]

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

[commit-bot: I haz the power, 2015-06-05 09:11:09]

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

[commit-bot: I haz the power, 2015-06-05 09:11:10]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57814)

[svillar, 2015-06-05 09:31:35]

Now with unit tests also ported to the new syntax

[svillar, 2015-06-05 09:32:13]

The CQ bit was checked by svillar@igalia.com

[svillar, 2015-06-05 09:32:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com, jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1168453002/#ps60001
(title: "Patch for landing v2")

[commit-bot: I haz the power, 2015-06-05 09:32:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1168453002/60001

[commit-bot: I haz the power, 2015-06-05 11:00:42]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196568

[Julien - ping for review, 2015-06-05 16:51:20]

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1168453002/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:3551: if
(!maybeParseGridLineNames(*arguments, *repeatedValues))
On 2015/06/05 at 07:25:43, svillar wrote:
> On 2015/06/04 23:17:58, Julien Chaffraix - PST wrote:
> > This function is not 'maybe' parsing anymore if we make all the caller
reject
> > the value list when it returns false. It should be rename to
parseGridLineNames.
> 
> Sure, I also thought about that.
> 
> > I am concerned that we implemented too complicated a logic due to those
maybe
> > function.
> 
> Not sure what you exactly mean. With the old parser it made total sense,
because we were getting a proper CSSParserValueList from the parser in the event
of having a valid ident list. Right now that has totally changed and we have to
do that parsing on our own.

The comment was basically that with the new parser, maybe a lot of the
complexity in the code is not needed anymore (as embodied by the maybe*
functions) and we could follow with a simplification. Again this was me thinking
out loud.