[webkit-reviews] review denied: [Bug 96130] Replace WTF::numberTo*String() with String::number[ToStringECMAScript]() : [Attachment 162805] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 96130: Replace WTF::numberTo*String() with
String::number[ToStringECMAScript]()
https://bugs.webkit.org/show_bug.cgi?id=96130

Attachment 162805: Patch
https://bugs.webkit.org/attachment.cgi?id=162805&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162805&action=review


On NumberPrototype and V8DependentRetained, I don't think we can afford one
extra function call + the extra branch in String::number().

> Source/WebCore/css/CSSPrimitiveValue.cpp:1028
> -		   NumberToStringBuffer buffer;
> -		   const char* alphaString =
numberToFixedPrecisionString(color.alpha() / 255.0f, 6, buffer, true);
> -		   result.append(alphaString, strlen(alphaString));
> +		   String alphaString = String::number(color.alpha() / 255.0f);

> +		   result.append(alphaString.characters8(),
alphaString.length());

This is also a regression.
Previously, we would only allocate NumberToStringBuffer on the stack. With this
change, we will allocate a new StringImpl, then copy the content of the buffer
to it.

I am also not a fan of relying on the implementation generating a 8bits buffer.
It is the case today but that could change in the future.

Given that someone took the time to write the original code is such a contrived
way, I assume the performance is important here.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:90
> -    NumberToStringBuffer buffer;
> -    return String(numberToString(number, buffer));
> +    return String::numberToStringECMAScript(number);

This looks like a good idea. It makes the code simpler and will enjoy
https://bugs.webkit.org/show_bug.cgi?id=96132 for free.

> Source/WebCore/platform/Decimal.cpp:685
> -    if (isfinite(doubleValue)) {
> -	   NumberToStringBuffer buffer;
> -	   return fromString(numberToString(doubleValue, buffer));
> -    }
> +    if (isfinite(doubleValue))
> +	   return fromString(String::numberToStringECMAScript(doubleValue));

Ditto.