CSS Parser: Implement parseValue

CSS Parser: Implement parseValue

This patch begins the parser portion of the CSS parser rewrite. To avoid
making a copy of the CSSPropertyParser, we temporarily add a conversion
from the new CSSParserTokens to CSSParserValues. Note the runtime flag
is not marked as test/experimental, so this code is not yet being run.

This patch passes all tests except CSSPropertyParserTest.GridTrackLimits
and fast/css-grid-layout/named-grid-line-get-set.html, which will be
fixed in a later patch. The former fails due to bad handling of large
numbers in the new tokenizer, and the latter fails as the Bison CSS
parser is a bit more restrictive in handling brackets.

BUG=330389
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185003

[Timothy Loh, 2014-10-29 05:53:13]

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

[alancutter (OOO until 2018), 2014-11-07 19:36:12]

There should really be unit tests added here for the odd cases that aren't
likely to be tested by LayoutTests eg. "f(calc((45 + var(--x)) / (17 - attr(y".
I suspect this should parse but a unit test would say for sure and make
understanding this code easier.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:35: {
ASSERT(end.type() == EOFToken) ?

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:50: || ((it + 3)->type() ==
RightParenthesisToken && (it + 2)->type() == WhitespaceToken)) {
These iterator accesses need to be swapped around to catch the EOF correctly and
avoid an out of bounds access.
Given the nature of the bug we ought to have unit tests to catch these.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:84: if (!calcDepth) {
calcDepth == 0

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:97: if (!bracketCounts.last()) {
backetCounts.last() == 0

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:111: }
ASSERT(calcDepth > 0) for readability.

[Timothy Loh, 2014-11-08 00:42:49]

When we're ready to switch on the new parser every property value in all the
layout tests will be going through this code path, which seems like adequate
testing to me. CSSParserValues aren't built up for testing, so to properly test
this we'd need to make it much easier to manually build these and also to
compare them. The conversion is also only temporary and will be removed shortly
after the new parser is enabled. It seems to me that unit testing isn't worth
the effort overall.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:35: {
On 2014/11/07 19:36:12, alancutter wrote:
> ASSERT(end.type() == EOFToken) ?

It basically never makes sense to deference an end iterator. Actually in the
future other codepaths will call into here with other just-past-the-end tokens;
probably including ;, !, ), }.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:50: || ((it + 3)->type() ==
RightParenthesisToken && (it + 2)->type() == WhitespaceToken)) {
On 2014/11/07 19:36:11, alancutter wrote:
> These iterator accesses need to be swapped around to catch the EOF correctly
and
> avoid an out of bounds access.
> Given the nature of the bug we ought to have unit tests to catch these.

I tweaked the logic a bit here, ptal.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:84: if (!calcDepth) {
On 2014/11/07 19:36:12, alancutter wrote:
> calcDepth == 0

Done.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:97: if (!bracketCounts.last()) {
On 2014/11/07 19:36:12, alancutter wrote:
> backetCounts.last() == 0

Done.

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:111: }
On 2014/11/07 19:36:12, alancutter wrote:
> ASSERT(calcDepth > 0) for readability.

Done.

[alancutter (OOO until 2018), 2014-11-08 00:59:08]

lgtm

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/647483009/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:50: || ((it + 3)->type() ==
RightParenthesisToken && (it + 2)->type() == WhitespaceToken)) {
On 2014/11/08 00:42:49, Timothy Loh wrote:
> On 2014/11/07 19:36:11, alancutter wrote:
> > These iterator accesses need to be swapped around to catch the EOF correctly
> and
> > avoid an out of bounds access.
> > Given the nature of the bug we ought to have unit tests to catch these.
> 
> I tweaked the logic a bit here, ptal.

This is better.

[Timothy Loh, 2014-11-08 01:02:02]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-11-08 01:03:18]

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

[commit-bot: I haz the power, 2014-11-08 01:03:20]

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

[commit-bot: I haz the power, 2014-11-08 02:59:10]

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

[commit-bot: I haz the power, 2014-11-08 02:59:11]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35423)

[Timothy Loh, 2014-11-08 04:28:12]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-11-08 04:28:44]

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

[commit-bot: I haz the power, 2014-11-08 06:01:12]

Committed patchset #4 (id:60001) as 185003