Message:
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?

Description:
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

Please review this at http://codereview.chromium.org/7129051/

SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/

Affected files:
   M     src/extensions/experimental/experimental.gyp
   M     src/extensions/experimental/i18n-extension.cc
   M     src/extensions/experimental/i18n.js
   A     src/extensions/experimental/number-format.h
   A     src/extensions/experimental/number-format.cc

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

http://codereview.chromium.org/7129051/diff/3001/src/extensions/exper...
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/exper...
File src/extensions/experimental/number-format.h (right):

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

http://codereview.chromium.org/7129051/

http://codereview.chromium.org/7129051/

10. јун 2011. 00.25, <a...@chromium.org> је написао/ла:

Don't rely on it unless there is a good reason, though. It's not
guaranteed to stay that way.
And if you use is_nan (which you must, because NaN != NaN, even if
it's the same NaN representation), it won't matter.

/L
--
Lasse R.H. Nielsen
l...@chromium.org

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/exper...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/3001/src/extensions/exper...
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:

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

http://codereview.chromium.org/7129051/

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

http://codereview.chromium.org/7129051/

http://codereview.chromium.org/7129051/

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

http://codereview.chromium.org/7129051/diff/11004/src/extensions/expe...
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/expe...
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/expe...
File src/extensions/experimental/number-format.cc (right):

http://codereview.chromium.org/7129051/diff/11004/src/extensions/expe...
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/expe...
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/expe...
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/expe...
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/expe...
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/expe...
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/expe...
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/expe...
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/expe...
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.

http://codereview.chromium.org/7129051/