[v8-dev] Re: Adding break iterator support to the i18n api extension.... (issue6598014)

[ager]

Getting really close. One type checking thing and then we are ready to  
land. :)

http://codereview.chromium.org/6598014/diff/6008/src/extensions/experimental/break-iterator.cc
File src/extensions/experimental/break-iterator.cc (right):

http://codereview.chromium.org/6598014/diff/6008/src/extensions/experimental/break-iterator.cc#newcode65
src/extensions/experimental/break-iterator.cc:65:
v8::String::New("Internal - Unexpected object type.")));
Might be nice to make the error message more informative? How about
"BreakIterator method called on an object that is not a BreakIterator".

http://codereview.chromium.org/6598014/diff/6008/src/extensions/experimental/break-iterator.cc#newcode147
src/extensions/experimental/break-iterator.cc:147: // No need to check
args here, we do it in JavaScript.
I actually think that you do want to check that the arguments are
strings here and throw an exception otherwise. In the JS code, you only
check that the arguments are not undefined. You can get arbitrary
objects in here and converting them to string does not necessarily
succeed.

Consider the following JavaScript code:

var o = { valueOf: function() { throw 42; } };
var s = "" + o;

This will perform a ToString operation on 'o' which will result in an
exception being thrown. You can get in this situation for the two
ToString calls here and you do not deal with the exception case (where
you will get a null handle back from V8).

I think it is fine to not deal with it by checking that the two
arguments are strings before the conversion and throwing an exception
otherwise.

http://codereview.chromium.org/6598014/diff/6008/src/extensions/experimental/break-iterator.cc#newcode182
src/extensions/experimental/break-iterator.cc:182:
raw_template->SetClassName(v8::String::New("v8Locale.v8BreakIterator"));
Maybe just v8BreakIterator?

http://codereview.chromium.org/6598014/diff/6008/src/extensions/experimental/i18n-extension.cc
File src/extensions/experimental/i18n-extension.cc (right):

http://codereview.chromium.org/6598014/diff/6008/src/extensions/experimental/i18n-extension.cc#newcode92
src/extensions/experimental/i18n-extension.cc:92:
"v8Locale.v8BreakIterator = function(locale, type) {"
Do you need to have the v8 prefix on everything in here? I would guess
that having a V8 prefix on the v8Locale object is enough. You cannot get
to any of this without going through that which makes it clear that this
is v8 specific.

I would remove the v8 prefix from v8BreakIterator and from
v8CreateBreakIterator (which would be consistent with using names like
displayName instead of v8DisplayName for other methods).

http://codereview.chromium.org/6598014/

--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev