[webkit-reviews] review denied: [Bug 77018] implement Number.toLocaleString() using ICU : [Attachment 124001] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Eli Fidler <efidler at rim.com>'s request
for review:
Bug 77018: implement Number.toLocaleString() using ICU
https://bugs.webkit.org/show_bug.cgi?id=77018

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124001&action=review


> Source/JavaScriptCore/ChangeLog:4
> +	   Implement Number.toLocaleString() using ICU
> +	   https://bugs.webkit.org/show_bug.cgi?id=77018

Changes like this need test coverage.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:70
> +    UErrorCode status = U_ZERO_ERROR;
> +    UNumberFormat* nf = unum_open(UNUM_DEFAULT, 0, 0, 0, 0, &status);
> +    if (!nf || U_FAILURE(status))
> +	   return jsEmptyString(exec);

To make this useful for platforms other than RIM, this should pass in a default
locale. While both POSIX and ICU have a default locale concept, it’s a global
concept so on most platforms it’s not something that WebKit can set without
affecting other code running in the same process.

Because of that, we should continue to follow the design we have in WebCore
where we have a platform-specific function to return the appropriate locale for
ICU usage. I didn’t insist on this for the last patch because it was a function
with locale-specific implementations for many platforms already, and the old
code that the ICU code was replacing was using a POSIX call. But here we are
doing a new implementation used by many platforms and so I think proper locale
handling for WebKit is a must.

Creating a new UNumberFormat object each time formatLocaleNumber is called is
likely to be extremely slow. We should do an implementation where it is cached.


> Source/JavaScriptCore/wtf/DecimalNumber.cpp:164
> +unsigned DecimalNumber::toStringDecimal(char* buffer, unsigned bufferLength)
const
> +{
> +    ASSERT_UNUSED(bufferLength, bufferLength >=
bufferLengthForStringDecimal());
> +
> +    // Should always be at least one digit to add to the string!
> +    ASSERT(m_precision);
> +    char* next = buffer;

Is there a way to deal with this without copying and pasting an entire
function? In other cases we have used templates for this sort of thing.