CSS Tokenizer: Comprehensive unit tests

CSS Tokenizer: Comprehensive unit tests

This patch adds comprehensive unit tests to the css-syntax based
tokenizer. I've tried to cover all the edge cases I could think of here.

A few cases don't work correctly, which I've left as commented out
tests:
- Idents starting with two dashes like --abc aren't parsed as idents
- Escape should be replaced with U+FFFD when they represent either
(U+0000), a surrogate codepoint (U+D800 - U+DFFF) or are larger than
the maximum allowed codepoint (U+110000).
- We don't correctly replace U+0000 with U+FFFD
- Our handling of newline escapes (i.e. "check if two code points are a
valid escape") is incorrect in some cases as we don't perform
preprocessing.

Note that there have been minor spec changes since the candidate rec and
the tests here are based on the latest editor's draft.

http://dev.w3.org/csswg/css-syntax

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

[Timothy Loh, 2014-10-23 00:31:49]

Patchset #1 (id:1) has been deleted

[Timothy Loh, 2014-10-23 00:37:08]

timloh@chromium.org changed reviewers:
+ ikilpatrick@chromium.org, yoav@yoav.ws

[Yoav Weiss, 2014-10-23 06:37:14]

Absolutely awesome! 

I love the fact there's no test specific code in the implementation files now,
and it'd be easy to test new tokens, with no need for hacks.

LGTM

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

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:30: 
This might be slightly more readable if split out into its own function. Not
critical though.

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:89: static const CSSParserToken
whitespace(WhitespaceToken);
This fails to build. I think I've resolved something similar in the past by
using DEFINE_STATIC_LOCAL

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:148: TEST_TOKENS("spac\\65\n\rs",
ident("space"), whitespace, ident("s"));
"spac\\65\r\ns" as ident("spaces") would be interesting here as well.

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:241: TEST_TOKENS("-12.34E+2",
number(NumberValueType, -1234));
Can you add tests for something like "1." and "1.e2"? I know we fail these in
HTML and interpret them as a number.

[Timothy Loh, 2014-10-23 16:17:58]

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

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:30: 
On 2014/10/23 06:37:14, Yoav Weiss wrote:
> This might be slightly more readable if split out into its own function. Not
> critical though.

Pulled out token comparison into a separate function

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:89: static const CSSParserToken
whitespace(WhitespaceToken);
On 2014/10/23 06:37:14, Yoav Weiss wrote:
> This fails to build. I think I've resolved something similar in the past by
> using DEFINE_STATIC_LOCAL

Done. It compiled fine on g++ :/

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:148: TEST_TOKENS("spac\\65\n\rs",
ident("space"), whitespace, ident("s"));
On 2014/10/23 06:37:14, Yoav Weiss wrote:
> "spac\\65\r\ns" as ident("spaces") would be interesting here as well.

Done.

https://codereview.chromium.org/646893003/diff/40001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:241: TEST_TOKENS("-12.34E+2",
number(NumberValueType, -1234));
On 2014/10/23 06:37:13, Yoav Weiss wrote:
> Can you add tests for something like "1." and "1.e2"? I know we fail these in
> HTML and interpret them as a number.

There's already a test "13.", but I've added "1.e2" to the list too.

[Timothy Loh, 2014-10-23 16:18:04]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-10-23 16:18:49]

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

[commit-bot: I haz the power, 2014-10-23 19:09:37]

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

[commit-bot: I haz the power, 2014-10-23 19:09:39]

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

[ikilpatrick, 2014-10-23 22:24:02]

lgtm

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSTokenizerTest.cpp (right):

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:203: TEST_TOKENS("fun\\(ction(",
function("fun(ction"));
worth testing more esorteric names like: "--bar(" etc? Or little risk of this?
Up to you.

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:222: TEST_TOKENS("'bad\fstring",
badString, whitespace, ident("string"));
Worth adding a mismatched start/end char string here?

I.e. 'mismatch"

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:251: TEST_TOKENS("1.e2",
number(IntegerValueType, 1), delim('.'), ident("e2"));
test for:

"1e.5", "1e1.5", "1e5." etc.

[Timothy Loh, 2014-10-24 00:29:01]

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSTokenizerTest.cpp (right):

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:203: TEST_TOKENS("fun\\(ction(",
function("fun(ction"));
On 2014/10/23 22:24:02, ikilpatrick wrote:
> worth testing more esorteric names like: "--bar(" etc? Or little risk of this?
> Up to you.

This doesn't really add any coverage.

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:222: TEST_TOKENS("'bad\fstring",
badString, whitespace, ident("string"));
On 2014/10/23 22:24:02, ikilpatrick wrote:
> Worth adding a mismatched start/end char string here?
> 
> I.e. 'mismatch"
> 

Done (but basically tested by "'quotes'" and '"quotes"' already)

https://codereview.chromium.org/646893003/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSTokenizerTest.cpp:251: TEST_TOKENS("1.e2",
number(IntegerValueType, 1), delim('.'), ident("e2"));
On 2014/10/23 22:24:02, ikilpatrick wrote:
> test for:
> 
> "1e.5", "1e1.5", "1e5." etc.

Done.

[Timothy Loh, 2014-10-24 00:29:06]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-10-24 00:30:25]

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

[commit-bot: I haz the power, 2014-10-24 01:27:55]

Committed patchset #5 (id:100001) as 184322