CSS Tokenizer: Fix edge cases in number parsing

CSS Tokenizer: Fix edge cases in number parsing

This patch fixes a couple of small issues in the css-syntax number
parsing routine:

1) We shouldn't consume a "." if the next character isn't a digit
2) If any exponent argument is present we should use a 'number'
type (in the code we just check if we consumed anything after the
integer portion)

http://dev.w3.org/csswg/css-syntax/#consume-number

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

[Timothy Loh, 2014-10-20 03:03:51]

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

[Timothy Loh, 2014-10-20 03:27:02]

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

[Yoav Weiss, 2014-10-20 07:06:46]

LGTM

I think I understand the change, but added a couple of questions just in case.
You need to fix CSSParserToken so that the tests would pass on all locales.

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizer.cpp (left):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizer.cpp:289: }
So beyond the refactoring (which I like), and the elimination of digitsNumber,
there is no functionality change here, right?

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizer.cpp (right):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizer.cpp:329: if (offset != integerPartEndOffset)
This is where the magic happens, and exponents are turned into a number, right?

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizerTest.cpp (right):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizerTest.cpp:31: { "(max-width: 1000e-1px)",
"(max-width: 100.000000px)" },
These tests would fail on some system locales, where '.' is replaced by ','.
You should add "setlocale(LC_NUMERIC, "en_US.UTF-8");" to
CSSParserToken::textForUnitTests (and include <locale.h>) to make this go away.

[Timothy Loh, 2014-10-20 07:18:24]

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizer.cpp (left):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizer.cpp:289: }
On 2014/10/20 07:06:45, Yoav Weiss wrote:
> So beyond the refactoring (which I like), and the elimination of digitsNumber,
> there is no functionality change here, right?

There's a change here which is to only consume the '.' when the following
character is a digit.

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizer.cpp (right):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizer.cpp:329: if (offset != integerPartEndOffset)
On 2014/10/20 07:06:45, Yoav Weiss wrote:
> This is where the magic happens, and exponents are turned into a number,
right?

Yep

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizerTest.cpp (right):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizerTest.cpp:31: { "(max-width: 1000e-1px)",
"(max-width: 100.000000px)" },
On 2014/10/20 07:06:46, Yoav Weiss wrote:
> These tests would fail on some system locales, where '.' is replaced by ','.
> You should add "setlocale(LC_NUMERIC, "en_US.UTF-8");" to
> CSSParserToken::textForUnitTests (and include <locale.h>) to make this go
away.

Good catch. There's probably a more appropriate float->string helper in WTF
somewhere, I'll have a look.

[Yoav Weiss, 2014-10-20 07:26:14]

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
File Source/core/css/parser/CSSTokenizer.cpp (left):

https://codereview.chromium.org/662343002/diff/1/Source/core/css/parser/CSSTo...
Source/core/css/parser/CSSTokenizer.cpp:289: }
On 2014/10/20 07:18:24, Timothy Loh wrote:
> On 2014/10/20 07:06:45, Yoav Weiss wrote:
> > So beyond the refactoring (which I like), and the elimination of
digitsNumber,
> > there is no functionality change here, right?
> 
> There's a change here which is to only consume the '.' when the following
> character is a digit.

Oh, right. So the move from 'isASCIIDigit(input.peek(++offset))' to
'isASCIIDigit(input.peek(offset + 1))' is the functional change here.

[Timothy Loh, 2014-10-20 12:59:55]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-10-20 13:00:50]

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

[commit-bot: I haz the power, 2014-10-20 13:04:19]

Committed patchset #1 (id:1) as 183979