LGTM with two nits below taken care of.

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#newcode113
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/i18n-extension.cc#newcode156
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/i18n-extension.cc#newcode169
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.

http://codereview.chromium.org/5671002/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to