All done. Thanks for the review!
I'll have to create a new client to land this once I get lgtm from you.
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.")));
On 2011/03/01 09:10:14, Mads Ager wrote:
Might be nice to make the error message more informative? How about
"BreakIterator method called on an object that is not a
BreakIterator".
Done.
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.
On 2011/03/01 09:10:14, Mads Ager wrote:
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.
Done.
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"));
I did some testing yesterday before I decided on this name. If you do
this:
Foo = function() {};
Foo.Bar = function() {};
var bar = new Foo.Bar();
Inspector in Chrome will show Foo.Bar for the type, not just Bar.
If you still think I should rename, I'll do it.
On 2011/03/01 09:10:14, Mads Ager wrote:
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) {"
I'll rename v8Locale into LocaleInfo soon so we'll lose the vendor
branding there.
BreakIterator is different from other methods under Locale object
because it's not part of the current EcmaScript proposal and I want to
keep all vendor specific methods/object under v8 prefix for now.
So for now we have ugly(?) v8Locale.v8BreakIterator, but as soon as I
rename v8Locale we will have only LocaleInfo.v8BreakIterator.
On 2011/03/01 09:10:14, Mads Ager wrote:
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
[email protected]
http://groups.google.com/group/v8-dev
