Adding support for number formating to the JS i18n API.

Adding support for number formating to the JS i18n API.

This is the last part of the API that belongs in public spec.

Methods supported:
- format
- derive

Options supported:
- style (decimal, scientific, currency and percent)
- pattern
- skeleton

TEST= Visit i18n.kaziprst.org/numberformat.html
Committed: http://code.google.com/p/v8/source/detail?r=8379

[Nebojša Ćirić, 2011-06-09 22:40:11]

Jungshink: please take a look at ICU calls and overall logic.

Mads:
I can't use internal methods so I had to make some interesting choices on how to
detect NaN in C++ code.
Could you take a look at JSNumberFormat method where I eval('NaN') to get actual
value and see if it makes sense.
From some forum posts it seems v8 uses 2 values for NaN (-NaN and NaN). True?

[Mads Ager (chromium), 2011-06-10 07:25:30]

This version of NaN handling is not going to work since NaN is not equal to
anything. The rest of the V8 usage looks fine.

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
src/extensions/experimental/number-format.cc:90: if (value == nan_) {
This does not work. NaN is not equal to anything so this should always be false.
Have you tested this?

You do need a is_nan method here. Most platforms provide one directly and
windows has is_nan_. Can you introduce a i18n-extension-utils.h file that does
the right platform #ifdef magic to make a is_nan method available on all
platforms?

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
File src/extensions/experimental/number-format.h (right):

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
src/extensions/experimental/number-format.h:58: void* param);
Indentation is a bit off.

[Mads Ager (chromium), 2011-06-10 07:29:46]

Doesn't ICU deal with NaN on its own? I would have guessed that ICU would give
you either "NaN" or some special value signaling that it was asked to format a
NaN value? If it does that will simplify the problem because you let ICU deal
with it and just switch on the result from the ICU formatting.

[Nebojša Ćirić, 2011-06-14 21:46:21]

Sorry, I was away Fri-Mon.

Not ready for another review.

I'll add currencyCode (to match update in the docs) to the settings and then ask
for another round.

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
src/extensions/experimental/number-format.cc:90: if (value == nan_) {
ICU can handle NaN properly so I've removed hacky code.

On 2011/06/10 07:25:30, Mads Ager wrote:
> This does not work. NaN is not equal to anything so this should always be
false.
> Have you tested this?
> 
> You do need a is_nan method here. Most platforms provide one directly and
> windows has is_nan_. Can you introduce a i18n-extension-utils.h file that does
> the right platform #ifdef magic to make a is_nan method available on all
> platforms?

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
File src/extensions/experimental/number-format.h (right):

http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/...
src/extensions/experimental/number-format.h:58: void* param);
On 2011/06/10 07:25:30, Mads Ager wrote:
> Indentation is a bit off.

Done.

[Nebojša Ćirić, 2011-06-15 22:19:00]

PTAL

I did one more pass over the code to fix couple of skeleton issues.

I also added currencyCode setting in i18n.js. If present it will override region
ID inferred currency code (in native code).

[Mads Ager (chromium), 2011-06-16 09:55:07]

The V8 API parts LGTM

[jungshik at Google, 2011-06-16 18:08:19]

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/i18n.js:267:
/^[a-zA-Z]{3}$/.test(settings['currencyCode'])) {
Is it your intent to allow mixed-case 3-letter code?

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/i18n.js:268: cleanSettings['currencyCode'] =
settings['currencyCode'].toUpperCase();
It looks like it's assumed that v8's toUpperCase will remain always
locale-independent (i.e. acting like POSIX locale : [a-z] => [A-Z]).  At least,
add a comment about that.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:220: // It affects currency
formatters only.
Pls, add a comment that |region| here is actually a full-blown locale id in the
form of 'und_' + region ID. An alternative is: 1) do not prepend 'und_' on the
JavaScript-side 2) add 'und_' at the last moment inside GetCurrencyCode().

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:227: }
What if the locale id (icu_locale) itself has a currency code specified (with
'-u-cu-FOO')? We have to determine the priority order among three possible
sources.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:244: // UChar representation of
\x00A4 currency symbol.
nit: \x00A4 => U+00A4

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:245: const UChar currency_symbol =
0xA4;
nit: Didn't any complier complain?  Perhaps, it's safer to add 'u'.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:249: // Find how many 0xA4 are
there. There is at least one.
nit: U+00A4

BTW, the code below assumes that a skeleton has one or more U+00A4's in a row.
What if there is an intervening character between them?  Shouldn't we use a
regex matcher (to match 1 to 3 (or 4) U+00A4's in a row? Actually, using regex
seems to be an overkill. Perhaps, after finding |end_index}, you can walk from
index to end_index to make sure there' no other character in-between.  If there
is, you can reject the skeleton as invalid.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:268:
icu::NumberFormat::createPercentInstance(icu_locale, *status));
no support for scientific with a skeleton?

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:279: // Copy important information
from skeleton to the new formatter.
I wonder if there's anything else to pass from skeleton.  e.g. roundingIncrement
(http://goo.gl/RU4qG )? 

roundingMode cannot be set via a pattern. So, we can't support in skeleton, but
roundingIncrement can be set via a pattern....

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:311: // or provided region id.
Here again, it's confusing to say 'region id' because it's more than that.
('und_' + region ID).

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:326: const char* region =
icu_locale.getName();
wrapping up 'und_' + regionID in Locale class only to get back the raw string
seems a bit roo round-about. I'd just pass around 'region ID' alone. OTOH,
having to prepend 'und_' with char array is not fun...  So, it's up to you.

Anyway, GetCurrencyCode() has to be revised to deal with '-u-cu-FOO' case in
icu_locale.

[Nebojša Ćirić, 2011-06-17 17:48:45]

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/i18n.js:267:
/^[a-zA-Z]{3}$/.test(settings['currencyCode'])) {
I don't think it's a problem to allow that in this case. User can do EUR or eur,
or less likely EUr.

On 2011/06/16 18:08:19, Jungshik Shin wrote:
> Is it your intent to allow mixed-case 3-letter code?

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/i18n.js:268: cleanSettings['currencyCode'] =
settings['currencyCode'].toUpperCase();
It's locale independent. There is a locale depended version - toLocaleUpperCase
(https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/t...).

On 2011/06/16 18:08:19, Jungshik Shin wrote:
> It looks like it's assumed that v8's toUpperCase will remain always
> locale-independent (i.e. acting like POSIX locale : [a-z] => [A-Z]).  At
least,
> add a comment about that.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:220: // It affects currency
formatters only.
On 2011/06/16 18:08:19, Jungshik Shin wrote:
> Pls, add a comment that |region| here is actually a full-blown locale id in
the
> form of 'und_' + region ID. An alternative is: 1) do not prepend 'und_' on the
> JavaScript-side 2) add 'und_' at the last moment inside GetCurrencyCode(). 

Done.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:227: }
I made priority:

- specified ISO code in currencyCode
- specified -u-cu- in locale
- infer from region ID

On 2011/06/16 18:08:19, Jungshik Shin wrote:
> What if the locale id (icu_locale) itself has a currency code specified (with
> '-u-cu-FOO')? We have to determine the priority order among three possible
> sources.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:244: // UChar representation of
\x00A4 currency symbol.
On 2011/06/16 18:08:19, Jungshik Shin wrote:
> nit: \x00A4 => U+00A4

Done.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:245: const UChar currency_symbol =
0xA4;
On 2011/06/16 18:08:19, Jungshik Shin wrote:
> nit: Didn't any complier complain?  Perhaps, it's safer to add 'u'. 

Done.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:249: // Find how many 0xA4 are
there. There is at least one.
I did this as a regexp in JS code (i18n.js), \u00a4+[^\u00a4]+\u00a4+. If that
matches I convert it to simple \u00A4.

As for more than 3 U+00A4 in a row, C++ code handles it as having 3 in default:
case in switch.

We didn't flesh out the actual error handling. Throwing an exception here would
be too much, but user can examine the skeleton and see what happened.

On 2011/06/16 18:08:19, Jungshik Shin wrote:
> nit: U+00A4
> 
> BTW, the code below assumes that a skeleton has one or more U+00A4's in a row.
> What if there is an intervening character between them?  Shouldn't we use a
> regex matcher (to match 1 to 3 (or 4) U+00A4's in a row? Actually, using regex
> seems to be an overkill. Perhaps, after finding |end_index}, you can walk from
> index to end_index to make sure there' no other character in-between.  If
there
> is, you can reject the skeleton as invalid.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:268:
icu::NumberFormat::createPercentInstance(icu_locale, *status));
Not for now. I want to see what Mark has to say about the design doc. I can add
it later on. I'll add todo.

On 2011/06/16 18:08:19, Jungshik Shin wrote:
> no support for scientific with a skeleton?

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:279: // Copy important information
from skeleton to the new formatter.
Added TODO.

On 2011/06/16 18:08:19, Jungshik Shin wrote:
> I wonder if there's anything else to pass from skeleton.  e.g.
roundingIncrement
> (http://goo.gl/RU4qG )? 
> 
> roundingMode cannot be set via a pattern. So, we can't support in skeleton,
but
> roundingIncrement can be set via a pattern....

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:311: // or provided region id.
On 2011/06/16 18:08:19, Jungshik Shin wrote:
> Here again, it's confusing to say 'region id' because it's more than that.
> ('und_' + region ID). 

Done.

http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental...
src/extensions/experimental/number-format.cc:326: const char* region =
icu_locale.getName();
On 2011/06/16 18:08:19, Jungshik Shin wrote:
> wrapping up 'und_' + regionID in Locale class only to get back the raw string
> seems a bit roo round-about. I'd just pass around 'region ID' alone. OTOH,
> having to prepend 'und_' with char array is not fun...  So, it's up to you.
> 
> Anyway, GetCurrencyCode() has to be revised to deal with '-u-cu-FOO' case in
> icu_locale. 

Done.

[jungshik at Google, 2011-06-21 23:28:41]

LGTM with nits below taken care of.

http://codereview.chromium.org/7129051/diff/19003/src/extensions/experimental...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/19003/src/extensions/experimental...
src/extensions/experimental/number-format.cc:250: // Find how many U+00A4 are
there. There is at least one.
nit: Add a comment that non-consecutive U+00A4's have been taken care of in
i18n.js

http://codereview.chromium.org/7129051/diff/19003/src/extensions/experimental...
src/extensions/experimental/number-format.cc:347: return true;
nit: You can combine the above 4 lines to "return !!U_SUCCESS(status);"

[Nebojša Ćirić, 2011-06-22 16:50:41]

http://codereview.chromium.org/7129051/diff/19003/src/extensions/experimental...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/19003/src/extensions/experimental...
src/extensions/experimental/number-format.cc:250: // Find how many U+00A4 are
there. There is at least one.
On 2011/06/21 23:28:41, Jungshik Shin wrote:
> nit: Add a comment that non-consecutive U+00A4's have been taken care of in
> i18n.js

Done.

http://codereview.chromium.org/7129051/diff/19003/src/extensions/experimental...
src/extensions/experimental/number-format.cc:347: return true;
On 2011/06/21 23:28:41, Jungshik Shin wrote:
> nit: You can combine the above 4 lines to "return !!U_SUCCESS(status);" 

Done.