[webkit-reviews] review granted: [Bug 111088] Objective-C API: Need a good way to reference event handlers without causing cycles : [Attachment 191337] style fixes

[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 191337: style fixes
https://bugs.webkit.org/attachment.cgi?id=191337&action=review

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


Please address comments before landing.

> Source/JavaScriptCore/API/JSAPIWrapperObject.cpp:56
> +    void* objcObject = thisObject->getPrivate();

Let's use a direct data member for the objcObject, and then we don't need the
cast to JSCallbackObject.

> Source/JavaScriptCore/API/JSContext.mm:78
>      return self;

Would be nice to call initWithGlobalContextRef instead of duplicating this
code. That can be a follow-up patch.

> Source/JavaScriptCore/API/JSContext.mm:271
> +- (void)garbageCollect
> +{
> +    JSC::APIEntryShim entryShim(toJS(m_context));
> +    toJS(m_context)->globalData().heap.collectAllGarbage();
> +}

I'm worried, even though this is SPI with a comment about debugging, that folks
will find this symbol and use it to bad effect. Let's link our test code
directly against the C++ API, and call that instead.

> Source/JavaScriptCore/API/JSManagedValue.h:39
> + at interface JSManagedValue : JSWeakValue

I'm concerned about adding API surface area here, when we don't know of clients
that want that raw weak behavior. Let's leave out JSWeakValue. We can always
add it later, as a subclass of JSManagedValue or a sibling class.

> Source/JavaScriptCore/API/JSManagedValue.h:42
> +- (id)initWithValue:(JSValue *)value owner:(id)owner;
> ++ (JSManagedValue *)managedValueWithValue:(JSValue *)value owner:(id)owner;

Class methods should be sorted first.

> Source/JavaScriptCore/API/JSManagedValue.mm:43
> +    virtual ~JSManagedValueHandleOwner();

No need for a dtor here, since we don't do anything it.

> Source/JavaScriptCore/API/JSManagedValue.mm:61
> +- (id)init

Since we don't export API for this, I don't think we should have it. There's no
good way for us to test if this works.

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

We don't want to call JSValueToObject here because that will perform a
conversion, possibly allocating a new object. Let's call JSValueIsObject
instead, and be null if not object.

> Source/JavaScriptCore/API/JSObjectRef.cpp:350
> +    if (jsObject->inherits(&JSCallbackObject<JSAPIWrapperObject>::s_info))
> +	   return
jsCast<JSCallbackObject<JSAPIWrapperObject>*>(jsObject)->getPrivate();

Ultimately, I think it would be nice to remove the dependency that requires our
automatic wrappers to be JSCallbackObjects. That can be a follow-up patch.

> Source/JavaScriptCore/API/JSWeakValue.h:41
> ++ (id)weakValueWithValue:(JSValue *)value;

Class methods should be sorted first.

> Source/JavaScriptCore/API/JSWrapperMap.mm:120
> +static JSObjectRef JSObjectMakeWrapper(JSContextRef ctx, JSClassRef jsClass,
void* data)

Let's not use the "JSObject" prefix for this function, since that implies that
it belongs to the C API. Let's just call this "makeWrapper".