[webkit-reviews] review denied: [Bug 77018] implement Number.toLocaleString() using ICU : [Attachment 125324] 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 125324: Patch
https://bugs.webkit.org/attachment.cgi?id=125324&action=review

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


This patch is not applying in EWS, probably because we have gotten rid of
JavaScriptCore.exp. I suggest you rebase the patch to adapt to that change.

> Source/JavaScriptCore/GNUmakefile.list.am:772
> +	Source/JavaScriptCore/wtf/unicode/icu/UnicodeIcu.cpp \

The file names should have ICU in all caps. I’m not sure why UnicodeIcu.h is
named wrong, but I highly suggest not repeating that mistake.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:44
> +#if USE(ICU_UNICODE) && !UCONFIG_NO_FORMATTING &&
defined(ICU_VERSION_AT_LEAST) && ICU_VERSION_AT_LEAST(4, 4, 0)

Rather than repeating this expression over and over again, I suggest we put a
named macro somewhere, perhaps in UnicodeIcu.h. USE_ICU_FOR_NUMBER_FORMATTING
is a possible name.

> Source/JavaScriptCore/wtf/DecimalNumber.cpp:160
> +template unsigned DecimalNumber::toStringDecimal(char* buffer, unsigned
bufferLength) const;
> +template unsigned DecimalNumber::toStringDecimal(UChar* buffer, unsigned
bufferLength) const;

I’m not sure all the compilers we use support this; I haven’t seen any other
explicit template instantiation. We need the EWS to work to answer that
question.

> Source/JavaScriptCore/wtf/DecimalNumber.h:88
> +    template <typename T>
> +    WTF_EXPORT_PRIVATE unsigned toStringDecimal(T* buffer, unsigned
bufferLength) const;

Not sure the export macro works properly in a case like that. We need the EWS
to tell us.

> Source/JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:103
> +#define ICU_VERSION (U_ICU_VERSION_MAJOR_NUM * 10000 +
U_ICU_VERSION_MINOR_NUM * 100 + U_ICU_VERSION_PATCHLEVEL_NUM)
> +#define ICU_VERSION_AT_LEAST(major, minor, patch) (ICU_VERSION >= (major *
10000 + minor * 100 + patch))

This doesn’t make sense to me. This header is used on many systems that do not
have ICU at all. Any use of this macro probably won’t compile. I think we need
a different approach.

> Source/JavaScriptCore/wtf/unicode/icu/UnicodeIcu.cpp:36
> +    return CFStringGetCStringPtr(localeID, CFStringGetSystemEncoding());

CFStringGetCStringPtr is not guaranteed to return a non-NULL value for a
string. It’s also not correct to use the system encoding in the C string.
Locale identifiers are probably in ASCII or UTF-8. It’s just random to let the
system encoding have any impact on this. There is working code to do this kind
of thing in the currentSearchLocaleID function in
TextBreakIteratorInternalICUMac.mm and you should make sure this code is more
like that. That code gets the conversion to a C string and caching and buffer
management right, and this code gets it wrong.

> Source/JavaScriptCore/wtf/unicode/icu/UnicodeIcu.cpp:38
> +    return uloc_getDefault();

Seems OK to return a 0 here as long as we document it, since 0 gives default
locale behavior in the ICU API.