Change v8Locale to match proposal - constructor is different and I've added

Change v8Locale to match proposal - constructor is different (takes settings object, not a plain string) and I've added derive method to it.

Added comments to i18n.js methods and properties, and util functions to check settings and locale validity.

Added LanguageMatcher class until ICU gets C implementation (in progress, but late for our current deadline). I added TODO to remove LanguageMatcher code. 

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

[Mads Ager (chromium), 2011-05-02 07:54:40]

Bindings part LGTM

[jungshik at Google, 2011-05-02 19:57:23]

looks good overall, but need some minor fixes noted below.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
File src/extensions/experimental/i18n-locale.cc (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:89: if (region_id->IsString() &&
region_id->ToString()->Length() != 0) {
just checking: IsString() covers cases where kRegionID is not set in settings ?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:94: // Expand "de" into "de-Latn-DE"
and grab country info.
nit: This command seems to get out of blue. How about this?

Maximize locale_id to infer the region (e.g. expand "de" to "de-Latn-DE" and
grab "DE" from the result).

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:99: status = U_ZERO_ERROR;
YOu're not supposed to reset the error code here. Just let it fall through
(uloc_getCountry will return right away if it's called with U_FAILURE(status)
being true) and at the end of this function, return a boolean value  indicating
whether the function succeeded or not (!!U_SUCCESS(status) or
!U_FAILURE(status)). Then, a caller can decides what to do about it.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:33: *     Unicode identifier of the locale.
nit: a link to the section of UTR 35 defining Unicode locale ID (based on BCP47)
?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:49: // Keep ICU locale around so we don't
have to convert later on.
nit: Keep the resolved ICU locale ID around to avoid resolving localeID to ICU
locale ID every time BreakIterator, Collator and so forth are called.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:81: // This should be
locale.options.localeID, but ICU would require conversion.
In this particular case (BreakIterator), just passing BCP47/Unicode locale ID
works fine, but there's no harm done to use an ICU locale ID, instead.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:112: * Collator class implements locale
aware sort.
nit: locale-aware or locale-sensitive

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:140: * valid takes default values and
constructs new settings object.
nit: valid takes ==> valid, takes.

BTW, the description does not seem to match what the function actually does. 

|defaults| and |settings| are merged, aren't they?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
File src/extensions/experimental/language-matcher.cc (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:40: const unsigned int
LanguageMatcher::kLanguageDistance = 75;
nit: again, this is not a distance but a score/weight or inverse-distance.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:120:
icu::Locale::getAvailableLocales(available_count);
Don't we need any lock around here?

More importantly, what's returned by getAvailableLocale() can be deceptive (at
least in Chrome) because the return value will include 70+ locales for which ICU
data (in Chrome) has only nominal data (the native name, the script and writing
direction of the primary language of a locale) and nothing else. 

See
http://www.google.com/codesearch/p?hl=en#hfE6470xZHk/app/l10n_util.cc&q=IsLoc...
  for a hackish way of detecting this. 

An alternative is to call getAvailableLocale for DateTimeFormatter and use it
instead. (
http://icu-project.org/apiref/icu4c/classDateFormat.html#a5934ceffff9eafdd3ed...
).

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:124: icu::Locale
user_locale(locale);
how about input_locale?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:166: if (!strcmp(language,
"tl")) return "fil";
Comment these entries? e.g. Tagalog => Filipino

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:175:
uloc_forLanguageTag(*v8::String::Utf8Value(locale_id), locale,
ASCIIValue?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:180: // Returns 1 for match or-1
for mismatch.
nit: "or-1" -> "or -1"

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:188: //   base_name: de_DE
base_name of the matched locale (ICU ID): de_DE

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:189: //   user_locale_name:
de_AT@collation=phonebk
input_locale_name (ICU ID)

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:190: //   ICU compatible result:
de_DE@collation=phonebk
result (ICU ID)

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:191: //   BCP47 compatible
result: de-DE-u-co-phonebk
result (BCP 47)

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:207: ULOC_FULLNAME_CAPACITY,
false, &status);
Instead of ignoring status, either assert U_SUCCESS() or return the result in a
boolean value so that a caller can do something about it.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
File src/extensions/experimental/language-matcher.h (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.h:44: char
bcp47_locale[ULOC_FULLNAME_CAPACITY];
optional: bcp47_id and icu_id seem better.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.h:49: // Rank for this locale when
compared to others.
Isn't this score?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.h:70: static const unsigned int
kLanguageDistance;
I think what you meant is 'Weight' rather than 'Distance' and same with other
"FooDistance'.

[jungshik at Google, 2011-05-02 20:00:14]

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
File src/extensions/experimental/i18n-locale.cc (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:61: } else if (locale_id->IsString())
{
perhaps, ASCII-ness has to be checked for input.

[Nebojša Ćirić, 2011-05-02 22:44:01]

All done. Also snprintf didn't look like it would work cross platform so I used
OS::SNPrintF instead.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
File src/extensions/experimental/i18n-locale.cc (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:61: } else if (locale_id->IsString())
{
I made the change in language-matcher.cc:CompareToSupportedLocaleIDList

On 2011/05/02 20:00:15, Jungshik Shin wrote:
> perhaps, ASCII-ness has to be checked for input.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:89: if (region_id->IsString() &&
region_id->ToString()->Length() != 0) {
My test behaves as expected for this case, and v8 source returns heap->undefined
if it can't find the property, which is expected.
In short, I think it's ok to keep it like this.

On 2011/05/02 19:57:23, Jungshik Shin wrote:
> just checking: IsString() covers cases where kRegionID is not set in settings
?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:94: // Expand "de" into "de-Latn-DE"
and grab country info.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: This command seems to get out of blue. How about this?
> 
> Maximize locale_id to infer the region (e.g. expand "de" to "de-Latn-DE" and
> grab "DE" from the result). 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n-locale.cc:99: status = U_ZERO_ERROR;
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> YOu're not supposed to reset the error code here. Just let it fall through
> (uloc_getCountry will return right away if it's called with U_FAILURE(status)
> being true) and at the end of this function, return a boolean value 
indicating
> whether the function succeeded or not (!!U_SUCCESS(status) or
> !U_FAILURE(status)). Then, a caller can decides what to do about it. 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
File src/extensions/experimental/i18n.js (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:33: *     Unicode identifier of the locale.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: a link to the section of UTR 35 defining Unicode locale ID (based on
BCP47)
> ? 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:49: // Keep ICU locale around so we don't
have to convert later on.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: Keep the resolved ICU locale ID around to avoid resolving localeID to ICU
> locale ID every time BreakIterator, Collator and so forth are called. 
> 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:81: // This should be
locale.options.localeID, but ICU would require conversion.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> In this particular case (BreakIterator), just passing BCP47/Unicode locale ID
> works fine, but there's no harm done to use an ICU locale ID, instead.

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:112: * Collator class implements locale
aware sort.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: locale-aware or locale-sensitive

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/i1...
src/extensions/experimental/i18n.js:140: * valid takes default values and
constructs new settings object.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: valid takes ==> valid, takes.
> 
> BTW, the description does not seem to match what the function actually does. 
> 
> |defaults| and |settings| are merged, aren't they? 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
File src/extensions/experimental/language-matcher.cc (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:40: const unsigned int
LanguageMatcher::kLanguageDistance = 75;
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: again, this is not a distance but a score/weight or inverse-distance. 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:120:
icu::Locale::getAvailableLocales(available_count);
Added a TODO, but I don't think it's an issue. One of the calls would win, and
they all return the same information, so there is no difference which one wins.

As for DateFormat, I agree. Done.

On 2011/05/02 19:57:23, Jungshik Shin wrote:
> Don't we need any lock around here?
> 
> More importantly, what's returned by getAvailableLocale() can be deceptive (at
> least in Chrome) because the return value will include 70+ locales for which
ICU
> data (in Chrome) has only nominal data (the native name, the script and
writing
> direction of the primary language of a locale) and nothing else. 
> 
> See
>
http://www.google.com/codesearch/p?hl=en#hfE6470xZHk/app/l10n_util.cc&q=IsLoc...
>   for a hackish way of detecting this. 
> 
> An alternative is to call getAvailableLocale for DateTimeFormatter and use it
> instead. (
>
http://icu-project.org/apiref/icu4c/classDateFormat.html#a5934ceffff9eafdd3ed...
> ). 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:124: icu::Locale
user_locale(locale);
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> how about input_locale? 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:166: if (!strcmp(language,
"tl")) return "fil";
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> Comment these entries? e.g. Tagalog => Filipino

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:175:
uloc_forLanguageTag(*v8::String::Utf8Value(locale_id), locale,
Already done in parent method.

On 2011/05/02 19:57:23, Jungshik Shin wrote:
> ASCIIValue?

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:180: // Returns 1 for match or-1
for mismatch.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> nit: "or-1" -> "or -1"

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:188: //   base_name: de_DE
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> base_name of the matched locale (ICU ID): de_DE

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:189: //   user_locale_name:
de_AT@collation=phonebk
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> input_locale_name (ICU ID)

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:190: //   ICU compatible result:
de_DE@collation=phonebk
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> result (ICU ID)

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:191: //   BCP47 compatible
result: de-DE-u-co-phonebk
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> result (BCP 47)

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.cc:207: ULOC_FULLNAME_CAPACITY,
false, &status);
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> Instead of ignoring status, either assert U_SUCCESS() or return the result in
a
> boolean value so that a caller can do something about it.

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
File src/extensions/experimental/language-matcher.h (right):

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.h:44: char
bcp47_locale[ULOC_FULLNAME_CAPACITY];
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> optional: bcp47_id and icu_id seem better. 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.h:49: // Rank for this locale when
compared to others.
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> Isn't this score? 

Done.

http://codereview.chromium.org/6901141/diff/11/src/extensions/experimental/la...
src/extensions/experimental/language-matcher.h:70: static const unsigned int
kLanguageDistance;
On 2011/05/02 19:57:23, Jungshik Shin wrote:
> I think what you meant is 'Weight' rather than 'Distance' and same with other
> "FooDistance'. 

Done.

[jungshik at Google, 2011-05-03 20:04:04]

LGTM with minor changes below.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
File src/extensions/experimental/i18n-locale.cc (right):

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/i18n-locale.cc:77:
GetBestMatchForRegionID(result.icu_id, region, region_id);
shouldn't you act on the return value here? If failed, perhaps return Undefined
or leave region_id empty?

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/i18n-locale.cc:94: "%s", user_locale.getCountry());
nit: 'return true' here and get rid of "else" below?

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
File src/extensions/experimental/language-matcher.cc (right):

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:94:
CompareToSupportedLocaleIDList(locale_id->ToString(), &match);
Perhaps, you can check the return value and skip if it's a failure.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:116: }
How about this? 

if (CompareToSupportedLocaleIDList(...) && match.score >= kThresold)  ...

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:124: // with little data.
DateFormat variant returns well populated Locales only.
The above comment is specific to Chrome's ICU build.  Perhaps, this is better:

Depending on how ICU data is built, locales returned by
Locale::getAvailableLocale() are not guaranteed to support  DateFormat,
Collation and other services.  We can call getAvailableLocale() of all the
services we want to support and take the intersection of them all, but using
DateFormat::getAvailableLocales() can do in practice.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:171: input_locale.getName(),
result);
If we just want to throw away the return value, there's no point of returning a
boolean, no?  
Perhaps, you can change the above to 

return BuildLocaleName(.....); 

Then, a caller can decide what to do. see above.

[jungshik at Google, 2011-05-03 20:05:08]

LGTM with minor changes below.

[Nebojša Ćirić, 2011-05-03 20:38:59]

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
File src/extensions/experimental/i18n-locale.cc (right):

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/i18n-locale.cc:77:
GetBestMatchForRegionID(result.icu_id, region, region_id);
I've assigned empty string to region id in case of failure.

On 2011/05/03 20:04:05, Jungshik Shin wrote:
> shouldn't you act on the return value here? If failed, perhaps return
Undefined
> or leave region_id empty?

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/i18n-locale.cc:94: "%s", user_locale.getCountry());
On 2011/05/03 20:04:05, Jungshik Shin wrote:
> nit: 'return true' here and get rid of "else" below? 

Done.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
File src/extensions/experimental/language-matcher.cc (right):

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:94:
CompareToSupportedLocaleIDList(locale_id->ToString(), &match);
On 2011/05/03 20:04:05, Jungshik Shin wrote:
> Perhaps, you can check the return value and skip if it's a failure. 

Done.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:116: }
On 2011/05/03 20:04:05, Jungshik Shin wrote:
> How about this? 
> 
> if (CompareToSupportedLocaleIDList(...) && match.score >= kThresold)  ... 
> 

Done.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:124: // with little data.
DateFormat variant returns well populated Locales only.
On 2011/05/03 20:04:05, Jungshik Shin wrote:
> The above comment is specific to Chrome's ICU build.  Perhaps, this is better:
> 
> Depending on how ICU data is built, locales returned by
> Locale::getAvailableLocale() are not guaranteed to support  DateFormat,
> Collation and other services.  We can call getAvailableLocale() of all the
> services we want to support and take the intersection of them all, but using
> DateFormat::getAvailableLocales() can do in practice. 

Done.

http://codereview.chromium.org/6901141/diff/11001/src/extensions/experimental...
src/extensions/experimental/language-matcher.cc:171: input_locale.getName(),
result);
On 2011/05/03 20:04:05, Jungshik Shin wrote:
> If we just want to throw away the return value, there's no point of returning
a
> boolean, no?  
> Perhaps, you can change the above to 
> 
> return BuildLocaleName(.....); 
> 
> Then, a caller can decide what to do. see above.

Done.