Add property parser code path based on CSSParserTokenRange

Add property parser code path based on CSSParserTokenRange

Add property parser code path based on CSSParserTokenRange and put most
of this code in CSSPropertyParser.cpp, keeping the legacy code
in LegacyCSSPropertyParser.cpp.

Port some simple properties and two shorthands, and introduce some
CSSParserTokenRange specific helper methods.

BUG=499780
Committed: https://crrev.com/0a1b19a1a6f31bf6dfade6bcf5877531bea23daf
git-svn-id: svn://svn.chromium.org/blink/trunk@202208 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[Timothy Loh, 2015-09-03 02:05:53]

timloh@chromium.org changed reviewers:
+ alancutter@chromium.org, timloh@chromium.org

[Timothy Loh, 2015-09-03 02:05:53]

+alancutter

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

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:93: CSSParserValueList* valueList,
CSSParserTokenRange* range, const CSSParserContext& context,
Having the token range as a pointer is odd, I'd just pass it in by value (it's
only two pointers) and store it by value.

Also it would be nice to remove the CSSParserValueList argument and do the
conversion here instead of CSSParserImpl (this would make CSSParserValues.h only
included here after 1317743006)

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:6876: // Every comma-separated list
of identifiers is a valid will-change value,
Heh, the function doesn't actually do this...

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:6901:
values->append(cssValuePool().createIdentifierValue(currentToken.id()));
Maybe we should have something like:

static consumeIdent(CSSParserValueRange& range) {
    ASSERT(range.peek().type() == IdentToken);
    return
cssValuePool().createIdentifierValue(range.consumeIncludingWhitespace().id());
}

WDYT? Consuming early like we're doing here seems a bit strange to me.

[Timothy Loh, 2015-09-03 02:06:58]

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

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:6876: // Every comma-separated list
of identifiers is a valid will-change value,
On 2015/09/03 02:05:53, Timothy Loh wrote:
> Heh, the function doesn't actually do this...

That said, let's try and keep behaviour changes separate from moving properties
away from CSSParserValues.

[rwlbuis, 2015-09-03 22:27:54]

PTAL. Also I found webkitMarginCollapse as a possible nice shorthand to test in
the new path, hopefully a follow-up patch.

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

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:93: CSSParserValueList* valueList,
CSSParserTokenRange* range, const CSSParserContext& context,
On 2015/09/03 02:05:53, Timothy Loh wrote:
> Having the token range as a pointer is odd, I'd just pass it in by value (it's
> only two pointers) and store it by value.
> 
> Also it would be nice to remove the CSSParserValueList argument and do the
> conversion here instead of CSSParserImpl (this would make CSSParserValues.h
only
> included here after 1317743006)

Done.

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:6876: // Every comma-separated list
of identifiers is a valid will-change value,
On 2015/09/03 02:06:58, Timothy Loh wrote:
> On 2015/09/03 02:05:53, Timothy Loh wrote:
> > Heh, the function doesn't actually do this...
> 
> That said, let's try and keep behaviour changes separate from moving
properties
> away from CSSParserValues.

I agree. Sadly I don't think this is the only case of a comment not matching
what is happening...

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:6901:
values->append(cssValuePool().createIdentifierValue(currentToken.id()));
On 2015/09/03 02:05:53, Timothy Loh wrote:
> Maybe we should have something like:
> 
> static consumeIdent(CSSParserValueRange& range) {
>     ASSERT(range.peek().type() == IdentToken);
>     return
> cssValuePool().createIdentifierValue(range.consumeIncludingWhitespace().id());
> }
> 
> WDYT? Consuming early like we're doing here seems a bit strange to me.

Sounds good, I did that. A bit sad that we need consumeIncludingWhitespace in
two other places.

[Timothy Loh, 2015-09-04 01:24:40]

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

https://codereview.chromium.org/1319343004/diff/20001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:6901:
values->append(cssValuePool().createIdentifierValue(currentToken.id()));
On 2015/09/03 22:27:54, rwlbuis wrote:
> On 2015/09/03 02:05:53, Timothy Loh wrote:
> > Maybe we should have something like:
> > 
> > static consumeIdent(CSSParserValueRange& range) {
> >     ASSERT(range.peek().type() == IdentToken);
> >     return
> >
cssValuePool().createIdentifierValue(range.consumeIncludingWhitespace().id());
> > }
> > 
> > WDYT? Consuming early like we're doing here seems a bit strange to me.
> 
> Sounds good, I did that. A bit sad that we need consumeIncludingWhitespace in
> two other places.

Yeah, although I think it's more indicative that the code is doing something
weird. At least the first call should probably change to consumeIdent to fix the
FIXME. I suspect most other properties won't need consumeIncludingWhitespace all
over the place.

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

https://codereview.chromium.org/1319343004/diff/40001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:97: CSSParserValueList
valueList(range);
Probably OK for now but at some point this needs to happen *after* we call the
new-path parser, since this conversion has a pretty noticeable cost.

[rwlbuis, 2015-09-04 15:13:07]

On 2015/09/04 01:24:40, Timothy Loh wrote:
> > Sounds good, I did that. A bit sad that we need consumeIncludingWhitespace
in
> > two other places.
> 
> Yeah, although I think it's more indicative that the code is doing something
> weird. At least the first call should probably change to consumeIdent to fix
the
> FIXME. I suspect most other properties won't need consumeIncludingWhitespace
all
> over the place.

Did use consumeIdent in first call, now this works (like in FF):
$0.style.willChange = "auto"
"auto"
$0.style.willChange
"auto"

>
https://codereview.chromium.org/1319343004/diff/40001/Source/core/css/parser/...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1319343004/diff/40001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:97: CSSParserValueList
> valueList(range);
> Probably OK for now but at some point this needs to happen *after* we call the
> new-path parser, since this conversion has a pretty noticeable cost.

Agreed. Main problem with that atm is that the inherit/initial/unset checks need
m_valueList.

I wonder if we need a runtime flag for this?
Pro: no temporary speed loss because of if checks/case statement for the new
path. No risk of subtle bugs within the transition phase (passing all tests may
not be enough to catch everything).
Con: still have to support old path for a while so not able to remove code, only
add code.

[alancutter (OOO until 2018), 2015-09-08 00:10:48]

I would like to see a stronger separation between the token based and
CSSParserValueList based code paths, ideally as separate files so they can have
their own families of helper functions giving you more freedom to rethink how to
do things in case they can be made better with the token based approach.

That being said I just think it would be a nice to have and I'm not against the
current approach.

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

https://codereview.chromium.org/1319343004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:504: }
Why not pass everything through parseSingleValue()? We should avoid duplicating
the list of properties handled by parseSingleValue().

[rwlbuis, 2015-09-08 15:35:41]

Patchset #5 (id:80001) has been deleted

[rwlbuis, 2015-09-08 22:40:06]

I think the suggestion of seperating the new code path from the old makes a lot
of sense so I moved stuff into its own file.

This is probably the biggest patch we can do before adding validUnit logic.

PTAL.

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

https://codereview.chromium.org/1319343004/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:504: }
On 2015/09/08 00:10:48, alancutter wrote:
> Why not pass everything through parseSingleValue()? We should avoid
duplicating
> the list of properties handled by parseSingleValue().

Done.

[alancutter (OOO until 2018), 2015-09-09 04:45:26]

I think we should rename the old CSSPropertyParser LegacyCSSPropertyParser and
the new one CSSPropertyParser, WDYT?

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

https://codereview.chromium.org/1319343004/diff/160001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:59: #include
"core/css/parser/CSSPropertyParserNew.cpp"
Don't include CPP files, parseSingleValue() is defined in the header so this is
unnecessary.

[rwlbuis, 2015-09-09 23:14:36]

On 2015/09/09 04:45:26, alancutter wrote:
> I think we should rename the old CSSPropertyParser LegacyCSSPropertyParser and
> the new one CSSPropertyParser, WDYT?

Great idea, I did that in my latest patches.

>
https://codereview.chromium.org/1319343004/diff/160001/Source/core/css/parser...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1319343004/diff/160001/Source/core/css/parser...
> Source/core/css/parser/CSSPropertyParser.cpp:59: #include
> "core/css/parser/CSSPropertyParserNew.cpp"
> Don't include CPP files, parseSingleValue() is defined in the header so this
is
> unnecessary.

Fixed.

Note that at least going by the two shorthand props I ported, there are good
chances ShorthandScope should not be needed indeed.

The other thing with my latest patch, I am not sure if the parse functions
called by parseSingleValue should be static free functions or methods, I prefer
free functions myself. Also maybe the parseFoo methods/functions should really
be consumeFoo methods/functions?

PTAL.

[Timothy Loh, 2015-09-10 01:13:37]

parseFoo -> consumeFoo SGTM. Static functions also SGTM.

I think we should move the driving code (the ctor and entry parseValue) to the
non-legacy file. The calling order should probably be new path calls into legacy
path if it can't handle a property.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSParserToken.cpp:116: if (m_type != IdentToken &&
m_type != FunctionToken)
Can we split this into id() and functionId()? Otherwise something like "page:
auto(" would be accepted.

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

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015 :-)

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:29: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumePrimitiveCustomIdentValue(CSSParserTokenRange& range)
I'd prefer consumeCustomIdent and consumeString, WDYT? These might end up being
moved out of CSSPrimitiveValue some time anyway
(https://codereview.chromium.org/1306823004/)

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:31: ASSERT(range.peek().unitType()
== CSSPrimitiveValue::UnitType::String || range.peek().type() == IdentToken);
shouldn't be a string

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:37: ASSERT(range.peek().type() ==
StringToken || range.peek().type() == IdentToken);
shouldn't be an ident :-)

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:150: if
(!CSSParserFastPaths::isValidKeywordPropertyAndValue(webkitMarginCollapseShorthand().properties()[0],
id))
let's just put the actual property ids :)

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:154: CSSValue* value =
m_parsedProperties.last().value();
Not really a fan of accessing the parsedProperties vector. How about:

RefPtrWillBeRawPtr<CSSValue> beforeCollapse = consumeIdent();
if (!beforeCollapse || !..validKeyword..(CSSPropertyWebkitMarginBeforeCollapse,
beforeCollapse->getValueID()))
    return false;
addProperty(CSSPropertyWebkitMarginBeforeColapse, beforeCollapse);

(and similar for afterCollapse, overflow for consistency)

[rwlbuis, 2015-09-10 21:52:42]

> parseFoo -> consumeFoo SGTM. Static functions also SGTM.

Done.

>I think we should move the driving code (the ctor and entry >parseValue) to the
>non-legacy file. The calling order should probably be new >path calls into
legacy
>path if it can't handle a property.

I did most of this. I prefer to change the calling order a bit later once the
conversion is more complete.

PTAL.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSParserToken.cpp:116: if (m_type != IdentToken &&
m_type != FunctionToken)
On 2015/09/10 01:13:37, Timothy Loh wrote:
> Can we split this into id() and functionId()? Otherwise something like "page:
> auto(" would be accepted.

Good idea, done. I also changed a few places outside of the property parser to
use these.

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

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/09/10 01:13:37, Timothy Loh wrote:
> 2015 :-)

Done.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:29: static
PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
consumePrimitiveCustomIdentValue(CSSParserTokenRange& range)
On 2015/09/10 01:13:37, Timothy Loh wrote:
> I'd prefer consumeCustomIdent and consumeString, WDYT? These might end up
being
> moved out of CSSPrimitiveValue some time anyway
> (https://codereview.chromium.org/1306823004/)

Done.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:31: ASSERT(range.peek().unitType()
== CSSPrimitiveValue::UnitType::String || range.peek().type() == IdentToken);
On 2015/09/10 01:13:37, Timothy Loh wrote:
> shouldn't be a string

Done.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:37: ASSERT(range.peek().type() ==
StringToken || range.peek().type() == IdentToken);
On 2015/09/10 01:13:37, Timothy Loh wrote:
> shouldn't be an ident :-)

Done.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:150: if
(!CSSParserFastPaths::isValidKeywordPropertyAndValue(webkitMarginCollapseShorthand().properties()[0],
id))
On 2015/09/10 01:13:37, Timothy Loh wrote:
> let's just put the actual property ids :)

Done.

https://codereview.chromium.org/1319343004/diff/210001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:154: CSSValue* value =
m_parsedProperties.last().value();
On 2015/09/10 01:13:37, Timothy Loh wrote:
> Not really a fan of accessing the parsedProperties vector. How about:
> 
> RefPtrWillBeRawPtr<CSSValue> beforeCollapse = consumeIdent();
> if (!beforeCollapse ||
!..validKeyword..(CSSPropertyWebkitMarginBeforeCollapse,
> beforeCollapse->getValueID()))
>     return false;
> addProperty(CSSPropertyWebkitMarginBeforeColapse, beforeCollapse);
> 
> (and similar for afterCollapse, overflow for consistency)

I removed the parsedProperties usage. I don't think we can use consumeIdent as
above since it could ASSERT.

[alancutter (OOO until 2018), 2015-09-11 07:53:49]

Overall approach lgtm.

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

https://codereview.chromium.org/1319343004/diff/250001/Source/core/css/parser...
Source/core/css/parser/CSSPropertyParser.cpp:75: ASSERT(range.peek().type() ==
IdentToken);
Let's just return nullptr, that way we can avoid duplicating this check
everywhere that calls this function. Ditto for the other consumeX() functions
that currently ASSERT.

https://codereview.chromium.org/1319343004/diff/250001/Source/core/css/resolv...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1319343004/diff/250001/Source/core/css/resolv...
Source/core/css/resolver/StyleBuilderCustom.cpp:680: else if
(primitiveValue->getValueID() != CSSValueAuto)
It's weird to be changing behaviour in a refactor change. This should be moved
to a separate patch.
Also should there be updates to test cases if it's changing?

[rwlbuis, 2015-09-11 14:38:41]

Patchset #15 (id:290001) has been deleted

[rwlbuis, 2015-09-14 10:51:12]

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

[rwlbuis, 2015-09-14 10:51:12]

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

[commit-bot: I haz the power, 2015-09-14 10:51:20]

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

[commit-bot: I haz the power, 2015-09-14 11:30:34]

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

[commit-bot: I haz the power, 2015-09-14 11:30:35]

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

[rwlbuis, 2015-09-14 12:29:16]

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

[commit-bot: I haz the power, 2015-09-14 12:29:28]

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

[commit-bot: I haz the power, 2015-09-14 13:14:34]

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

[commit-bot: I haz the power, 2015-09-14 13:14:35]

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

[rwlbuis, 2015-09-14 13:57:55]

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

[commit-bot: I haz the power, 2015-09-14 13:58:07]

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

[commit-bot: I haz the power, 2015-09-14 14:39:31]

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

[commit-bot: I haz the power, 2015-09-14 14:39:31]

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

[rwlbuis, 2015-09-14 14:57:58]

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

[commit-bot: I haz the power, 2015-09-14 14:58:02]

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

[commit-bot: I haz the power, 2015-09-14 15:41:39]

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

[commit-bot: I haz the power, 2015-09-14 15:41:40]

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

[rwlbuis, 2015-09-14 17:15:07]

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

[commit-bot: I haz the power, 2015-09-14 17:15:31]

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

[commit-bot: I haz the power, 2015-09-14 17:37:57]

Committed patchset #14 (id:270001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202208

[commit-bot: I haz the power, 2015-09-23 12:33:48]

Patchset 14 (id:??) landed as
https://crrev.com/0a1b19a1a6f31bf6dfade6bcf5877531bea23daf