Move some grid related longhands into CSSPropertyParser

Move some grid related longhands into CSSPropertyParser

Move the following grid related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

justify-self, justify-items,
align-self, align-items

BUG=499780
Committed: https://crrev.com/4699914d8fe7b1594c711c94a3c3362b8fe54c1f
Cr-Commit-Position: refs/heads/master@{#380513}

[rwlbuis, 2016-03-09 23:06:29]

Description was changed from

==========
justify-self WIP

BUG=
==========

to

==========
Move some grid related longhands into CSSPropertyParser

Move the following grid related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

justify-self, justify-items,
align-self, align-items

BUG=499780
==========

[rwlbuis, 2016-03-09 23:07:47]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2016-03-09 23:07:48]

PTAL.

[Timothy Loh, 2016-03-10 00:12:14]

ccing jfernandez - fyi this will conflict with a patch you have up. Also could
you comment on my suggestion below to rename variable names? Thanks!

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

https://codereview.chromium.org/1776983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3570: if
(isItemPositionKeyword(range.peek().id())) {
Isn't this almost the same as the regular pattern ? Seems like we should just do
it like normal. Also maybe a good idea to match the terms in the current spec?

overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
selfPosition = consumeSelfPositionKeyword(range);
if (!selfPosition)
    return false;
if (!overflowPosition)
    overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
...

[jfernandez, 2016-03-10 14:11:01]

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

[jfernandez, 2016-03-10 14:11:02]

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

https://codereview.chromium.org/1776983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3570: if
(isItemPositionKeyword(range.peek().id())) {
On 2016/03/10 00:12:14, Timothy Loh wrote:
> Isn't this almost the same as the regular pattern ? Seems like we should just
do
> it like normal.

The only special thing we do here is to take care about the order we consume the
different keywords. Specs defines the syntax <overflow-position> &&
<self-position>, so they can be defined in any order. 

 Also maybe a good idea to match the terms in the current spec?
> 

Totally agree about the names; it as <item-position> at the time we implemented
that parsing logic.

> overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
> selfPosition = consumeSelfPositionKeyword(range);
> if (!selfPosition)
>     return false;
> if (!overflowPosition)
>     overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
> ...

[rwlbuis, 2016-03-10 19:20:19]

On 2016/03/10 14:11:02, jfernandez wrote:
>
https://codereview.chromium.org/1776983003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1776983003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3570: if
> (isItemPositionKeyword(range.peek().id())) {
> On 2016/03/10 00:12:14, Timothy Loh wrote:
> > Isn't this almost the same as the regular pattern ? Seems like we should
just
> do
> > it like normal.
> 
> The only special thing we do here is to take care about the order we consume
the
> different keywords. Specs defines the syntax <overflow-position> &&
> <self-position>, so they can be defined in any order. 
> 
>  Also maybe a good idea to match the terms in the current spec?
> > 
> 
> Totally agree about the names; it as <item-position> at the time we
implemented
> that parsing logic.
> 
> > overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
> > selfPosition = consumeSelfPositionKeyword(range);
> > if (!selfPosition)
> >     return false;
> > if (!overflowPosition)
> >     overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
> > ...

Thanks guys, I basically used above code in patch set 2. BTW I implement the
parsing logic based on what I hope is the latest spec:
https://www.w3.org/TR/css3-align/#propdef-align-self

PTAL.

[jfernandez, 2016-03-10 19:39:39]

On 2016/03/10 19:20:19, rwlbuis wrote:
> On 2016/03/10 14:11:02, jfernandez wrote:
> >
>
https://codereview.chromium.org/1776983003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1776983003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3570: if
> > (isItemPositionKeyword(range.peek().id())) {
> > On 2016/03/10 00:12:14, Timothy Loh wrote:
> > > Isn't this almost the same as the regular pattern ? Seems like we should
> just
> > do
> > > it like normal.
> > 
> > The only special thing we do here is to take care about the order we consume
> the
> > different keywords. Specs defines the syntax <overflow-position> &&
> > <self-position>, so they can be defined in any order. 
> > 
> >  Also maybe a good idea to match the terms in the current spec?
> > > 
> > 
> > Totally agree about the names; it as <item-position> at the time we
> implemented
> > that parsing logic.
> > 
> > > overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
> > > selfPosition = consumeSelfPositionKeyword(range);
> > > if (!selfPosition)
> > >     return false;
> > > if (!overflowPosition)
> > >     overflowPosition = consumeIdent<CSSValueUnsafe, CSSValueSafe>(range);
> > > ...
> 
> Thanks guys, I basically used above code in patch set 2. BTW I implement the
> parsing logic based on what I hope is the latest spec:
> https://www.w3.org/TR/css3-align/#propdef-align-self
> 
> PTAL.

Better use this spec version

https://drafts.csswg.org/css-align/#align-self-property

[rwlbuis, 2016-03-10 19:48:37]

On 2016/03/10 19:39:39, jfernandez wrote:
> Better use this spec version
> 
> https://drafts.csswg.org/css-align/#align-self-property

Thanks, I renamed based on this in patch set 3.

[Timothy Loh, 2016-03-10 23:08:22]

On 2016/03/10 19:48:37, rwlbuis wrote:
> On 2016/03/10 19:39:39, jfernandez wrote:
> > Better use this spec version
> > 
> > https://drafts.csswg.org/css-align/#align-self-property
> 
> Thanks, I renamed based on this in patch set 3.

lgtm

[rwlbuis, 2016-03-10 23:12:42]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-03-10 23:13:38]

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

[commit-bot: I haz the power, 2016-03-10 23:58:55]

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

[commit-bot: I haz the power, 2016-03-10 23:58:56]

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

[rwlbuis, 2016-03-11 00:01:55]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-03-11 00:03:09]

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

[commit-bot: I haz the power, 2016-03-11 00:41:32]

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

[commit-bot: I haz the power, 2016-03-11 00:41:32]

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

[rwlbuis, 2016-03-11 01:09:44]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-03-11 01:10:23]

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

[commit-bot: I haz the power, 2016-03-11 01:47:32]

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

[commit-bot: I haz the power, 2016-03-11 01:47:34]

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

[rwlbuis, 2016-03-11 02:31:37]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-03-11 02:32:11]

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

[commit-bot: I haz the power, 2016-03-11 03:06:58]

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

[commit-bot: I haz the power, 2016-03-11 03:06:59]

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

[rwlbuis, 2016-03-11 03:30:47]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-03-11 03:31:39]

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

[commit-bot: I haz the power, 2016-03-11 05:14:00]

Description was changed from

==========
Move some grid related longhands into CSSPropertyParser

Move the following grid related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

justify-self, justify-items,
align-self, align-items

BUG=499780
==========

to

==========
Move some grid related longhands into CSSPropertyParser

Move the following grid related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

justify-self, justify-items,
align-self, align-items

BUG=499780
==========

[commit-bot: I haz the power, 2016-03-11 05:14:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-11 05:14:56]

Description was changed from

==========
Move some grid related longhands into CSSPropertyParser

Move the following grid related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

justify-self, justify-items,
align-self, align-items

BUG=499780
==========

to

==========
Move some grid related longhands into CSSPropertyParser

Move the following grid related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

justify-self, justify-items,
align-self, align-items

BUG=499780

Committed: https://crrev.com/4699914d8fe7b1594c711c94a3c3362b8fe54c1f
Cr-Commit-Position: refs/heads/master@{#380513}
==========

[commit-bot: I haz the power, 2016-03-11 05:14:58]

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