[webkit-reviews] review granted: [Bug 106059] Objective-C API: Need a way to use the Objective-C JavaScript API with WebKit : [Attachment 189378] 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 106059: Objective-C API: Need a way to use the Objective-C JavaScript API
with WebKit
https://bugs.webkit.org/show_bug.cgi?id=106059

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

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


r=me, with some comments

> Source/JavaScriptCore/API/JSContext.mm:174
> +- (id)initWithGlobalContextRef:(JSGlobalContextRef)context

I think we should name this "initWithJSGlobalContextRef" or name the other
thingy "globalContextRef". But they should be consistent.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:62
> +    if (!wrapperCache)
> +	   initWrapperCache();

Would be nice to have a wrapperCache() helper function that did the null check
for us.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:87
> +    // The extra JSContextGroupRetain is balanced here.

I think you mean that the JSContextGroupCreate is balanced here.

> Tools/TestWebKitAPI/Tests/mac/WebViewDidCreateJavaScriptContext.mm:107
> +    __block JSContext *tempContext = context;
> +    context[@"insertMyCustomProperty"] = ^(JSValue *element) {
> +	   JSValue *fortyTwo = [JSValue valueWithInt32:42
inContext:tempContext];

This is a memory leak because the block retains the context, which has a
property retaining the block. The right way to get a context inside a block
callback is [JSContext currentContext].