Bug 77018 – implement Number.toLocaleString() using ICU

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Eli Fidler]

ICU provides methods to do locale-specific number to string conversion, which can make Number.toLocaleString actually useful.

[Eli Fidler]

Created attachment 123967 [details]
Patch

[Early Warning System Bot]

Comment on attachment 123967 [details]
Patch

Attachment 123967 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11294611

[Eli Fidler]

Created attachment 124001 [details]
Patch

[Darin Adler]

Comment on attachment 124001 [details]
Patch

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.

[Eli Fidler]

(In reply to comment #4)
> (From update of attachment 124001 [details])
> 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.

I've added some minimal test coverage, but we don't really have a way to do locale-specific testing.

> 
> > 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.
> 

Ok, I added currentNumericLocaleID(). I totally made up the OS(DARWIN) implementation, so hopefully it's ok. This code doesn't actually execute on Darwin, since the ICU version is too old to have unum_formatDecimal().

> 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.
> 

Added a cache.

> > 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.

Switched to a template function.

[Eli Fidler]

Created attachment 125181 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 125181 [details]
Patch

Attachment 125181 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11421006

[Eli Fidler]

Created attachment 125324 [details]
Patch

[Darin Adler]

Comment on attachment 125324 [details]
Patch

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.

[Darin Adler]

Comment on attachment 125324 [details]
Patch

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.

[Darin Adler]

Sorry about the double comment. Reason for that is described in bug 77750.

[Eli Fidler]

Sorry it took so long to update the patch.

(In reply to comment #10)
> (From update of attachment 125324 [details])
> 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.
> 

rebased

> > 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.
> 

done

> > 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.
>

I switched to a single macro in NumberPrototype.cpp
 
> > 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.

dropped the macros from that file

> > 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.
> 

This function isn't called on OSX/IOS since the ICU version is currently too old, so I replaced it with a #error. I don't really have enough experience with CF to be confident getting this correct.

> > 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.

I'm happier returning the actual locale since this function could be useful for something other than passing to unum_open()

[Eli Fidler]

Created attachment 126881 [details]
Patch

[Eli Fidler]

Created attachment 126987 [details]
Patch

[WebKit Review Bot]

Attachment 126987 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 126987 [details]
Patch

Attachment 126987 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11523286

[Gustavo Noronha (kov)]

Comment on attachment 126987 [details]
Patch

Attachment 126987 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11518338

[Eli Fidler]

Created attachment 127206 [details]
Patch

[WebKit Review Bot]

Attachment 127206 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/wtf/unicode/icu/UnicodeICU.cpp:33:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Wingo]

Regarding attachment 127206 [details]:
> +        * JavaScriptCore.exp:

Probably need to trim this from the ChangeLog.

(In reply to comment #4)
> (From update of attachment 124001 [details])
> 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.

Is there any possible test to make?  The spec is very vague.

I would expect that formatting "1" could even produce numbers in other scripts.

[Early Warning System Bot]

Comment on attachment 127206 [details]
Patch

Attachment 127206 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11834930

[Eli Fidler]

Created attachment 131447 [details]
Patch

[Early Warning System Bot]

Comment on attachment 131447 [details]
Patch

Attachment 131447 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11945160

[Gyuyoung Kim]

Comment on attachment 131447 [details]
Patch

Attachment 131447 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11948223

[Build Bot]

Comment on attachment 131447 [details]
Patch

Attachment 131447 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11947349

[Eli Fidler]

Created attachment 132114 [details]
Patch

[Early Warning System Bot]

Comment on attachment 132114 [details]
Patch

Attachment 132114 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11957746

[Build Bot]

Comment on attachment 132114 [details]
Patch

Attachment 132114 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11958738

[Gyuyoung Kim]

Comment on attachment 132114 [details]
Patch

Attachment 132114 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11957758

[Rob Buis]

Created attachment 135902 [details]
Patch

[Rob Buis]

Build against ToT and try to fix win build. Efl and Qt build will probably still fail, unless something changed on the build machines (different icu lib version for instance).

[Philippe Normand]

Comment on attachment 135902 [details]
Patch

Attachment 135902 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12336540

[Rob Buis]

Created attachment 135916 [details]
Patch

[Rob Buis]

Gtk fix got lost in the new patch. Also speculative fix for efl.

[Gyuyoung Kim]

Comment on attachment 135916 [details]
Patch

Attachment 135916 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12339572

[Early Warning System Bot]

Comment on attachment 135916 [details]
Patch

Attachment 135916 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12337525

[Rob Buis]

From irc, efl buildbot uses ubuntu 10.4, which uses icu 4.2, and unum_formatDecimal was added in icu 4.4. This explains the efl build problem with the patch.

[Eli Fidler]

The patch checks ICU_VERSION >= 40400. If they have ICU 4.2, none of this code should be compiled.

[Rob Buis]

(In reply to comment #38)
> The patch checks ICU_VERSION >= 40400. If they have ICU 4.2, none of this code should be compiled.

So the question may be, is ICU_VERSION picked up correctly...

[Raphael Kubo da Costa (:rakuco)]

The bot seems to be in a quite messy situation, with ICU 4.4 being installed into /usr/local. ICU 4.4 is being picked up by CMake, so I wonder why that error is happening.

I'll see what I can find out locally, will report back in a few hours.

[Raphael Kubo da Costa (:rakuco)]

In ICU 4.4.2 (which was installed into /usr/local and being correctly picked up by CMake), unum_formatDecimal was marked as draft API and, even though configure showed draft APIs were being built, the symbol was not found in the generated .so files.

I've decided to go nuclear and just updated ICU to v4.9.1.1, which is the latest one. Please resubmit the patch for us to see if it works now.

[Rob Buis]

Created attachment 136568 [details]
Retry to see if elf builds

[Early Warning System Bot]

Comment on attachment 136568 [details]
Retry to see if elf builds

Attachment 136568 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12386094

[Rob Buis]

Hi Raphael,

(In reply to comment #41)
> In ICU 4.4.2 (which was installed into /usr/local and being correctly picked up by CMake), unum_formatDecimal was marked as draft API and, even though configure showed draft APIs were being built, the symbol was not found in the generated .so files.
> 
> I've decided to go nuclear and just updated ICU to v4.9.1.1, which is the latest one. Please resubmit the patch for us to see if it works now.

That seems to have fixed the Efl build with this patch, thanks for your hard work!
Cheers,

Rob.

[Rob Buis]

Created attachment 136665 [details]
Checking Qt build again

[Early Warning System Bot]

Comment on attachment 136665 [details]
Checking Qt build again

Attachment 136665 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12386384

[Csaba Osztrogonác]

Comment on attachment 136665 [details]
Checking Qt build again

View in context: https://bugs.webkit.org/attachment.cgi?id=136665&action=review

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:122
> +        length = unum_formatDecimal(nf, numberString, length, buffer, WTF::NumberToStringBufferLength, 0, &status);

It seems unum_formatDecimal() is missing from our ICU. (4.4.1)

[Rob Buis]

Hi Ossy,

(In reply to comment #47)
> (From update of attachment 136665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136665&action=review
> 
> > Source/JavaScriptCore/runtime/NumberPrototype.cpp:122
> > +        length = unum_formatDecimal(nf, numberString, length, buffer, WTF::NumberToStringBufferLength, 0, &status);
> 
> It seems unum_formatDecimal() is missing from our ICU. (4.4.1)

Thanks for digging into this!
Eli, maybe the ICU_VERSION check needs to verify against something higher than 4.4.0? Rafael reported problems with 4.4.2 too....

[Raphael Kubo da Costa (:rakuco)]

Yeah, as I mentioned in comment #41, even though the API is marked as draft in the 4.4 series and ICU was built _with_ draft API functions, it didn't seem to be present in the generated .so files.

I guess checking for >= 4.6 (when the call was marked as stable API) is safer.

[Rob Buis]

Created attachment 136705 [details]
Require libicuuc >= 4.6

Trying with a new requirement of libicuuc >= 4.6.

[Rob Buis]

Thanks to the help of rakuco and Ossy, the patch now builds and links fine. Let the reviewing begin :)

[Darin Adler]

Comment on attachment 136705 [details]
Require libicuuc >= 4.6

View in context: https://bugs.webkit.org/attachment.cgi?id=136705&action=review

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:68
> +static UNumberFormat* cachedNumberFormatter = 0;

This global is used only inside the formatLocaleNumber function, so I suggest moving it in there.

Or, better, we could put it in per-thread globals so we don’t have to do all the locking.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:75
> +static JSCell* formatLocaleNumber(ExecState* exec, JSValue v)

Could we use a word, numberValue or value, instead of the letter “v” here please?

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:81
> +    UNumberFormat* nf = 0;

Could we use a word, formatter, instead of the letters “nf” here please?

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:83
> +        const char* cachedNumberFormatterLocale = unum_getLocaleByType(cachedNumberFormatter, ULOC_REQUESTED_LOCALE, &status);

This looks inefficient. The value of caching a formatter is lost if we are doing all this work every time.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:90
> +            // locale changed... we'll need a new formatter

WebKit comments are typically formatted as sentences.

    // Locale changed, so we'll need a new formatter.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:110
> +        int32_t num = v.asInt32();

Our coding style typically prefers a word, number, over the abbreviation “num” for a local variable like this one.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:117
> +        if (v.isDouble())
> +            num = v.asDouble();
> +        else
> +            num = v.toNumber(exec);

Why the special case for v.isDouble? That’s already what toNumber would do, so why add extra code.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:119
> +        DecimalNumber d(num);

Please use a word here rather than the letter “d” for the name of this local variable.

> Source/WTF/wtf/unicode/icu/UnicodeICU.cpp:36
> +#if OS(DARWIN) && USE(CF) && !PLATFORM(QT)
> +    // Mac OS X doesn't set UNIX locale to match user-selected one, so ICU default doesn't work.
> +    // FIXME: Need a way to get the numeric locale on Darwin
> +    CRASH();
> +#else

If the ifdef was around the function implementation then we’d get a link time failure instead of a runtime crash, which is much better. Please put the ifdef around the entire function, not just in the body.

[Carlos Garcia Campos]

Created attachment 193569 [details]
Rebased and updated patch

I've rebased the patch and updated to address review comments

[Carlos Garcia Campos]

(In reply to comment #52)
> (From update of attachment 136705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136705&action=review
> 
> > Source/JavaScriptCore/runtime/NumberPrototype.cpp:68
> > +static UNumberFormat* cachedNumberFormatter = 0;
> 
> This global is used only inside the formatLocaleNumber function, so I suggest moving it in there.
> 
> Or, better, we could put it in per-thread globals so we don’t have to do all the locking.

I've left the mutex for now, because I'm not sure how to make the formatter global per-thread. I guess I could use ThreadSpecific, but the formatter is not a C++ class (I think). I could make a small C++ class wrapping the formatter to use it with ThreadSpecific, though.

> > Source/JavaScriptCore/runtime/NumberPrototype.cpp:83
> > +        const char* cachedNumberFormatterLocale = unum_getLocaleByType(cachedNumberFormatter, ULOC_REQUESTED_LOCALE, &status);
> 
> This looks inefficient. The value of caching a formatter is lost if we are doing all this work every time.

I think it's very unlikely that the locale changes at run time, so I've removed the check.

[Build Bot]

Comment on attachment 193569 [details]
Rebased and updated patch

Attachment 193569 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17141339

[Carlos Garcia Campos]

Created attachment 193588 [details]
Updated patch

Fixed a typo in BlackBerry WTF cmake file and added also the new file to WTF gyp file. It will still fail for mac because I don't have a mac to update the XCode files.

[Build Bot]

Comment on attachment 193588 [details]
Updated patch

Attachment 193588 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17192532

[Build Bot]

Comment on attachment 193588 [details]
Updated patch

Attachment 193588 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17137439

[Carlos Garcia Campos]

Created attachment 197728 [details]
Updated patch to remove gyp changes

Still needs the xcode changes that I can't do.

[WebKit Commit Bot]

Attachment 197728 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/number-toLocaleString-expected.txt', u'LayoutTests/fast/js/number-toLocaleString.html', u'LayoutTests/fast/js/script-tests/number-toLocaleString.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/PlatformEfl.cmake', u'Source/JavaScriptCore/runtime/NumberPrototype.cpp', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/wtf/DecimalNumber.cpp', u'Source/WTF/wtf/DecimalNumber.h', u'Source/WTF/wtf/PlatformBlackBerry.cmake', u'Source/WTF/wtf/PlatformEfl.cmake', u'Source/WTF/wtf/unicode/icu/UnicodeICU.cpp', u'Source/WTF/wtf/unicode/icu/UnicodeIcu.h']" exit_code: 1
Source/WTF/wtf/unicode/icu/UnicodeICU.cpp:20:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 197728 [details]
Updated patch to remove gyp changes

View in context: https://bugs.webkit.org/attachment.cgi?id=197728&action=review

>> Source/WTF/wtf/unicode/icu/UnicodeICU.cpp:20
>> +#include "UnicodeIcu.h"
> 
> Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

I wonder why the style checker didn't complain locally. In any case, I think UnicodeIcu.h should be renamed to UnicodeICU.h but I don't think it's appropriate for this patch.

[Build Bot]

Comment on attachment 197728 [details]
Updated patch to remove gyp changes

Attachment 197728 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/103086

[Build Bot]

Comment on attachment 197728 [details]
Updated patch to remove gyp changes

Attachment 197728 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/128030

[Darin Adler]

Comment on attachment 197728 [details]
Updated patch to remove gyp changes

Patch does not build on Mac.

[Carlos Garcia Campos]

(In reply to comment #64)
> (From update of attachment 197728 [details])
> Patch does not build on Mac.

Yes, I said I need help with that, I don't have a mac, so I uploaded the patch anyway so that it can be reviewed (not necessarily r+'ed). Do you mean the only problem with the patch in this moment is that it needs to update the xcode files?

[Philippe Normand]

Created attachment 203420 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 203420 [details]
Patch

Thank you Phil!

[WebKit Commit Bot]

Attachment 203420 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/number-toLocaleString-expected.txt', u'LayoutTests/fast/js/number-toLocaleString.html', u'LayoutTests/fast/js/script-tests/number-toLocaleString.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/NumberPrototype.cpp', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.pro', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/CMakeLists.txt', u'Source/WTF/wtf/DecimalNumber.cpp', u'Source/WTF/wtf/DecimalNumber.h', u'Source/WTF/wtf/unicode/icu/UnicodeICU.cpp', u'Source/WTF/wtf/unicode/icu/UnicodeIcu.h']" exit_code: 1
Source/WTF/wtf/unicode/icu/UnicodeICU.cpp:21:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 203420 [details]
Patch

Attachment 203420 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/664236

[Build Bot]

Comment on attachment 203420 [details]
Patch

Attachment 203420 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/657225

[Liam Quinn]

Comment on attachment 203420 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203420&action=review

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:103
> +        bufferLength = decimalNumber.toStringDecimal(numberString, WTF::NumberToStringBufferLength);

DecimalNumber doesn't actually check the buffer length in release builds, and WTF::NumberToStringBufferLength assumes exponential notation that toStringDecimal() doesn't do. This would crash for large numbers and NaN.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:104
> +        bufferLength = unum_formatDecimal(formatter, numberString, bufferLength, buffer, WTF::NumberToStringBufferLength, 0, &status);

Is there a reason not to use unum_formatDouble instead of unum_formatDecimal? Then you wouldn't need the DecimalNumber.

[Alexey Proskuryakov]

*** Bug 138244 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

This patch title says "implement using ICU", but this is probably not how we want it implemented. On OS X in particular, the user can configure various aspects number formatting manually, so we can't just get the data from locale name.

[Andy VanWagoner]

Is this now a duplicate of 147610?

[Darin Adler]

Yes, a duplicate. And Alexey’s comment still applies.

*** This bug has been marked as a duplicate of bug 147610 ***