[webkit-reviews] review granted: [Bug 111088] Objective-C API: Need a good way to reference event handlers without causing cycles : [Attachment 191571] fix JSManagedValue init

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 111088: Objective-C API: Need a good way to reference event handlers
without causing cycles
https://bugs.webkit.org/show_bug.cgi?id=111088

Attachment 191571: fix JSManagedValue init
https://bugs.webkit.org/attachment.cgi?id=191571&action=review

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


r=me

> Source/JavaScriptCore/API/JSBase.cpp:114
> +void JSSynchronousGarbageCollect(JSContextRef ctx)

Let's add "ForDebugging" to the end of the name, in case that helps.

> Source/JavaScriptCore/API/JSBasePrivate.h:55
> +/*!
> + at function
> + at abstract Synchronously runs a full mark-sweep garbage collection.
> + at param ctx The execution context to use.
> + at discussion This function is only used internally for tests. If you're not 
> +sure you need to use this function then you don't need to.
> +*/
> +JS_EXPORT void JSSynchronousGarbageCollect(JSContextRef ctx);

Let's not declare this in the header at all. Just put the declaration in
testapi.mm. That way, we'll reduce the chances that someone uses it
accidentally.

> Source/JavaScriptCore/API/JSManagedValue.h:39
> ++ (JSManagedValue *)weakValueWithValue:(JSValue *)value;

This should be "managedValueWithValue" -- that's the idiomatic ObjC naming
scheme.

Not sure that we need this, but I guess it's OK to have it.

> Source/JavaScriptCore/API/JSManagedValue.mm:91
> +	   JSC::JSObject* object = toJS(JSValueToObject([value.context
globalContextRef], [value JSValueRef], 0));

You can cast to JSObjectRef here instead of calling JSValueToObject(), since
you've checked JSValueIsObject().