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

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


r=me

> Source/JavaScriptCore/API/JSContextInternal.h:75
> ++ (JSContext
*)contextFromGlobalContextRef:(JSGlobalContextRef)globalContext;

Minor naming comment: Usually these class factory methods have the same name as
their related designated initializer. In this case, that's
"contextWithGlobalContextRef".

> Source/JavaScriptCore/API/JSObjCWrapperCache.mm:38
> +    DEFINE_STATIC_LOCAL(Mutex, mutex, ());

This initialization is not thread-safe. To my shock and horror, there are 17
other bugs like this in WebKit. I guess threads are indeed evil. I guess I will
not make you fix this, since our whole library is built on this shaky
foundation, and we'll have to fix it later some other way.

> Source/JavaScriptCore/API/JSObjCWrapperCache.mm:65
> +    return static_cast<id>(NSMapGet(wrapperCache, object));

This should autorelease to keep up the contract that our +factory methods
return autoreleased values.

> Source/JavaScriptCore/API/JSVirtualMachineInternal.h:38
> +JS_EXPORT_PRIVATE JSVirtualMachine
*virtualMachineFromContextGroupRef(JSContextGroupRef);

Let's use the class factory method pattern here like you did in JSContext. It's
the Cocoa way.