Fixed an assert related to MediaQuerytokenizer code points table

Fixed an assert related to MediaQuerytokenizer code points table

The assert was a result of an off-by-one error in both the array's definition and the assert itself.

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

[eseidel, 2014-03-18 19:27:49]

Needs a test.

Also, why the +1?

[eseidel, 2014-03-18 19:28:17]

Are codepoints 1-indexed instead of zero indexed somehow?

[Yoav Weiss, 2014-03-18 19:49:42]

On 2014/03/18 19:27:49, eseidel wrote:
> Needs a test.
> 
> Also, why the +1?

The codepoints are from 0 to 127. SCHAR_MAX is 127, so the codepoints array
should be defined to 128.
The problem is that I did not define memset to set 128 zeros in that array. I've
fixed that now.

I'll also add a test.

[eseidel, 2014-03-18 19:53:00]

Still need a test, but OK.

[Yoav Weiss, 2014-03-18 21:19:20]

On 2014/03/18 19:53:00, eseidel wrote:
> Still need a test, but OK.

Added a test.

[eseidel, 2014-03-18 21:23:33]

This is not a good test.  It's just copy/paste from the implementation.

Better would be to test something more generic, like to have a for loop which
walked the whole table and checked that each entry had a value, for instance.

[Yoav Weiss, 2014-03-18 21:29:02]

On 2014/03/18 21:23:33, eseidel wrote:
> This is not a good test.  It's just copy/paste from the implementation.

It isn't a copy/paste, but I'd agree that it's a higher level test (it checks
the output token type, rather than the table's codepoint value)
> 
> Better would be to test something more generic, like to have a for loop which
> walked the whole table and checked that each entry had a value, for instance.

The table itself is private. Should I make it public in order for it to be
testable on that level?

[eseidel, 2014-03-18 21:29:07]

Oh sorry.  I misread your test.  This is great actually.

[eseidel, 2014-03-18 21:29:52]

https://codereview.chromium.org/203343004/diff/40001/Source/core/css/parser/M...
File Source/core/css/parser/MediaQueryTokenizerTest.cpp (right):

https://codereview.chromium.org/203343004/diff/40001/Source/core/css/parser/M...
Source/core/css/parser/MediaQueryTokenizerTest.cpp:64: testToken(1098,
IdentToken);
So everything above SCHAR_MAX is an IdentToken, correct?

[Yoav Weiss, 2014-03-18 21:31:41]

On 2014/03/18 21:29:52, eseidel wrote:
>
https://codereview.chromium.org/203343004/diff/40001/Source/core/css/parser/M...
> File Source/core/css/parser/MediaQueryTokenizerTest.cpp (right):
> 
>
https://codereview.chromium.org/203343004/diff/40001/Source/core/css/parser/M...
> Source/core/css/parser/MediaQueryTokenizerTest.cpp:64: testToken(1098,
> IdentToken);
> So everything above SCHAR_MAX is an IdentToken, correct?

Yeah. I just picked a random number to test that. should I add a comment?

[eseidel, 2014-03-18 21:32:10]

This test is fine.  I just read it too quickly the first pass.  This is a great
way to test this, to loop over all possible values and make sure they fall into
a known bucket.

I might have changed the for loop to have an absolute value for the stop, like
"1000" and then have an if in teh body which is if (c > SCHAR_MAX) test(Ident);

Which would have made clear that everything above SCHAR_MAX is ident, but this
is also fine.

You might also test values at the extermes of your input, like UNIT_MAX or
whatever the largest input value is OK.

[eseidel, 2014-03-18 21:32:23]

lgtm

[Yoav Weiss, 2014-03-18 21:54:08]

On 2014/03/18 21:32:10, eseidel wrote:
> This test is fine.  I just read it too quickly the first pass.  This is a
great
> way to test this, to loop over all possible values and make sure they fall
into
> a known bucket.
> 
> I might have changed the for loop to have an absolute value for the stop, like
> "1000" and then have an if in teh body which is if (c > SCHAR_MAX)
test(Ident);
> 
> Which would have made clear that everything above SCHAR_MAX is ident, but this
> is also fine.
> 
> You might also test values at the extermes of your input, like UNIT_MAX or
> whatever the largest input value is OK.

Modified the test according to your suggestions.

[Yoav Weiss, 2014-03-18 21:55:02]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-03-18 21:55:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/203343004/60001

[commit-bot: I haz the power, 2014-03-18 22:57:20]

Change committed as 169494