[webkit-reviews] review granted: [Bug 112608] Objective-C API: Need a good way to preserve custom properties on JS wrappers : [Attachment 193712] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 112608: Objective-C API: Need a good way to preserve custom properties on
JS wrappers
https://bugs.webkit.org/show_bug.cgi?id=112608

Attachment 193712: Patch
https://bugs.webkit.org/attachment.cgi?id=193712&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=193712&action=review


r=me

> Source/JavaScriptCore/ChangeLog:20
> +	   1. The child is referenced from JavaScript.

Let's say, "The child is exported to JavaScript".

> Source/JavaScriptCore/API/JSManagedReference.h:39
> ++ (void)addManagedReference:(id)object withOwner:(id)owner
toVirtualMachine:(JSVirtualMachine *)virtualMachine;
> ++ (void)removeManagedReference:(id)object withOwner:(id)owner
fromVirtualMachine:(JSVirtualMachine *)virtualMachine;

Let's put these methods on JSVirtualMachine. Since we never create a
JSManagedReference object, it's probably clearer that way.

> Source/JavaScriptCore/API/JSManagedReference.mm:93
> +void scanExternalObjectGraph(JSC::JSGlobalData& globalData,
JSC::SlotVisitor& visitor, void* root)

Let's use a WTF data structure for the stack here, since it's so much more
efficient.

Let's use a weak NSMapTable for the ownedObjects set to avoid retaining things
longer than necessary if the client forgets to call removeManagedReference.

Let's put counting in the set, so you can call this in a multiple owner style.

> Source/JavaScriptCore/heap/SlotVisitorInlines.h:124
> +inline bool SlotVisitor::threadSafeContainsOpaqueRoot(void* root)

"Thread safe" doesn't fully describe the requirements here. How about
containsOpaqueRootTriState(), returning either TrueTriStae or MixedTriState,
but never FalseTriState, since we can never say for sure that other threads
haven't locally added this thing?