Move the grid shorthand into CSSPropertyParser

Move the grid shorthand into CSSPropertyParser

Move the grid shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

This CL also moves the remaining methods out of
LegacyCSSPropertyParser.cpp and deletes it.

BUG=499780
Committed: https://crrev.com/5d14b507fcd9a1d4af9d2c7b96edb20e7c7b7414
Cr-Commit-Position: refs/heads/master@{#387225}

[rwlbuis, 2016-04-12 19:07:09]

Description was changed from

==========
grid WIP

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

to

==========
Move the grid shorthand into CSSPropertyParser

Move the grid shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

This CL also moves the remaining methods out of
LegacyCSSPropertyParser.cpp and deletes it.

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

[rwlbuis, 2016-04-12 20:17:15]

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

[rwlbuis, 2016-04-12 20:17:16]

PTAL.

I can do the moving helpers/removal of LegacyCSSPropertyParser.cpp in a later
patch if you prefer.

[Timothy Loh, 2016-04-13 06:09:42]

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

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:63: void
CSSPropertyParser::addProperty(CSSPropertyID propId, CSSValue* value, bool
important, bool implicit)
since you're moving this, can you rename propId to property

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:81: void
CSSPropertyParser::addExpandedPropertyForValue(CSSPropertyID propId, CSSValue*
value, bool important)
propId -> property

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3141: for (auto
value : valueList) {
does it work to write

for (CSSValue* value : valueList)

?

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3150:
ASSERT(value->isPrimitiveValue() || (value->isFunctionValue() &&
toCSSFunctionValue(*value).item(0)));
I guess this assertion is unnecessary since the conversions below will check the
same

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3170: if
(text[i] == ' ') {
Needs a TODO that this whitespace check is missing \n and \t

https://drafts.csswg.org/css-grid/#valdef-grid-template-areas-string
https://drafts.csswg.org/css-syntax-3/#whitespace

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3184: } else {
Needs a TODO to handle trash tokens (should only allow name code points here)

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3202: if
(gridRowNames.isEmpty() || gridRowNames.containsOnlyWhitespace())
I feel like I had this discussion before... but isEmpty is redundant here
because it vacuously contains only whitespace.

Anyway I think this is a strange way to handle this error condition. I think
it'd be easiest if parseGridTemplateAreasColumnNames just returned an empty
vector as its failure case (when we support rejecting trash characters in the
string), and if we do that then there's no point in explicitly checking for
whitespace-only up-front (and it doesn't really make sense as an optimisation
because authors shouldn't be writing these invalid declarations in the first
place). But let's make that a separate change.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3206: if
(!columnCount) {
IMO more obvious if you write if (rowCount == 0)

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3210: // The
declaration is invalid is all the rows don't have the number of columns.
invalid is -> invalid if

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3214: for
(size_t currentCol = 0; currentCol < columnCount; ++currentCol) {
currentColumn

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3221: // We
handle several grid areas with the same name at once to simplify the validation
code.
I'm not really sure what this comment is trying to say. Maybe better to remove
it. Grid cells with the same name form a single grid area.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3222: size_t
lookAheadCol;
imo more readable as

size_t lookAheadColumn = currentColumn + 1;
while (lookAheadColumn < columnCount && columnNames[lookAheadColumn] !=
gridAreaName)
    lookAheadColumn++;

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4774:
addProperty(CSSPropertyGridTemplateColumns,
cssValuePool().createImplicitInitialValue(), important);
I think it makes sense for the longhands to always get added in the same order,
so lets move the grid-template-* longhands before the grid-auto-* longhands to
match the above order and the CSSProperties.in order.

[rwlbuis, 2016-04-13 22:16:29]

PTAL (assuming it goes green).

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

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:63: void
CSSPropertyParser::addProperty(CSSPropertyID propId, CSSValue* value, bool
important, bool implicit)
On 2016/04/13 06:09:41, Timothy Loh wrote:
> since you're moving this, can you rename propId to property

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:81: void
CSSPropertyParser::addExpandedPropertyForValue(CSSPropertyID propId, CSSValue*
value, bool important)
On 2016/04/13 06:09:42, Timothy Loh wrote:
> propId -> property

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3141: for (auto
value : valueList) {
On 2016/04/13 06:09:42, Timothy Loh wrote:
> does it work to write
> 
> for (CSSValue* value : valueList)
> 
> ?

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3150:
ASSERT(value->isPrimitiveValue() || (value->isFunctionValue() &&
toCSSFunctionValue(*value).item(0)));
On 2016/04/13 06:09:42, Timothy Loh wrote:
> I guess this assertion is unnecessary since the conversions below will check
the
> same

It is not 100% covered just by that code. In theory the value could be something
else than a primitive of function value. In reality, the way
consumeGridTrackList works it can't be something else. I don't feel too strongly
about this, can remove it if you want, but it is not completely useless just
because of the ternary below.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3170: if
(text[i] == ' ') {
On 2016/04/13 06:09:42, Timothy Loh wrote:
> Needs a TODO that this whitespace check is missing \n and \t
> 
> https://drafts.csswg.org/css-grid/#valdef-grid-template-areas-string
> https://drafts.csswg.org/css-syntax-3/#whitespace

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3184: } else {
On 2016/04/13 06:09:42, Timothy Loh wrote:
> Needs a TODO to handle trash tokens (should only allow name code points here)

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3202: if
(gridRowNames.isEmpty() || gridRowNames.containsOnlyWhitespace())
On 2016/04/13 06:09:42, Timothy Loh wrote:
> I feel like I had this discussion before... but isEmpty is redundant here
> because it vacuously contains only whitespace.
> 
> Anyway I think this is a strange way to handle this error condition. I think
> it'd be easiest if parseGridTemplateAreasColumnNames just returned an empty
> vector as its failure case (when we support rejecting trash characters in the
> string), and if we do that then there's no point in explicitly checking for
> whitespace-only up-front (and it doesn't really make sense as an optimisation
> because authors shouldn't be writing these invalid declarations in the first
> place). But let's make that a separate change.

Acknowledged.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3206: if
(!columnCount) {
On 2016/04/13 06:09:42, Timothy Loh wrote:
> IMO more obvious if you write if (rowCount == 0)

You meant columnCount == 0, right? Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3210: // The
declaration is invalid is all the rows don't have the number of columns.
On 2016/04/13 06:09:41, Timothy Loh wrote:
> invalid is -> invalid if

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3214: for
(size_t currentCol = 0; currentCol < columnCount; ++currentCol) {
On 2016/04/13 06:09:41, Timothy Loh wrote:
> currentColumn

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3221: // We
handle several grid areas with the same name at once to simplify the validation
code.
On 2016/04/13 06:09:42, Timothy Loh wrote:
> I'm not really sure what this comment is trying to say. Maybe better to remove
> it. Grid cells with the same name form a single grid area.

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3222: size_t
lookAheadCol;
On 2016/04/13 06:09:42, Timothy Loh wrote:
> imo more readable as
> 
> size_t lookAheadColumn = currentColumn + 1;
> while (lookAheadColumn < columnCount && columnNames[lookAheadColumn] !=
> gridAreaName)
>     lookAheadColumn++;

Done.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4774:
addProperty(CSSPropertyGridTemplateColumns,
cssValuePool().createImplicitInitialValue(), important);
On 2016/04/13 06:09:42, Timothy Loh wrote:
> I think it makes sense for the longhands to always get added in the same
order,
> so lets move the grid-template-* longhands before the grid-auto-* longhands to
> match the above order and the CSSProperties.in order.

Done.

[Timothy Loh, 2016-04-14 02:04:12]

lgtm

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

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3150:
ASSERT(value->isPrimitiveValue() || (value->isFunctionValue() &&
toCSSFunctionValue(*value).item(0)));
On 2016/04/13 22:16:29, rwlbuis wrote:
> On 2016/04/13 06:09:42, Timothy Loh wrote:
> > I guess this assertion is unnecessary since the conversions below will check
> the
> > same
> 
> It is not 100% covered just by that code. In theory the value could be
something
> else than a primitive of function value. In reality, the way
> consumeGridTrackList works it can't be something else. I don't feel too
strongly
> about this, can remove it if you want, but it is not completely useless just
> because of the ternary below.

Not sure I follow. Ternary below says:

non-null primitive value (case 1)
OR
function value, first value is non-null primitive value (case 2)

If it's neither a primitive nor function value, then toCSSFunctionValue will hit
an assertion. So unless I'm misunderstanding, I think it'd be better to not have
the assertion because it isn't useful.

https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3206: if
(!columnCount) {
On 2016/04/13 22:16:29, rwlbuis wrote:
> On 2016/04/13 06:09:42, Timothy Loh wrote:
> > IMO more obvious if you write if (rowCount == 0)
> 
> You meant columnCount == 0, right? Done.

Nope, I meant rowCount. I think it makes more sense to check (rowCount == 0),
which is "this is the first row", rather than (columnCount == 0), which is "we
don't know how many columns yet, which we signal by setting it to 0".

[rwlbuis, 2016-04-14 02:21:59]

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

[rwlbuis, 2016-04-14 02:22:00]

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/1877073004/#ps80001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-04-14 02:22:42]

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

[rwlbuis, 2016-04-14 02:24:58]

On 2016/04/14 02:04:12, Timothy Loh wrote:
> Not sure I follow. Ternary below says:
> 
> non-null primitive value (case 1)
> OR
> function value, first value is non-null primitive value (case 2)
> 
> If it's neither a primitive nor function value, then toCSSFunctionValue will
hit
> an assertion. So unless I'm misunderstanding, I think it'd be better to not
have
> the assertion because it isn't useful.

Ah, I had not realized toCSSFunctionValue would assert. I removed the line.

>
https://codereview.chromium.org/1877073004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3206: if
> (!columnCount) {
> > You meant columnCount == 0, right? Done.
> 
> Nope, I meant rowCount. I think it makes more sense to check (rowCount == 0),
> which is "this is the first row", rather than (columnCount == 0), which is "we
> don't know how many columns yet, which we signal by setting it to 0".

I see, done :)

Once this is in, what is left? Do you want to make a patch removing
CSSParserValues.*? And there is also the parseValueStart TODO.

[commit-bot: I haz the power, 2016-04-14 03:32:17]

Description was changed from

==========
Move the grid shorthand into CSSPropertyParser

Move the grid shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

This CL also moves the remaining methods out of
LegacyCSSPropertyParser.cpp and deletes it.

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

to

==========
Move the grid shorthand into CSSPropertyParser

Move the grid shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

This CL also moves the remaining methods out of
LegacyCSSPropertyParser.cpp and deletes it.

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

[commit-bot: I haz the power, 2016-04-14 03:32:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-14 03:34:05]

Description was changed from

==========
Move the grid shorthand into CSSPropertyParser

Move the grid shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

This CL also moves the remaining methods out of
LegacyCSSPropertyParser.cpp and deletes it.

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

to

==========
Move the grid shorthand into CSSPropertyParser

Move the grid shorthand from LegacyCSSPropertyParser
into CSSPropertyParser.

This CL also moves the remaining methods out of
LegacyCSSPropertyParser.cpp and deletes it.

BUG=499780

Committed: https://crrev.com/5d14b507fcd9a1d4af9d2c7b96edb20e7c7b7414
Cr-Commit-Position: refs/heads/master@{#387225}
==========

[commit-bot: I haz the power, 2016-04-14 03:34:06]

Patchset 5 (id:??) landed as
https://crrev.com/5d14b507fcd9a1d4af9d2c7b96edb20e7c7b7414
Cr-Commit-Position: refs/heads/master@{#387225}

[Timothy Loh, 2016-04-14 03:38:46]

On 2016/04/14 02:24:58, rwlbuis wrote:
> Once this is in, what is left? Do you want to make a patch removing
> CSSParserValues.*? And there is also the parseValueStart TODO.

Feel free to remove CSSParserValues. I think we can remove
ShorthandScope/m_inParseShorthand and just set m_currentShorthand instead in the
one place it's used. There's a bunch of other stuff that I'm thinking of but.. I
need to actually check that any of it makes sense first. When I have time I'll
probably scroll through the entire property parser and make some clean-ups.

But yeah, once CSSParserValues is removed I think we can close the bug and call
this finished.

[rwlbuis, 2016-04-14 16:54:57]

On 2016/04/14 03:38:46, Timothy Loh wrote:
> On 2016/04/14 02:24:58, rwlbuis wrote:
> > Once this is in, what is left? Do you want to make a patch removing
> > CSSParserValues.*? And there is also the parseValueStart TODO.
> 
> Feel free to remove CSSParserValues. I think we can remove
> ShorthandScope/m_inParseShorthand and just set m_currentShorthand instead in
the
> one place it's used. There's a bunch of other stuff that I'm thinking of but..
I
> need to actually check that any of it makes sense first. When I have time I'll
> probably scroll through the entire property parser and make some clean-ups.
> 
> But yeah, once CSSParserValues is removed I think we can close the bug and
call
> this finished.

Sounds good, I put that CSSParserValues removal patch up here:
https://codereview.chromium.org/1883983003/