DOM wrappers that are not retained from Dart should be collected.

DOM wrappers that are not retained from Dart should be collected.


R=antonm@chromium.org

Committed: https://src.chromium.org/viewvc/multivm?view=rev&revision=334

[Anton Muhin, 2012-03-28 18:03:23]

1st round, but it looks really good.

https://chromiumcodereview.appspot.com/9837116/diff/1/Source/WebCore/bindings...
File Source/WebCore/bindings/dart/DartIsolate.cpp (right):

https://chromiumcodereview.appspot.com/9837116/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/DartIsolate.cpp:140: Dart_Handle persistentHandle =
Dart_NewWeakPersistentHandle(object, peer, &DartIsolate::weakCallbackWrapper);
as another option which might be more natural: as a peer for persistent handle
you may pass something like:

struct Peer {
  void* webkitObject;
  Dart_WeakPersistentHandleFinalizer finalizer;
};

WDYT?

https://chromiumcodereview.appspot.com/9837116/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/DartIsolate.cpp:152:
(*weakCallbackData.weakCallback)(persistentHandle, peer);
you allow 0 weak callback in ctor, so check here appears to be missing, but I am
not sure 0 should be allowed at all.

https://chromiumcodereview.appspot.com/9837116/diff/1/Source/WebCore/bindings...
File Source/WebCore/bindings/dart/DartIsolate.h (right):

https://chromiumcodereview.appspot.com/9837116/diff/1/Source/WebCore/bindings...
Source/WebCore/bindings/dart/DartIsolate.h:85: static void
weakCallbackWrapper(Dart_Handle object, void* peer);
do you really need a map here?  it looks like all you need is to be able to
iterate over all the elements and remove an element, couldn't you use doubly
linked list?

[podivilov, 2012-03-28 18:20:05]

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
File Source/WebCore/bindings/dart/DartIsolate.cpp (right):

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
Source/WebCore/bindings/dart/DartIsolate.cpp:140: Dart_Handle persistentHandle =
Dart_NewWeakPersistentHandle(object, peer, &DartIsolate::weakCallbackWrapper);
On 2012/03/28 18:03:23, antonmuhin wrote:
> as another option which might be more natural: as a peer for persistent handle
> you may pass something like:
> 
> struct Peer {
>   void* webkitObject;
>   Dart_WeakPersistentHandleFinalizer finalizer;
> };
> 
> WDYT?

That would require creating the Peer in the heap and deleting it in weak
callback. I prefer to avoid it.

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
Source/WebCore/bindings/dart/DartIsolate.cpp:152:
(*weakCallbackData.weakCallback)(persistentHandle, peer);
On 2012/03/28 18:03:23, antonmuhin wrote:
> you allow 0 weak callback in ctor, so check here appears to be missing, but I
am
> not sure 0 should be allowed at all.

0 is not allowed. Default ctor is used for denoting an empty value in
WTF::HashMap.

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
File Source/WebCore/bindings/dart/DartIsolate.h (right):

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
Source/WebCore/bindings/dart/DartIsolate.h:85: static void
weakCallbackWrapper(Dart_Handle object, void* peer);
On 2012/03/28 18:03:23, antonmuhin wrote:
> do you really need a map here?  it looks like all you need is to be able to
> iterate over all the elements and remove an element, couldn't you use doubly
> linked list?

I will need to dispose persistent handles in some cases, e.g. for inspected
values.

[Anton Muhin, 2012-03-28 18:25:54]

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
File Source/WebCore/bindings/dart/DartIsolate.cpp (right):

http://codereview.chromium.org/9837116/diff/1/Source/WebCore/bindings/dart/Da...
Source/WebCore/bindings/dart/DartIsolate.cpp:140: Dart_Handle persistentHandle =
Dart_NewWeakPersistentHandle(object, peer, &DartIsolate::weakCallbackWrapper);
What's the problem w/ it?  Allocating small objects is rather cheap w/ tcmalloc.
 And it won't lead to worst case O(size(map)) performance on insertion/removal.

On 2012/03/28 18:20:05, podivilov wrote:
> On 2012/03/28 18:03:23, antonmuhin wrote:
> > as another option which might be more natural: as a peer for persistent
handle
> > you may pass something like:
> > 
> > struct Peer {
> >   void* webkitObject;
> >   Dart_WeakPersistentHandleFinalizer finalizer;
> > };
> > 
> > WDYT?
> 
> That would require creating the Peer in the heap and deleting it in weak
> callback. I prefer to avoid it.

[podivilov, 2012-03-29 09:22:37]

Don't use HashMap for weak persistent handles as we never delete them.

[Anton Muhin, 2012-03-29 19:18:45]

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartDOMData.h (right):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartDOMData.h:50: typedef
HashSet<WeakCallbackData*> DartWeakCallbacks;
nit: name mismatch: CallbackData vs. Callbacks

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartDOMWrapper.h (left):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartDOMWrapper.h:415: kDerefObjectFunctionIndex,
please, do not forget to change pure Dart implementation as well.

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartDOMWrapper.h (right):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartDOMWrapper.h:316: template <class WebKitClass>
if you please, do not rename as a part of another change---it's hard to tell
rename changes from semantic ones.

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartUtilities.cpp (right):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartUtilities.cpp:396:
DartUtilities::domDataForCurrentIsolate()->weakCallbacks()->remove(callbackData);
maybe performance FIXME?  linear search looks scary

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartUtilities.cpp:403: WeakCallbackData*
callbackData = new WeakCallbackData;
why not provide a ctor in the class itself?

[podivilov, 2012-03-30 10:36:27]

PTAL.

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartDOMData.h (right):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartDOMData.h:50: typedef
HashSet<WeakCallbackData*> DartWeakCallbacks;
On 2012/03/29 19:18:45, antonmuhin wrote:
> nit: name mismatch: CallbackData vs. Callbacks

Done.

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartDOMWrapper.h (left):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartDOMWrapper.h:415: kDerefObjectFunctionIndex,
On 2012/03/29 19:18:45, antonmuhin wrote:
> please, do not forget to change pure Dart implementation as well.

https://chromiumcodereview.appspot.com/9968004

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartDOMWrapper.h (right):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartDOMWrapper.h:316: template <class WebKitClass>
On 2012/03/29 19:18:45, antonmuhin wrote:
> if you please, do not rename as a part of another change---it's hard to tell
> rename changes from semantic ones.

Done.

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
File Source/WebCore/bindings/dart/DartUtilities.cpp (right):

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartUtilities.cpp:396:
DartUtilities::domDataForCurrentIsolate()->weakCallbacks()->remove(callbackData);
On 2012/03/29 19:18:45, antonmuhin wrote:
> maybe performance FIXME?  linear search looks scary

weakCallbacks is HashSet, not vector.

http://codereview.chromium.org/9837116/diff/8001/Source/WebCore/bindings/dart...
Source/WebCore/bindings/dart/DartUtilities.cpp:403: WeakCallbackData*
callbackData = new WeakCallbackData;
On 2012/03/29 19:18:45, antonmuhin wrote:
> why not provide a ctor in the class itself?

Done.

[Anton Muhin, 2012-03-30 11:36:00]

LGTM w/ nit addressed

http://codereview.chromium.org/9837116/diff/13001/Source/WebCore/bindings/dar...
File Source/WebCore/bindings/dart/DartUtilities.cpp (right):

http://codereview.chromium.org/9837116/diff/13001/Source/WebCore/bindings/dar...
Source/WebCore/bindings/dart/DartUtilities.cpp:395: void* peer;
nit: m_peer, m_weakCallback, etc.?