Adding experimental JavaScript internationalization API to V8 as an extension...

Adding experimental JavaScript internationalization API to V8 as an extension.
This CL implements Locale object only.

Each embeder has to decide whether to include this extension or not by editing their build rules.

See ecmascript strawman document for details on i18n API. http://wiki.ecmascript.org/doku.php?id=strawman:i18n_api

TEST=WebKit CL (in progress) will have layout tests for extension.

Landed in bleeding_edge revision 5983.

[Nebojša Ćirić, 2010-12-08 23:23:27]

Jungshik, you already reviewed most of the code, but you may want to take one
more look at it.

tools/presubmit.py found no errors.

[jungshik at Google, 2010-12-09 00:05:35]

LGTM with two nits below taken care of.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
File src/extensions/experimental/i18n-extension.cc (right):

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:113: // TODO(cira): Fetch browser
locale. Accept en-US as good default for now.
Wonder what your plan is for this. Can the registration mechanism pass a param?

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:156: std::replace(result.begin(),
result.end(), '_', '-');
Wouldn't it be better (style-compliant) to include <algorithm> explicitly for
this?

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:169:
uloc_addLikelySubtags(locale_name.c_str(), max_locale,
same here. For uloc_*, pls include <unicode/uloc.h> although it gets compiled
without that.

[Mads Ager (chromium), 2010-12-09 00:36:12]

LGTM with style nits fixed.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
File src/extensions/experimental/i18n-extension.cc (right):

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:42: "    native function
NativeJSLocale();"
Please use two-space indent.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:116: locale_name =
*v8::String::Utf8Value(args[0]->ToString());
two-space indent.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:148: all_locales->Set(i,
v8::String::New(icu_locales[i].getName()));
two-space indent and braces around the body of the for-loop.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:156: std::replace(result.begin(),
result.end(), '_', '-');
This is fine for an experimental extension. If this makes it as a language
feature in all browsers and we want to make this a part of the V8 build we
should attempt to get rid of the STL part. However, this also needs ICU which is
probably a bigger issue for eventual integration. :)

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:172: return v8::Undefined();
two space indent.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:181: return v8::Undefined();
two space indent.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:190: return v8::Undefined();
two space indent.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:200: return v8::Undefined();
two space indent.

[Nebojša Ćirić, 2010-12-09 00:57:13]

Sorry about indents. This code went from Chrome to WebKit and back...

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
File src/extensions/experimental/i18n-extension.cc (right):

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:42: "    native function
NativeJSLocale();"
On 2010/12/09 00:36:13, Mads Ager wrote:
> Please use two-space indent.

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:113: // TODO(cira): Fetch browser
locale. Accept en-US as good default for now.
On 2010/12/09 00:05:35, Jungshik Shin wrote:
> Wonder what your plan is for this. Can the registration mechanism pass a
param? 

Added your comment to todo. Seems reasonable.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:116: locale_name =
*v8::String::Utf8Value(args[0]->ToString());
On 2010/12/09 00:36:13, Mads Ager wrote:
> two-space indent.

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:148: all_locales->Set(i,
v8::String::New(icu_locales[i].getName()));
On 2010/12/09 00:36:13, Mads Ager wrote:
> two-space indent and braces around the body of the for-loop.

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:156: std::replace(result.begin(),
result.end(), '_', '-');
On 2010/12/09 00:05:35, Jungshik Shin wrote:
> Wouldn't it be better (style-compliant) to include <algorithm> explicitly for
> this? 

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:156: std::replace(result.begin(),
result.end(), '_', '-');
Ah joy :). Yes, this can be rewritten to not use STL. Added comment.

On 2010/12/09 00:36:13, Mads Ager wrote:
> This is fine for an experimental extension. If this makes it as a language
> feature in all browsers and we want to make this a part of the V8 build we
> should attempt to get rid of the STL part. However, this also needs ICU which
is
> probably a bigger issue for eventual integration. :)

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:169:
uloc_addLikelySubtags(locale_name.c_str(), max_locale,
On 2010/12/09 00:05:35, Jungshik Shin wrote:
> same here. For uloc_*, pls include <unicode/uloc.h> although it gets compiled
> without that. 

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:172: return v8::Undefined();
On 2010/12/09 00:36:13, Mads Ager wrote:
> two space indent.

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:181: return v8::Undefined();
On 2010/12/09 00:36:13, Mads Ager wrote:
> two space indent.

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:190: return v8::Undefined();
On 2010/12/09 00:36:13, Mads Ager wrote:
> two space indent.

Done.

http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18...
src/extensions/experimental/i18n-extension.cc:200: return v8::Undefined();
On 2010/12/09 00:36:13, Mads Ager wrote:
> two space indent.

Done.