Add v8Locale.Collator

Add v8Locale.Collator 

This is a partial implementation of Collator per what's agreed upon at the last
ECMAScript meeting + mailing list. Only the following three options are implemented: ignoreAccent, ignoreCase, numeric. 

ChromeOS and Chrome need this feature for M12. This could be added as chrome extension API. Giiven that we have a rough agreement on the collation part of ECMAScript API, we thought it'd save us some duplicated work adding this to v8 (experimental i18n api) now rather than implementing it in Chrome now and moving it later. 


BUG=28604
TEST=http://i18nl10n.com/chrome/coll2.html
Committed: http://code.google.com/p/v8/source/detail?r=7620

[jungshik at Google, 2011-03-11 07:05:07]

The CL is not yet ready for the full review. 

However, I need some help from Mads. In the current form, it more or less works
(see http://i18nl10n.com/chrome/coll.html). 

However, the following does not work (taken from
http://i18nl10n.com/chrome/coll.js) : 

var locale = new v8Locale(lang);
var coll = locale.createCollator(options);
var res = coll.compare("zeus", "Årenius"); 
var sortedWords = words.sort(coll.compare);

In CollatorCompare() in collator.cc, |UnpackCollator(args.Holder())| returns
NULL. I vaguely remember you mentioned something about a related issue, but I
didn't quite understand that. Could you enlighten me as to how to make the above
work? 

BTW,  if the last line is replaced with the following two lines, it works as
expected. 

var compare = function(a, b) { return coll.compare(a, b);};
var sortedWords = words.sort(compare);

[Nebojša Ćirić, 2011-03-11 21:15:20]

Plus some lint errors...

http://codereview.chromium.org/6673011/diff/1/src/extensions/experimental/col...
File src/extensions/experimental/collator.cc (right):

http://codereview.chromium.org/6673011/diff/1/src/extensions/experimental/col...
src/extensions/experimental/collator.cc:39: icu::Collator*
Collator::UnpackCollator(v8::Handle<v8::Object> obj) {
I see delete and unpack methods repeated in the future. We could extract them at
some point into template methods in extension-util.{cc,h}.

http://codereview.chromium.org/6673011/diff/1/src/extensions/experimental/col...
src/extensions/experimental/collator.cc:62: static v8::Handle<v8::Value>
ThrowUnexpectedObjectError() {
This could also be refactored into separate method/file, with error message as a
parameter.

http://codereview.chromium.org/6673011/diff/1/src/extensions/experimental/col...
src/extensions/experimental/collator.cc:101: v8::String::New("Locale and
collation option are required.")));
Is it option or options?

http://codereview.chromium.org/6673011/diff/1/src/extensions/experimental/col...
src/extensions/experimental/collator.cc:118: //v8::Local<v8::Object> options =
v8::Local<Object>::Cast(options(args[1]));
Remove commented out code?

http://codereview.chromium.org/6673011/diff/1/src/extensions/experimental/col...
src/extensions/experimental/collator.cc:120:
options->Get(v8::String::New("v8Strength"));
Do we need v8Strength? I know we didn't settle yet on what to use for the
options, but I don't think there is a problem with naming it Strength/CaseLevel.

[Mads Ager (chromium), 2011-03-14 18:08:09]

I haven't had the time to look at this today. However, I can quickly explain why


var sortedWords = words.sort(coll.compare);

does not work as you expect. You are extracting the compare function and passing
it to the sort function on arrays (I assume that words is a normal array here?).
That function is then called to compare the elements. However, it is not called
with the coll object as its receiver, it is called with the global object as the
receiver. The global object cannot be transformed to a Collator object because
it is not. 

Standalone JS example showing that this is the correct JS behavior:

$ ./shell
V8 version 3.2.0 (candidate)
> var ints = [2, 3, 4];         
> function f() { if (!this.hasOwnProperty('x')) throw "NOPE!"; }
> var o = new Object();
> o.f = f
function f() { if (!this.hasOwnProperty('x')) throw "NOPE!"; }
> o.f()
(shell):1: NOPE!
function f() { if (!this.hasOwnProperty('x')) throw "NOPE!"; }
                                              ^
> o.x = 42 
42
> o.f()
> ints.sort(o.f)
(shell):1: NOPE!
function f() { if (!this.hasOwnProperty('x')) throw "NOPE!"; }

[jungshik at Google, 2011-03-14 23:23:01]

Thank you for the explanation. 

Is there any way to make 'array.sort(coll.compare)' work on my side (in C++) or
is that the way sort() is supposed to work no matter what? It seems that the
latter is the case, but I'd better ask you to make sure. 

I guess many callers want to define their own compare function any way because
they're likely to compare more complex objects than simple strings. For
instance, a file browser may want to list directories before regular files and
would use the following: 

var compareFileName = function(a, b) { 
   if (a.type == b.type) { return coll.compare(a.name, b.name);}
   if (a.type =="directory") return -1;
   return 1;
} 

So, not being able to directly pass coll.compare() is probably ok. However, if
there's any way to make it possible, pls let me know.

[Mads Ager (chromium), 2011-03-15 07:55:48]

On 2011/03/14 23:23:01, Jungshik Shin wrote:
> Thank you for the explanation. 
> 
> Is there any way to make 'array.sort(coll.compare)' work on my side (in C++)
or
> is that the way sort() is supposed to work no matter what? It seems that the
> latter is the case, but I'd better ask you to make sure. 

This is just the way JavaScript works. You are extracting the function and
calling it with a receiver that is not a collator. You are right that you cannot
make this work this way. I don't think you should in any case because this is
not how JS works.

> I guess many callers want to define their own compare function any way because
> they're likely to compare more complex objects than simple strings. For
> instance, a file browser may want to list directories before regular files and
> would use the following: 
> 
> var compareFileName = function(a, b) { 
>    if (a.type == b.type) { return coll.compare(a.name, b.name);}
>    if (a.type =="directory") return -1;
>    return 1;
> } 
> 
> So, not being able to directly pass coll.compare() is probably ok. However, if
> there's any way to make it possible, pls let me know.

[Nebojša Ćirić, 2011-03-22 16:02:58]

I've presented the problem to EcmaScript body, and they proposed 2 solutions.

Q:

So if we have something like this:

LocaleInfo.Collator.prototype.compare = function(a, b) {
...
};

How do you specify it's bound by default?

A:

Two choices:

1) Don't put it on the prototype. Rather, make it part of each instance state,
don't use "this" in the function, and have the function lexically capture all
the state it needs.

2) Put it on the prototype using a binding accessor:

function makeBindingAccessorDescriptor(method) {
    return {
        get: function() { return method.bind(this); }
        set: writable: ??, enumerable: ??, configurable: ?? // according to your
intensions
    };
}

Object.defineProperty(LocaleInfo.Collator.prototype, 'compare', 
                                 makeBindingAccessorDescriptor(function(a, b) {
    ....
}));

More details about definePrototype and get/set statements -
http://ejohn.org/blog/ecmascript-5-objects-and-properties/

Now the question is, can we do 2) in v8...

On 2011/03/15 07:55:48, Mads Ager wrote:
> On 2011/03/14 23:23:01, Jungshik Shin wrote:
> > Thank you for the explanation. 
> > 
> > Is there any way to make 'array.sort(coll.compare)' work on my side (in C++)
> or
> > is that the way sort() is supposed to work no matter what? It seems that the
> > latter is the case, but I'd better ask you to make sure. 
> 
> This is just the way JavaScript works. You are extracting the function and
> calling it with a receiver that is not a collator. You are right that you
cannot
> make this work this way. I don't think you should in any case because this is
> not how JS works.
> 
> > I guess many callers want to define their own compare function any way
because
> > they're likely to compare more complex objects than simple strings. For
> > instance, a file browser may want to list directories before regular files
and
> > would use the following: 
> > 
> > var compareFileName = function(a, b) { 
> >    if (a.type == b.type) { return coll.compare(a.name, b.name);}
> >    if (a.type =="directory") return -1;
> >    return 1;
> > } 
> > 
> > So, not being able to directly pass coll.compare() is probably ok. However,
if
> > there's any way to make it possible, pls let me know.

[Nebojša Ćirić, 2011-03-22 16:04:17]

Also, defineProperty is part of Harmony but it seems only Opera doesn't have it
(FF4, IE9, Chrome 5 have it, and IE8 only for DOM).

On 2011/03/22 16:02:58, Nebojša Ćirić wrote:
> I've presented the problem to EcmaScript body, and they proposed 2 solutions.
> 
> Q:
> 
> So if we have something like this:
> 
> LocaleInfo.Collator.prototype.compare = function(a, b) {
> ...
> };
> 
> How do you specify it's bound by default?
> 
> A:
> 
> Two choices:
> 
> 1) Don't put it on the prototype. Rather, make it part of each instance state,
> don't use "this" in the function, and have the function lexically capture all
> the state it needs.
> 
> 2) Put it on the prototype using a binding accessor:
> 
> function makeBindingAccessorDescriptor(method) {
>     return {
>         get: function() { return method.bind(this); }
>         set: writable: ??, enumerable: ??, configurable: ?? // according to
your
> intensions
>     };
> }
> 
> Object.defineProperty(LocaleInfo.Collator.prototype, 'compare', 
>                                  makeBindingAccessorDescriptor(function(a, b)
{
>     ....
> }));
> 
> More details about definePrototype and get/set statements -
> http://ejohn.org/blog/ecmascript-5-objects-and-properties/
> 
> Now the question is, can we do 2) in v8...
> 
> On 2011/03/15 07:55:48, Mads Ager wrote:
> > On 2011/03/14 23:23:01, Jungshik Shin wrote:
> > > Thank you for the explanation. 
> > > 
> > > Is there any way to make 'array.sort(coll.compare)' work on my side (in
C++)
> > or
> > > is that the way sort() is supposed to work no matter what? It seems that
the
> > > latter is the case, but I'd better ask you to make sure. 
> > 
> > This is just the way JavaScript works. You are extracting the function and
> > calling it with a receiver that is not a collator. You are right that you
> cannot
> > make this work this way. I don't think you should in any case because this
is
> > not how JS works.
> > 
> > > I guess many callers want to define their own compare function any way
> because
> > > they're likely to compare more complex objects than simple strings. For
> > > instance, a file browser may want to list directories before regular files
> and
> > > would use the following: 
> > > 
> > > var compareFileName = function(a, b) { 
> > >    if (a.type == b.type) { return coll.compare(a.name, b.name);}
> > >    if (a.type =="directory") return -1;
> > >    return 1;
> > > } 
> > > 
> > > So, not being able to directly pass coll.compare() is probably ok.
However,
> if
> > > there's any way to make it possible, pls let me know.

[Nebojša Ćirić, 2011-03-22 16:30:18]

The question still stands, should we actually do this (.bind) or not, since most
of the core API don't...

On 2011/03/22 16:04:17, Nebojša Ćirić wrote:
> Also, defineProperty is part of Harmony but it seems only Opera doesn't have
it
> (FF4, IE9, Chrome 5 have it, and IE8 only for DOM).
> 
> On 2011/03/22 16:02:58, Nebojša Ćirić wrote:
> > I've presented the problem to EcmaScript body, and they proposed 2
solutions.
> > 
> > Q:
> > 
> > So if we have something like this:
> > 
> > LocaleInfo.Collator.prototype.compare = function(a, b) {
> > ...
> > };
> > 
> > How do you specify it's bound by default?
> > 
> > A:
> > 
> > Two choices:
> > 
> > 1) Don't put it on the prototype. Rather, make it part of each instance
state,
> > don't use "this" in the function, and have the function lexically capture
all
> > the state it needs.
> > 
> > 2) Put it on the prototype using a binding accessor:
> > 
> > function makeBindingAccessorDescriptor(method) {
> >     return {
> >         get: function() { return method.bind(this); }
> >         set: writable: ??, enumerable: ??, configurable: ?? // according to
> your
> > intensions
> >     };
> > }
> > 
> > Object.defineProperty(LocaleInfo.Collator.prototype, 'compare', 
> >                                  makeBindingAccessorDescriptor(function(a,
b)
> {
> >     ....
> > }));
> > 
> > More details about definePrototype and get/set statements -
> > http://ejohn.org/blog/ecmascript-5-objects-and-properties/
> > 
> > Now the question is, can we do 2) in v8...
> > 
> > On 2011/03/15 07:55:48, Mads Ager wrote:
> > > On 2011/03/14 23:23:01, Jungshik Shin wrote:
> > > > Thank you for the explanation. 
> > > > 
> > > > Is there any way to make 'array.sort(coll.compare)' work on my side (in
> C++)
> > > or
> > > > is that the way sort() is supposed to work no matter what? It seems that
> the
> > > > latter is the case, but I'd better ask you to make sure. 
> > > 
> > > This is just the way JavaScript works. You are extracting the function and
> > > calling it with a receiver that is not a collator. You are right that you
> > cannot
> > > make this work this way. I don't think you should in any case because this
> is
> > > not how JS works.
> > > 
> > > > I guess many callers want to define their own compare function any way
> > because
> > > > they're likely to compare more complex objects than simple strings. For
> > > > instance, a file browser may want to list directories before regular
files
> > and
> > > > would use the following: 
> > > > 
> > > > var compareFileName = function(a, b) { 
> > > >    if (a.type == b.type) { return coll.compare(a.name, b.name);}
> > > >    if (a.type =="directory") return -1;
> > > >    return 1;
> > > > } 
> > > > 
> > > > So, not being able to directly pass coll.compare() is probably ok.
> However,
> > if
> > > > there's any way to make it possible, pls let me know.

[jungshik at Google, 2011-04-11 22:53:51]

I left alone the binding part as was done initially. So, directly passing
'collator.compare' wouldn't work because 'compare' is not bound to a Collator.

We can sort that out once the ECMAScript committee has an agreement. For
internal use in ChromeOS, it should not matter.

[Nebojša Ćirić, 2011-04-11 23:10:33]

http://codereview.chromium.org/6673011/diff/17002/src/extensions/experimental...
File src/extensions/experimental/collator.cc (right):

http://codereview.chromium.org/6673011/diff/17002/src/extensions/experimental...
src/extensions/experimental/collator.cc:69: static void
ExtractBooleanOption(const v8::Local<v8::Object>& options,
Comment on static method?

http://codereview.chromium.org/6673011/diff/17002/src/extensions/experimental...
src/extensions/experimental/collator.cc:134: bool ignore_case, ignore_accents,
numeric;
Is this ok wrt. style?

http://codereview.chromium.org/6673011/diff/17002/src/extensions/experimental...
src/extensions/experimental/collator.cc:139:
collator->setStrength(icu::Collator::PRIMARY);
Maybe put a comment that this is a default strength?

http://codereview.chromium.org/6673011/diff/17002/src/extensions/experimental...
src/extensions/experimental/collator.cc:154: 
Extra new line here.

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

http://codereview.chromium.org/6673011/diff/17002/src/extensions/experimental...
src/extensions/experimental/i18n-extension.cc:113: "  collator.options =
options;"
Why are you hanging all options here? Only those we recognize and use we put in
collator.options (resolved values).

[jungshik at Google, 2011-04-13 00:22:53]

Mads and Nebojša , 

can you take a look? 

This won't be cleanly applied to the bleeding-edge (Nebojša moved the JS string
out of i18n-extension.cc), but merging should be easy. Once approved, I'll make
a patch against bleeding-edge and give it to Nebojša  so that he can land. 

Thank you,

[Mads Ager (google), 2011-04-13 13:43:24]

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
File src/extensions/experimental/collator.cc (right):

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:73: bool *result) {
bool * -> bool*

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:74: v8::Local<v8::Value> value =
options->Get(v8::String::New(option));
I would guess that this can throw an exception? Can the user get hold of the
option object? In that case, he could add something that throws an exception to
it:

option.__defineGetter__("my_option", function() { throw 42; });

I that case the value handle will be a null handle. Either you should have a
TryCatch and check that there was no exception or you should check that the
handle is not null.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:84: // When there's an ICU error, delete
|collator| and throw a JavaScript error
Please add to this comment that no JavaScript wrapper has been created when this
is called. I was initially worried that you would have a deleted pointer in an
internal field.

The side effect of deleting the collator here is sort of nasty. I would do that
at the call sites or rename this function.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:124: v8::String::New("Locale and
collation options are required.")));
Four-space indent.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:134: if (U_FAILURE(status))
Please use braces for multi-line if.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:152: if (U_FAILURE(status))
Braces.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:158: if (!ignore_accents)
Braces for these.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:167: if (U_FAILURE(status))
Braces.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
File src/extensions/experimental/collator.h (right):

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.h:49: // Unpacks break iterator object from
corresponding JavaScript object.
break iterator -> collator

[jungshik at Google, 2011-04-13 21:36:38]

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
File src/extensions/experimental/collator.cc (right):

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:73: bool *result) {
On 2011/04/13 13:43:24, ager wrote:
> bool * -> bool*

Done.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:84: // When there's an ICU error, delete
|collator| and throw a JavaScript error
On 2011/04/13 13:43:24, ager wrote:
> Please add to this comment that no JavaScript wrapper has been created when
this
> is called. I was initially worried that you would have a deleted pointer in an
> internal field.
> 
> The side effect of deleting the collator here is sort of nasty. I would do
that
> at the call sites or rename this function.

Ok. I changed this function to just throw an error with msg and made callers to
delete ICU collator.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:124: v8::String::New("Locale and
collation options are required.")));
On 2011/04/13 13:43:24, ager wrote:
> Four-space indent.

Done.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:134: if (U_FAILURE(status))
On 2011/04/13 13:43:24, ager wrote:
> Please use braces for multi-line if.

Done.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:152: if (U_FAILURE(status))
On 2011/04/13 13:43:24, ager wrote:
> Braces.

Done.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:158: if (!ignore_accents)
On 2011/04/13 13:43:24, ager wrote:
> Braces for these.

Done.

http://codereview.chromium.org/6673011/diff/24002/src/extensions/experimental...
src/extensions/experimental/collator.cc:167: if (U_FAILURE(status))
On 2011/04/13 13:43:24, ager wrote:
> Braces.

Done.

[jungshik at Google, 2011-04-13 21:56:32]

http://codereview.chromium.org/6673011/diff/23003/src/extensions/experimental...
File src/extensions/experimental/collator.cc (right):

http://codereview.chromium.org/6673011/diff/23003/src/extensions/experimental...
src/extensions/experimental/collator.cc:77: return false;
I put up a test page at http://www.i18nl10n.com/chrome/colltest.html 

When I open it, no exception is caught with this. So, try_catch lines are no-op
here.

On the other hand, value is indeed empty for ignoreAccents (whcih is set to a
function throwing an exception 42).  So, this function just return false as if
that option is not set. So far so good. 

However, down the road at line 179, an exception is thrown. I'll add more there.

http://codereview.chromium.org/6673011/diff/23003/src/extensions/experimental...
src/extensions/experimental/collator.cc:179: v8::Local<v8::FunctionTemplate>
raw_template(v8::FunctionTemplate::New());
Here I got an console message about an uncaught exception:

4984:2575:705643956858483:INFO:CONSOLE(0)] "Uncaught 42," source:
http://www.corp.google.com/~jungshik/chrome/colltest.html (7)

colltest.html line 7 is where ignoreAccents is set to a function throwing 42
exception with __defineGetter__. 

7: options.__defineGetter__("ignoreAccents", function() { throw 42; });
8: var coll = locale.createCollator(options);

When I ignore and goes on in a debugger, eventually, at line 200, renderer
crashes. 

200: collator_template_->GetFunction()->NewInstance()

Actual crash seems to happen in api.cc::3610 (has_pending_exception is true
there). 

Maybe, I can catch an exception at line 179. But, I wonder why one is thrown
there (which just makes a new function template).

[jungshik at Google, 2011-04-13 21:59:04]

Below is what's dumped in a terminal when a renderer crashes.  Could you
enlighten me as to what to do to avoid this crash? 

-----Cut----here-------

# Fatal error in /Volumes/Works/cr/t/src/v8/tools/gyp/../../src/handles-inl.h,
line 48
# CHECK(location_ != __null) failed
#


==== Stack trace ============================================

Security context: 0x2faf0021 <String[26]: http://www.corp.google.com>
    1: new constructor [v8/i18n:1] (this=0x3803b691 <JS
Object>#0#,locale=0x3803b0b1 <String[2]: en>,options=0x3803b3b5 <an Object>>#1#)
    3: createCollator [v8/i18n:1] (this=0x3803b02d <JS
Object>#2#,options=0x3803b3b5 <an Object>>#1#)
    4: testColl [http://www.corp.google.com/~jungshik/chrome/colltest.html:8]
(this=0x34c81c89 <JS Global Object>#3#)
    5: /* anonymous */
[http://www.corp.google.com/~jungshik/chrome/colltest.html:1] (this=0x34c81c89
<JS Global Object>#3#,evt=0x3803a9d9 <an Event>>#4#)
    6: onload [http://www.corp.google.com/~jungshik/chrome/colltest.html:2]
(this=0x34c81c89 <JS Global Object>#3#,evt=0x3803a9d9 <an Event>>#4#)

==== Details ================================================

[1]: new constructor [v8/i18n:1] (this=0x3803b691 <JS
Object>#0#,locale=0x3803b0b1 <String[2]: en>,options=0x3803b3b5 <an Object>>#1#)
{
  // stack-allocated locals
  var collator = 0x30fa004d <undefined>
  var NativeJSCollator = 0x3803b6c5 <JS Function NativeJSCollator>#5#
  // expression stack (top to bottom)
  [06] : 0x3803b6c5 <JS Function NativeJSCollator>#5#
  [05] : 0x3803b3b5 <an Object>>#1#
  [04] : 0x3803b0b1 <String[2]: en>
  [03] : 0x34c81c89 <JS Global Object>#3#
  [02] : 0x3803b6c5 <JS Function NativeJSCollator>#5#
--------- s o u r c e   c o d e ---------
function (locale, options) {  native function NativeJSCollator();  var collator
= NativeJSCollator(locale,      options === undefined ? {} : options);  return
collator;}
-----------------------------------------
}

[3]: createCollator [v8/i18n:1] (this=0x3803b02d <JS
Object>#2#,options=0x3803b3b5 <an Object>>#1#) {
  // expression stack (top to bottom)
  [02] : 0x3803b3b5 <an Object>>#1#
  [01] : 0x3803b0b1 <String[2]: en>
  [00] : 0x34c82305 <JS Function>#6#
--------- s o u r c e   c o d e ---------
function (options) {  return new v8Locale.Collator(this.locale, options);}
-----------------------------------------

}

[4]: testColl [http://www.corp.google.com/~jungshik/chrome/colltest.html:8]
(this=0x34c81c89 <JS Global Object>#3#) {
  // stack-allocated locals
  var coll = 0x30fa004d <undefined>
  var options = 0x3803b3b5 <an Object>>#1#
  var locale = 0x3803b02d <JS Object>#2#
--------- s o u r c e   c o d e ---------
function testColl() {?        var locale = new v8Locale("en");?        var
options = {"ignoreCase": false}; ?       
options.__defineGetter__("ignoreAccents", function() { throw 42; });?        var
coll = locale.createCollator(options);?      }
-----------------------------------------
}

[5]: /* anonymous */
[http://www.corp.google.com/~jungshik/chrome/colltest.html:1] (this=0x34c81c89
<JS Global Object>#3#,evt=0x3803a9d9 <an Event>>#4#) {
  // expression stack (top to bottom)
  [00] : 0x34c85bfd <JS Function testColl>#7#
--------- s o u r c e   c o d e ---------
function (evt){testColl()?}
-----------------------------------------
}

[6]: onload [http://www.corp.google.com/~jungshik/chrome/colltest.html:2]
(this=0x34c81c89 <JS Global Object>#3#,evt=0x3803a9d9 <an Event>>#4#) {
  // heap-allocated locals
  var .arguments = // warning: missing context slot - inconsistent frame?
  var arguments = // warning: missing context slot - inconsistent frame?
  // expression stack (top to bottom)
--------- s o u r c e   c o d e ---------
function onload(evt) {with (this.ownerDocument ? this.ownerDocument : {}) {with
(this.form ? this.form : {}) {with (this) {return
(function(evt){testColl()?}).call(this, evt);}}}}
-----------------------------------------
}


==== Key         ============================================

 #0# 0x3803b691: 0x3803b691 <JS Object>
 #1# 0x3803b3b5: 0x3803b3b5 <an Object>>
        ignoreCase: 0x30fa00b5 <false>
 #2# 0x3803b02d: 0x3803b02d <JS Object>
            locale: 0x3803b0b1 <String[2]: en>
          language: 0x3803b11d <String[2]: en>
            script: 0x30fa004d <undefined>
            region: 0x30fa004d <undefined>
 #3# 0x34c81c89: 0x34c81c89 <JS Global Object>
 #4# 0x3803a9d9: 0x3803a9d9 <an Event>>
 #5# 0x3803b6c5: 0x3803b6c5 <JS Function NativeJSCollator>
 #6# 0x34c82305: 0x34c82305 <JS Function>
 #7# 0x34c85bfd: 0x34c85bfd <JS Function testColl>
=====================


Program received signal SIGABRT, Aborted.
[Switching to process 4984]
0x977870a6 in __semwait_signal ()
------Cut----here-------

[jungshik at Google, 2011-04-14 00:19:48]

Mads, can you take another look? 

Using TryCatch directly instead of Local<TryCatch> works well.

[Mads Ager (chromium), 2011-04-14 08:57:52]

LGTM

http://codereview.chromium.org/6673011/diff/26002/src/extensions/experimental...
File src/extensions/experimental/collator.cc (right):

http://codereview.chromium.org/6673011/diff/26002/src/extensions/experimental...
src/extensions/experimental/collator.cc:54: // pointing to a break iterator.
break iterator -> collator

[jungshik at Google, 2011-04-14 19:44:20]

Thank you for the review.

cira landed it in the bleedingedge on my behalf ( r7620 ).