This is headed in the right direction on the use of the V8 API. There are a
couple of issues that we need to address. Some of this might be easier to
discuss face to face. Please feel free to setup a video conference meeting
with
me if needed.
The main issue is that we need to hook up the lifetime of the JS break
iterator
object to the lifetime of the C++ object. When there are no more JS
references
to the JS wrapper we can delete the C++ break iterator. For that we need a
persistent handle pr. BreakIterator to its wrapper object. That persistent
handle is the one that needs to be weak and have a callback that deletes the
BreakIterator *and* disposes the persistent handle.
http://codereview.chromium.org/6598014/diff/4003/src/extensions/experimental/break-iterator.cc
File src/extensions/experimental/break-iterator.cc (right):
http://codereview.chromium.org/6598014/diff/4003/src/extensions/experimental/break-iterator.cc#newcode42
src/extensions/experimental/break-iterator.cc:42: return
static_cast<icu::BreakIterator*>(obj->GetPointerFromInternalField(0));
You need to be more careful here. The users of UnpackBreakIterator are
functions. These functions can be moved to other objects and therefore
the holder does not need to be a break iterator object at all.
Instead of using an ObjectTemplate for you break iterator wrappers I
think you should use a FunctionTemplate instead. Then here you can check
that the argument is an instance of that function template. If it is, it
will have be internal field and the value in it will be a BreakIterator
pointer.
Have a look at FunctionTemplate. You add the methods on the
instance_template as you do now. Then you can use
templ->HasInstance(obj).
http://codereview.chromium.org/6598014/diff/4003/src/extensions/experimental/break-iterator.cc#newcode49
src/extensions/experimental/break-iterator.cc:49: delete
UnpackBreakIterator(v8::Persistent<v8::Object>::Cast(object));
You need to do object.Dispose() as well to delete the persistent handle
as well. This callback needs to be used on all break iterator instances
instead of on the template (see below).
http://codereview.chromium.org/6598014/diff/4003/src/extensions/experimental/break-iterator.cc#newcode150
src/extensions/experimental/break-iterator.cc:150: v8::HandleScope
handle_scope;
Is there a reason not have have this handle scope earlier in this
method? You are also allocating strings above for instance. The handles
for those might as well be local as well.
http://codereview.chromium.org/6598014/diff/4003/src/extensions/experimental/break-iterator.cc#newcode167
src/extensions/experimental/break-iterator.cc:167: // Make template weak
so we can delete iterator once GC kicks in.
This is not the thing that you want to make weak. The
break_iterator_template does not correspond to a concrete break
iterator?
http://codereview.chromium.org/6598014/diff/4003/src/extensions/experimental/break-iterator.cc#newcode174
src/extensions/experimental/break-iterator.cc:174:
wrapper->SetPointerInInternalField(0, break_iterator);
This is where you set up the correspondence. Here you need to decide on
the memory management story that you want. I think your BreakIterator
instance needs to have a reference (persistent handle) to the wrapper so
it can return it again for the next access to it. Also, that will allow
you to tie the life-time of the wrapper to the lifetime of the
breakiterator C++ instance.
Is the breakiterator C++ instance used by anything else than JavaScript?
If not, then each break iterator C++ instance should have a persistent
handle to the JS wrapper. This persistent handle should be weak and in
the callback you should call dispose on the persistent handle *and*
delete the break iterator object.
http://codereview.chromium.org/6598014/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
