Simplify CSSParserToken::textForUnitTests by using more WTF helpers

Simplify CSSParserToken::textForUnitTests by using more WTF helpers

Instead of manually handling char arrays and using sprintf in converting
CSSParserTokens to strings, we should just concatenate Strings and use
String::number. I didn't bother using StringBuilder here since the
function is only used in unit tests and it's a bit more readable this way.

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

[Timothy Loh, 2014-10-20 07:41:15]

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

[Yoav Weiss, 2014-10-20 08:04:43]

All in all looks good, but printing of doubles gets the wrong precision.

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
Source/core/css/parser/CSSParserToken.cpp:99: return
String::number(m_numericValue, 6, KeepTrailingZeros) + unit;
If I understand correctly the intention here is to leave 6 fractional digits. It
might be better to add that value as a const.

Otherwise, trying this patch along with
https://codereview.chromium.org/662343002/ it seems to truncate the overall
number to 6 digit precision, not just the fractional part. I took a quick look
into dtoa to see why this is happening, but it seems it requires more than a
quick look.

[Timothy Loh, 2014-10-20 08:16:39]

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
Source/core/css/parser/CSSParserToken.cpp:99: return
String::number(m_numericValue, 6, KeepTrailingZeros) + unit;
On 2014/10/20 08:04:43, Yoav Weiss wrote:
> If I understand correctly the intention here is to leave 6 fractional digits.
It
> might be better to add that value as a const.
> 
> Otherwise, trying this patch along with
> https://codereview.chromium.org/662343002/ it seems to truncate the overall
> number to 6 digit precision, not just the fractional part. I took a quick look
> into dtoa to see why this is happening, but it seems it requires more than a
> quick look.

Would it be clearer with "const int significantDigits = 6"? I don't think it
matters much whether we have 6 fractional digits or 6 significant digits (or
some other number of digits).

The other patch will of course need to be update to expect "500.000", etc.

[Yoav Weiss, 2014-10-20 08:38:24]

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
Source/core/css/parser/CSSParserToken.cpp:99: return
String::number(m_numericValue, 6, KeepTrailingZeros) + unit;
On 2014/10/20 08:16:38, Timothy Loh wrote:
> On 2014/10/20 08:04:43, Yoav Weiss wrote:
> > If I understand correctly the intention here is to leave 6 fractional
digits.
> It
> > might be better to add that value as a const.
> > 
> > Otherwise, trying this patch along with
> > https://codereview.chromium.org/662343002/ it seems to truncate the overall
> > number to 6 digit precision, not just the fractional part. I took a quick
look
> > into dtoa to see why this is happening, but it seems it requires more than a
> > quick look.
> 
> Would it be clearer with "const int significantDigits = 6"? I don't think it
> matters much whether we have 6 fractional digits or 6 significant digits (or
> some other number of digits).
> 
> The other patch will of course need to be update to expect "500.000", etc.

Maybe replace this line with 'return
String::numberToStringFixedWidth(m_numericValue, fractionalDigits) + unit;' ?
(while adding a fractionalDigits const)

That seems to give us a fixed number of fractional digits.

[Timothy Loh, 2014-10-20 10:52:32]

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
File Source/core/css/parser/CSSParserToken.cpp (right):

https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
Source/core/css/parser/CSSParserToken.cpp:99: return
String::number(m_numericValue, 6, KeepTrailingZeros) + unit;
On 2014/10/20 08:38:24, Yoav Weiss wrote:
> On 2014/10/20 08:16:38, Timothy Loh wrote:
> > On 2014/10/20 08:04:43, Yoav Weiss wrote:
> > > If I understand correctly the intention here is to leave 6 fractional
> digits.
> > It
> > > might be better to add that value as a const.
> > > 
> > > Otherwise, trying this patch along with
> > > https://codereview.chromium.org/662343002/ it seems to truncate the
overall
> > > number to 6 digit precision, not just the fractional part. I took a quick
> look
> > > into dtoa to see why this is happening, but it seems it requires more than
a
> > > quick look.
> > 
> > Would it be clearer with "const int significantDigits = 6"? I don't think it
> > matters much whether we have 6 fractional digits or 6 significant digits (or
> > some other number of digits).
> > 
> > The other patch will of course need to be update to expect "500.000", etc.
> 
> Maybe replace this line with 'return
> String::numberToStringFixedWidth(m_numericValue, fractionalDigits) + unit;' ?
> (while adding a fractionalDigits const)
> 
> That seems to give us a fixed number of fractional digits.

Done.

[Yoav Weiss, 2014-10-20 10:59:46]

On 2014/10/20 10:52:32, Timothy Loh wrote:
>
https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
> File Source/core/css/parser/CSSParserToken.cpp (right):
> 
>
https://codereview.chromium.org/667663002/diff/1/Source/core/css/parser/CSSPa...
> Source/core/css/parser/CSSParserToken.cpp:99: return
> String::number(m_numericValue, 6, KeepTrailingZeros) + unit;
> On 2014/10/20 08:38:24, Yoav Weiss wrote:
> > On 2014/10/20 08:16:38, Timothy Loh wrote:
> > > On 2014/10/20 08:04:43, Yoav Weiss wrote:
> > > > If I understand correctly the intention here is to leave 6 fractional
> > digits.
> > > It
> > > > might be better to add that value as a const.
> > > > 
> > > > Otherwise, trying this patch along with
> > > > https://codereview.chromium.org/662343002/ it seems to truncate the
> overall
> > > > number to 6 digit precision, not just the fractional part. I took a
quick
> > look
> > > > into dtoa to see why this is happening, but it seems it requires more
than
> a
> > > > quick look.
> > > 
> > > Would it be clearer with "const int significantDigits = 6"? I don't think
it
> > > matters much whether we have 6 fractional digits or 6 significant digits
(or
> > > some other number of digits).
> > > 
> > > The other patch will of course need to be update to expect "500.000", etc.
> > 
> > Maybe replace this line with 'return
> > String::numberToStringFixedWidth(m_numericValue, fractionalDigits) + unit;'
?
> > (while adding a fractionalDigits const)
> > 
> > That seems to give us a fixed number of fractional digits.
> 
> Done.

LGTM

[Timothy Loh, 2014-10-20 11:00:42]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2014-10-20 11:01:41]

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

[commit-bot: I haz the power, 2014-10-20 12:17:44]

Committed patchset #2 (id:20001) as 183975