Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

Also move parseGridTemplateAreasRow and related functions to
CSSPropertyParser.cpp and give them a consume prefix.

BUG=499780
Committed: https://crrev.com/496a1a7ab42e6cce170f32ccfad5454fe6d87987
Cr-Commit-Position: refs/heads/master@{#386397}

[rwlbuis, 2016-03-30 22:36:17]

Patchset #2 (id:20001) has been deleted

[rwlbuis, 2016-03-31 20:21:36]

Description was changed from

==========
grip-template WIP


Foo

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

to

==========
Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

Also move parseGridTemplateAreasRow and related functions to
CSSPropertyParser.cpp and give them a consume prefix.

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

[rwlbuis, 2016-03-31 20:25:23]

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

[rwlbuis, 2016-03-31 20:25:24]

PTAL.

Note:
- consumeGridTemplateRowsAndAreasAndColumns still seems a bit too big to me to
inline it into consumeGridTemplateShorthand.
- I did not try at all to clean up helpers like parseGridTemplateAreasRow.
- The actual moving of the helper functions can be done later if we feel the
patch is too big/does to much.

[jfernandez, 2016-04-01 10:48:11]

jfernandez@igalia.com changed reviewers:
+ rego@igalia.com, svillar@igalia.com

[jfernandez, 2016-04-01 10:48:11]

Adding @svillar and @rego as reviewers.

[svillar, 2016-04-01 12:52:47]

I would not spend too much time porting this (unless it blocks some other moves)
because grid-template was removed in the last draft of the specs. We're waiting
a bit before completely removing it just in case, but it will likely desapear.

[rwlbuis, 2016-04-01 14:20:19]

On 2016/04/01 12:52:47, svillar wrote:
> I would not spend too much time porting this (unless it blocks some other
moves)
> because grid-template was removed in the last draft of the specs. We're
waiting
> a bit before completely removing it just in case, but it will likely desapear.

Now he tells me ;)

But seriously, I think we still want to do this, since we need it to remove
CSSParserValue, which is one of the goals of issue 499780.

[Manuel Rego, 2016-04-01 14:26:30]

On 2016/04/01 14:20:19, rwlbuis wrote:
> On 2016/04/01 12:52:47, svillar wrote:
> > I would not spend too much time porting this (unless it blocks some other
> moves)
> > because grid-template was removed in the last draft of the specs. We're
> waiting
> > a bit before completely removing it just in case, but it will likely
desapear.
> 
> Now he tells me ;)
> 
> But seriously, I think we still want to do this, since we need it to remove
> CSSParserValue, which is one of the goals of issue 499780.

The CSS WG still has to decide what to do
as some people are asking to bring back the grid-template shorthand:
https://lists.w3.org/Archives/Public/www-style/2016Mar/0345.html

So, I'll port it anyway, it's going to be needed for the "grid" shorthand
itself.
And eventually, we could remove the "grid-template" shorthand
once there's a definitive resolution on the CSS WG.

[rwlbuis, 2016-04-01 14:35:09]

On 2016/04/01 14:26:30, Manuel Rego wrote:
> So, I'll port it anyway, it's going to be needed for the "grid" shorthand
> itself.
> And eventually, we could remove the "grid-template" shorthand
> once there's a definitive resolution on the CSS WG.

Good point, I forgot that the grid shorthand relies on parsing of the
grip-template shorthand.

[svillar, 2016-04-01 15:46:45]

On 2016/04/01 14:20:19, rwlbuis wrote:
> On 2016/04/01 12:52:47, svillar wrote:
> > I would not spend too much time porting this (unless it blocks some other
> moves)
> > because grid-template was removed in the last draft of the specs. We're
> waiting
> > a bit before completely removing it just in case, but it will likely
desapear.
> 
> Now he tells me ;)

:)

The first time I knew about this CL was when jfernandez added me to the Cc.
Please add me to changes related to grid layout too.

[rwlbuis, 2016-04-01 16:04:17]

On 2016/04/01 15:46:45, svillar wrote:
> > Now he tells me ;)
> 
> :)
> 
> The first time I knew about this CL was when jfernandez added me to the Cc.
> Please add me to changes related to grid layout too.

Sure, will do :)

[Timothy Loh, 2016-04-05 06:33:32]

Let's just leave the existing string parsing helpers as they are right now, the
patch is already big enough with other stuff.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3112: static
RawPtr<CSSGridLineNamesValue> consumeGridLineNames(CSSParserTokenRange& range,
CSSGridLineNamesValue* previousNamedAreaTrailingLineNames = nullptr)
Let's just call this lineNames and add a comment above saying
// Appends to the passed in CSSGridLineNamesValue if any, otherwise creates a
new one

Also no reason to have a separate local variable below.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4561: if
(consumeSlashIncludingWhitespace(m_range))
Shouldn't be needed if you make the below loop a do-while loop

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4568:
RefPtrWillBeRawPtr<CSSValueList> templateRows =
CSSValueList::createSpaceSeparated();
I'd wait for https://codereview.chromium.org/1858753003/ to land and then rebase
and remove all the WillBe or RawPtr types from your change

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4570: while
(!m_range.atEnd() && !(m_range.peek().type() == DelimiterToken &&
m_range.peek().delimiter() == '/')) {
do { } while

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4572:
CSSGridLineNamesValue* previousNamedAreaTrailingLineNames = nullptr;
Maybe all of this would be easier if we had outside of the loop

// Persists between loop iterations so we can use the same value for
// consecutive <line-names> values
CSSGridLineNamesValue* lineNames = nullptr;

and then

bool hasPreviousLineNames = lineNames != nullptr;
lineNames = consumeGridLineNames(m_range, lineNames);
if (lineNames && !hasPreviousLineNames)
    ...

and we can just remove the trailingIdentWasAdded bool.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4601:
columnsValue = consumeGridTrackList(m_range, m_context.mode());
Didn't we make consumeGridTrackList parse <track-list>|<auto-track-list>? Maybe
we need a bool to disallow <auto-track-list>.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4636:
RefPtrWillBeRawPtr<CSSValue> columnsValue = consumeIdent<CSSValueNone>(m_range);
consumeGridTemplatesRowsOrColumns ?

[rwlbuis, 2016-04-05 22:55:09]

PTAL. I agree with not touching the helper functions.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3112: static
RawPtr<CSSGridLineNamesValue> consumeGridLineNames(CSSParserTokenRange& range,
CSSGridLineNamesValue* previousNamedAreaTrailingLineNames = nullptr)
On 2016/04/05 06:33:31, Timothy Loh wrote:
> Let's just call this lineNames and add a comment above saying
> // Appends to the passed in CSSGridLineNamesValue if any, otherwise creates a
> new one
> 
> Also no reason to have a separate local variable below.

Done.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4561: if
(consumeSlashIncludingWhitespace(m_range))
On 2016/04/05 06:33:32, Timothy Loh wrote:
> Shouldn't be needed if you make the below loop a do-while loop

Done.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4568:
RefPtrWillBeRawPtr<CSSValueList> templateRows =
CSSValueList::createSpaceSeparated();
On 2016/04/05 06:33:31, Timothy Loh wrote:
> I'd wait for https://codereview.chromium.org/1858753003/ to land and then
rebase
> and remove all the WillBe or RawPtr types from your change

Acknowledged.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4570: while
(!m_range.atEnd() && !(m_range.peek().type() == DelimiterToken &&
m_range.peek().delimiter() == '/')) {
On 2016/04/05 06:33:31, Timothy Loh wrote:
> do { } while

Done.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4572:
CSSGridLineNamesValue* previousNamedAreaTrailingLineNames = nullptr;
On 2016/04/05 06:33:31, Timothy Loh wrote:
> Maybe all of this would be easier if we had outside of the loop
> 
> // Persists between loop iterations so we can use the same value for
> // consecutive <line-names> values
> CSSGridLineNamesValue* lineNames = nullptr;
> 
> and then
> 
> bool hasPreviousLineNames = lineNames != nullptr;
> lineNames = consumeGridLineNames(m_range, lineNames);
> if (lineNames && !hasPreviousLineNames)
>     ...
> 
> and we can just remove the trailingIdentWasAdded bool.

Done.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4601:
columnsValue = consumeGridTrackList(m_range, m_context.mode());
On 2016/04/05 06:33:32, Timothy Loh wrote:
> Didn't we make consumeGridTrackList parse <track-list>|<auto-track-list>?
Maybe
> we need a bool to disallow <auto-track-list>.

Correct, however not sure if we need to do this. First, situation is same before
this patch, may need a test and grid-template is deprecated AFAIK. But let me
know.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4636:
RefPtrWillBeRawPtr<CSSValue> columnsValue = consumeIdent<CSSValueNone>(m_range);
On 2016/04/05 06:33:31, Timothy Loh wrote:
> consumeGridTemplatesRowsOrColumns ?

Done.

[Timothy Loh, 2016-04-07 06:48:22]

The most recent patch set still moves over a whole bunch of helpers. Let's move
them across separately because it makes the patch look much larger than it is.

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1843773003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4601:
columnsValue = consumeGridTrackList(m_range, m_context.mode());
On 2016/04/05 22:55:08, rwlbuis wrote:
> On 2016/04/05 06:33:32, Timothy Loh wrote:
> > Didn't we make consumeGridTrackList parse <track-list>|<auto-track-list>?
> Maybe
> > we need a bool to disallow <auto-track-list>.
> 
> Correct, however not sure if we need to do this. First, situation is same
before
> this patch, may need a test and grid-template is deprecated AFAIK. But let me
> know.

Let's at least document it with a comment.

https://codereview.chromium.org/1843773003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1843773003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4643: // 3-
[<line-names>? <string> <track-size>? <line-names>? ]+ syntax.
might as well just write it out completely

[ <line-names>? <string> <track-size>? <line-names>? ]+ [ / <track-list> ]?

[rwlbuis, 2016-04-07 14:52:15]

Patchset #9 (id:180001) has been deleted

[rwlbuis, 2016-04-07 19:51:22]

https://codereview.chromium.org/1843773003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1843773003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4643: // 3-
[<line-names>? <string> <track-size>? <line-names>? ]+ syntax.
On 2016/04/07 06:48:22, Timothy Loh wrote:
> might as well just write it out completely
> 
> [ <line-names>? <string> <track-size>? <line-names>? ]+ [ / <track-list> ]?

Done.

[rwlbuis, 2016-04-07 19:53:22]

On 2016/04/07 06:48:22, Timothy Loh wrote:
> The most recent patch set still moves over a whole bunch of helpers. Let's
move
> them across separately because it makes the patch look much larger than it is.

Ah, I misunderstood, I thought you meant lets move the helpers but not change
them.
I left the helpers back in LegacyCSSParser.cpp, but removed the duplicated and
unused methods as before.

> > Correct, however not sure if we need to do this. First, situation is same
> before
> > this patch, may need a test and grid-template is deprecated AFAIK. But let
me
> > know.
> 
> Let's at least document it with a comment.

Done.

PTAL.

[Timothy Loh, 2016-04-11 03:27:32]

lgtm, but change all the RawPtr<T> to T*

https://codereview.chromium.org/1843773003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1843773003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3118: //
TODO(rob.buis): consider adding boolean parameter to disallow <auto-track-list>
if we keep supporing grid-template.
In any case, this is going to be used by the grid shorthand, so maybe more
accurate to write something like

// TODO(rob.buis): This needs a bool parameter so we can disallow
<auto-track-list> for the grid shorthand

[rwlbuis, 2016-04-11 14:32:41]

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

[rwlbuis, 2016-04-11 14:32:42]

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

[commit-bot: I haz the power, 2016-04-11 14:32:54]

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

[commit-bot: I haz the power, 2016-04-11 15:56:57]

Description was changed from

==========
Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

Also move parseGridTemplateAreasRow and related functions to
CSSPropertyParser.cpp and give them a consume prefix.

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

to

==========
Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

Also move parseGridTemplateAreasRow and related functions to
CSSPropertyParser.cpp and give them a consume prefix.

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

[commit-bot: I haz the power, 2016-04-11 15:56:58]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-04-11 15:58:33]

Description was changed from

==========
Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

Also move parseGridTemplateAreasRow and related functions to
CSSPropertyParser.cpp and give them a consume prefix.

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

to

==========
Move the grid-template shorthand into CSSPropertyParser

Move the grid-template shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

Also move parseGridTemplateAreasRow and related functions to
CSSPropertyParser.cpp and give them a consume prefix.

BUG=499780

Committed: https://crrev.com/496a1a7ab42e6cce170f32ccfad5454fe6d87987
Cr-Commit-Position: refs/heads/master@{#386397}
==========

[commit-bot: I haz the power, 2016-04-11 15:58:34]

Patchset 10 (id:??) landed as
https://crrev.com/496a1a7ab42e6cce170f32ccfad5454fe6d87987
Cr-Commit-Position: refs/heads/master@{#386397}