Bug 77018 - implement Number.toLocaleString() using ICU
Summary: implement Number.toLocaleString() using ICU
Status: RESOLVED DUPLICATE of bug 147610
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eli Fidler
URL:
Keywords:
: 138244 (view as bug list)
Depends on:
Blocks: 111729
  Show dependency treegraph
 
Reported: 2012-01-25 09:17 PST by Eli Fidler
Modified: 2015-12-13 16:35 PST (History)
22 users (show)

See Also:


Attachments
Patch (7.79 KB, patch)
2012-01-25 10:08 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (8.47 KB, patch)
2012-01-25 13:42 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (18.67 KB, patch)
2012-02-02 13:43 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (18.72 KB, patch)
2012-02-03 07:38 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (17.51 KB, patch)
2012-02-13 18:20 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (17.41 KB, patch)
2012-02-14 09:18 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (17.38 KB, patch)
2012-02-15 11:39 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (17.93 KB, patch)
2012-03-12 16:34 PDT, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2012-03-15 13:52 PDT, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (14.85 KB, patch)
2012-04-05 14:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (15.79 KB, patch)
2012-04-05 15:07 PDT, Rob Buis
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Retry to see if elf builds (15.79 KB, patch)
2012-04-10 16:23 PDT, Rob Buis
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Checking Qt build again (15.79 KB, patch)
2012-04-11 06:59 PDT, Rob Buis
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Require libicuuc >= 4.6 (15.79 KB, patch)
2012-04-11 11:01 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Rebased and updated patch (14.76 KB, patch)
2013-03-18 08:39 PDT, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (15.25 KB, patch)
2013-03-18 09:57 PDT, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Updated patch to remove gyp changes (14.78 KB, patch)
2013-04-12 01:01 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (18.17 KB, patch)
2013-05-31 01:33 PDT, Philippe Normand
lquinn: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eli Fidler 2012-01-25 09:17:30 PST
ICU provides methods to do locale-specific number to string conversion, which can make Number.toLocaleString actually useful.
Comment 1 Eli Fidler 2012-01-25 10:08:43 PST
Created attachment 123967 [details]
Patch
Comment 2 Early Warning System Bot 2012-01-25 10:22:24 PST
Comment on attachment 123967 [details]
Patch

Attachment 123967 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11294611
Comment 3 Eli Fidler 2012-01-25 13:42:03 PST
Created attachment 124001 [details]
Patch
Comment 4 Darin Adler 2012-01-26 10:24:58 PST
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.
Comment 5 Eli Fidler 2012-02-02 13:42:28 PST
(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.
Comment 6 Eli Fidler 2012-02-02 13:43:52 PST
Created attachment 125181 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2012-02-02 17:37:25 PST
Comment on attachment 125181 [details]
Patch

Attachment 125181 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11421006
Comment 8 Eli Fidler 2012-02-03 07:38:39 PST
Created attachment 125324 [details]
Patch
Comment 9 Darin Adler 2012-02-03 10:36:05 PST
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.
Comment 10 Darin Adler 2012-02-03 10:36:11 PST
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.
Comment 11 Darin Adler 2012-02-03 10:38:04 PST
Sorry about the double comment. Reason for that is described in bug 77750.
Comment 12 Eli Fidler 2012-02-13 18:20:34 PST
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()
Comment 13 Eli Fidler 2012-02-13 18:20:58 PST
Created attachment 126881 [details]
Patch
Comment 14 Eli Fidler 2012-02-14 09:18:23 PST
Created attachment 126987 [details]
Patch
Comment 15 WebKit Review Bot 2012-02-14 09:20:58 PST
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.
Comment 16 Early Warning System Bot 2012-02-14 09:36:36 PST
Comment on attachment 126987 [details]
Patch

Attachment 126987 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11523286
Comment 17 Gustavo Noronha (kov) 2012-02-14 10:14:48 PST
Comment on attachment 126987 [details]
Patch

Attachment 126987 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11518338
Comment 18 Eli Fidler 2012-02-15 11:39:57 PST
Created attachment 127206 [details]
Patch
Comment 19 WebKit Review Bot 2012-02-15 22:48:50 PST
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.
Comment 20 Andy Wingo 2012-03-06 06:34:45 PST
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.
Comment 21 Early Warning System Bot 2012-03-07 02:52:45 PST
Comment on attachment 127206 [details]
Patch

Attachment 127206 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11834930
Comment 22 Eli Fidler 2012-03-12 16:34:01 PDT
Created attachment 131447 [details]
Patch
Comment 23 Early Warning System Bot 2012-03-12 17:01:08 PDT
Comment on attachment 131447 [details]
Patch

Attachment 131447 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11945160
Comment 24 Gyuyoung Kim 2012-03-12 19:19:20 PDT
Comment on attachment 131447 [details]
Patch

Attachment 131447 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11948223
Comment 25 Build Bot 2012-03-12 23:08:32 PDT
Comment on attachment 131447 [details]
Patch

Attachment 131447 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11947349
Comment 26 Eli Fidler 2012-03-15 13:52:16 PDT
Created attachment 132114 [details]
Patch
Comment 27 Early Warning System Bot 2012-03-15 14:07:08 PDT
Comment on attachment 132114 [details]
Patch

Attachment 132114 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11957746
Comment 28 Build Bot 2012-03-15 14:13:42 PDT
Comment on attachment 132114 [details]
Patch

Attachment 132114 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11958738
Comment 29 Gyuyoung Kim 2012-03-15 14:45:04 PDT
Comment on attachment 132114 [details]
Patch

Attachment 132114 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11957758
Comment 30 Rob Buis 2012-04-05 14:12:49 PDT
Created attachment 135902 [details]
Patch
Comment 31 Rob Buis 2012-04-05 14:15:13 PDT
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).
Comment 32 Philippe Normand 2012-04-05 14:52:41 PDT
Comment on attachment 135902 [details]
Patch

Attachment 135902 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12336540
Comment 33 Rob Buis 2012-04-05 15:07:57 PDT
Created attachment 135916 [details]
Patch
Comment 34 Rob Buis 2012-04-05 15:10:16 PDT
Gtk fix got lost in the new patch. Also speculative fix for efl.
Comment 35 Gyuyoung Kim 2012-04-05 16:49:35 PDT
Comment on attachment 135916 [details]
Patch

Attachment 135916 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12339572
Comment 36 Early Warning System Bot 2012-04-05 16:59:53 PDT
Comment on attachment 135916 [details]
Patch

Attachment 135916 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12337525
Comment 37 Rob Buis 2012-04-10 10:19:07 PDT
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.
Comment 38 Eli Fidler 2012-04-10 13:01:44 PDT
The patch checks ICU_VERSION >= 40400. If they have ICU 4.2, none of this code should be compiled.
Comment 39 Rob Buis 2012-04-10 13:07:18 PDT
(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...
Comment 40 Raphael Kubo da Costa (:rakuco) 2012-04-10 13:30:29 PDT
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.
Comment 41 Raphael Kubo da Costa (:rakuco) 2012-04-10 16:21:10 PDT
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.
Comment 42 Rob Buis 2012-04-10 16:23:27 PDT
Created attachment 136568 [details]
Retry to see if elf builds
Comment 43 Early Warning System Bot 2012-04-10 18:22:04 PDT
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
Comment 44 Rob Buis 2012-04-11 06:34:42 PDT
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.
Comment 45 Rob Buis 2012-04-11 06:59:26 PDT
Created attachment 136665 [details]
Checking Qt build again
Comment 46 Early Warning System Bot 2012-04-11 07:16:50 PDT
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
Comment 47 Csaba Osztrogonác 2012-04-11 08:07:31 PDT
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)
Comment 48 Rob Buis 2012-04-11 08:26:05 PDT
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....
Comment 49 Raphael Kubo da Costa (:rakuco) 2012-04-11 09:13:45 PDT
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.
Comment 50 Rob Buis 2012-04-11 11:01:03 PDT
Created attachment 136705 [details]
Require libicuuc >= 4.6

Trying with a new requirement of libicuuc >= 4.6.
Comment 51 Rob Buis 2012-04-11 13:11:39 PDT
Thanks to the help of rakuco and Ossy, the patch now builds and links fine. Let the reviewing begin :)
Comment 52 Darin Adler 2012-04-23 15:30:04 PDT
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.
Comment 53 Carlos Garcia Campos 2013-03-18 08:39:48 PDT
Created attachment 193569 [details]
Rebased and updated patch

I've rebased the patch and updated to address review comments
Comment 54 Carlos Garcia Campos 2013-03-18 08:42:35 PDT
(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.
Comment 55 Build Bot 2013-03-18 09:33:13 PDT
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
Comment 56 Carlos Garcia Campos 2013-03-18 09:57:26 PDT
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.
Comment 57 Build Bot 2013-03-18 12:36:28 PDT
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
Comment 58 Build Bot 2013-03-18 13:06:02 PDT
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
Comment 59 Carlos Garcia Campos 2013-04-12 01:01:19 PDT
Created attachment 197728 [details]
Updated patch to remove gyp changes

Still needs the xcode changes that I can't do.
Comment 60 WebKit Commit Bot 2013-04-12 01:08:14 PDT
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.
Comment 61 Carlos Garcia Campos 2013-04-12 01:20:26 PDT
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.
Comment 62 Build Bot 2013-04-12 01:39:18 PDT
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
Comment 63 Build Bot 2013-04-12 01:48:33 PDT
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
Comment 64 Darin Adler 2013-05-09 10:00:25 PDT
Comment on attachment 197728 [details]
Updated patch to remove gyp changes

Patch does not build on Mac.
Comment 65 Carlos Garcia Campos 2013-05-09 10:04:39 PDT
(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?
Comment 66 Philippe Normand 2013-05-31 01:33:44 PDT
Created attachment 203420 [details]
Patch
Comment 67 Carlos Garcia Campos 2013-05-31 01:51:27 PDT
Comment on attachment 203420 [details]
Patch

Thank you Phil!
Comment 68 WebKit Commit Bot 2013-05-31 01:53:05 PDT
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.
Comment 69 Build Bot 2013-05-31 02:12:15 PDT
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
Comment 70 Build Bot 2013-05-31 02:33:51 PDT
Comment on attachment 203420 [details]
Patch

Attachment 203420 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/657225
Comment 71 Liam Quinn 2014-05-02 15:18:10 PDT
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.
Comment 72 Alexey Proskuryakov 2014-11-02 21:54:59 PST
*** Bug 138244 has been marked as a duplicate of this bug. ***
Comment 73 Alexey Proskuryakov 2014-11-02 22:01:29 PST
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.
Comment 74 Andy VanWagoner 2015-12-13 15:43:29 PST
Is this now a duplicate of 147610?
Comment 75 Darin Adler 2015-12-13 16:35:48 PST
Yes, a duplicate. And Alexey’s comment still applies.

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