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'])) { 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 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. 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/number-format.cc#newcode227 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/number-format.cc#newcode244 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/number-format.cc#newcode245 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/number-format.cc#newcode249 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/number-format.cc#newcode268 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/number-format.cc#newcode279 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/number-format.cc#newcode311 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/number-format.cc#newcode326 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/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
