Sorry about indents. This code went from Chrome to WebKit and back...
http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18n-extension.cc File src/extensions/experimental/i18n-extension.cc (right): http://codereview.chromium.org/5671002/diff/1/src/extensions/experimental/i18n-extension.cc#newcode42 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/i18n-extension.cc#newcode113 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/i18n-extension.cc#newcode116 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/i18n-extension.cc#newcode148 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/i18n-extension.cc#newcode156 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/i18n-extension.cc#newcode156 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/i18n-extension.cc#newcode169 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/i18n-extension.cc#newcode172 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/i18n-extension.cc#newcode181 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/i18n-extension.cc#newcode190 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/i18n-extension.cc#newcode200 src/extensions/experimental/i18n-extension.cc:200: return v8::Undefined(); On 2010/12/09 00:36:13, Mads Ager wrote:
two space indent.
Done. http://codereview.chromium.org/5671002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
