http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/i18n.js
File src/extensions/experimental/i18n.js (right):
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/i18n.js#newcode267
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/i18n.js#newcode268
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/toLocaleUpperCase).
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/number-format.cc
File src/extensions/experimental/number-format.cc (right):
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode220
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/number-format.cc#newcode227
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/number-format.cc#newcode244
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/number-format.cc#newcode245
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/number-format.cc#newcode249
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/number-format.cc#newcode268
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/number-format.cc#newcode279
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/number-format.cc#newcode311
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/number-format.cc#newcode326
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.
http://codereview.chromium.org/7129051/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
