Pull CSS-wide keyword handling out of LegacyCSSPropertyParser

Pull CSS-wide keyword handling out of LegacyCSSPropertyParser

This patch moves the handling for the css-wide keywords (initial,
inherit and unset) out of the LegacyCSSPropertyParser and into the
CSSPropertyParser where it can be parsed with CSSParserTokens. This
patch also adds an additional function between the public parseValue
and the internal parseValue.

Subsequent patches will make this new function also responsible for
handling CSS variables and calling out to parseSingleValue and
parseShorthand, instead of having these called from the legacy paths.

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

[Timothy Loh, 2015-11-20 07:37:42]

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

[Timothy Loh, 2015-11-20 07:37:42]

First of three patches (mostly I'm procrastinating parsing <position>...)

[rwlbuis, 2015-11-20 14:13:08]

On 2015/11/20 07:37:42, Timothy Loh wrote:
> First of three patches (mostly I'm procrastinating parsing <position>...)

Isn't converting border independent of <position>? That one is valuable too.

[rwlbuis, 2015-11-20 14:19:16]

I like the patch in general. Postponing the creation of CSSParserValueList can
only be a good thing :)

https://codereview.chromium.org/1459633004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1459633004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:90:
CSSParserTokenRange rangeCopy = m_range;
I know this is not very costly, but still I don't like it. Do you think we can
do the consumeWhitespace TODO above in this here patch to avoid it?

[Timothy Loh, 2015-11-24 03:34:24]

https://codereview.chromium.org/1459633004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1459633004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:90:
CSSParserTokenRange rangeCopy = m_range;
On 2015/11/20 14:19:16, rwlbuis (OOO - review only) wrote:
> I know this is not very costly, but still I don't like it. Do you think we can
> do the consumeWhitespace TODO above in this here patch to avoid it?

https://codereview.chromium.org/1471933003/ is in the cq for the
consumeWhitespace, but the copy is still needed so we don't actually consume the
first value if we don't just have a css-wide keyword.

[Timothy Loh, 2015-11-24 05:34:35]

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

[alancutter (OOO until 2018), 2015-11-24 05:54:30]

lgtm

https://codereview.chromium.org/1459633004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1459633004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:76: bool
CSSPropertyParser::parseValueStart(CSSPropertyID unresolvedProperty, bool
important)
I don't like this name, after you've moved stuff we want to keep out of the
legacy file you should rename this back to parseValue() and call the old one
legacyParseValue() or something similar.

[rwlbuis, 2015-11-24 14:16:45]

On 2015/11/24 03:34:24, Timothy Loh wrote:
>
https://codereview.chromium.org/1459633004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/1459633004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:90:
> CSSParserTokenRange rangeCopy = m_range;
> On 2015/11/20 14:19:16, rwlbuis (OOO - review only) wrote:
> > I know this is not very costly, but still I don't like it. Do you think we
can
> > do the consumeWhitespace TODO above in this here patch to avoid it?
> 
> https://codereview.chromium.org/1471933003/ is in the cq for the
> consumeWhitespace, but the copy is still needed so we don't actually consume
the
> first value if we don't just have a css-wide keyword.

Great CL. Well you could peek() first and consume after the return false, this
is what I'd prefer. I won't want to block this though, up to you.

[rwlbuis, 2015-11-24 14:17:01]

lgtm

[Timothy Loh, 2015-11-24 23:48:22]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2015-11-24 23:51:05]

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

[commit-bot: I haz the power, 2015-11-25 01:30:46]

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

[commit-bot: I haz the power, 2015-11-25 01:30:47]

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

[Timothy Loh, 2015-11-25 02:23:09]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2015-11-25 02:23:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, rob.buis@samsung.com
Link to the patchset: https://codereview.chromium.org/1459633004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-25 02:25:07]

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

[commit-bot: I haz the power, 2015-11-25 05:03:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-25 05:04:24]

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