Support for constructor-style callbacks for the Objective-C API to JSC is currently limited to Objective-C blocks. Any clients who try to call the constructor of a JSExport-ed Objective-C class are met with a type error stating that it cannot be called as a constructor. It would be nice to expand JSExport's functionality to support this idiom. It is a natural extension to JSExport and would increase the expressiveness and simplicity in both Objective-C and JavaScript client code. The way we'll do this is to expand the capabilities of ObjCCallbackFunction and associated classes. Instead of constructing a normal C API object for the constructor, we'll instead allocate a full-blown ObjCCallbackFunction object which can already properly handle being invoked as a constructor.
Created attachment 215235 [details] Patch
Comment on attachment 215235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215235&action=review > Source/JavaScriptCore/API/JSWrapperMap.mm:167 > + // Don't copy over init* methods because we handle those specially Let's say "init family" instead of "init*", since there's a term of art for it. > Source/JavaScriptCore/API/JSWrapperMap.mm:341 > + __block HashMap<String, Protocol *> initTable; This is a dangerous use of __block with C++. If any of our blocks are copied, they will copy initTable, instead of sharing it: ----- <https://developer.apple.com/library/ios/documentation/cocoa/conceptual/Blocks/Articles/bxVariables.html> C++ Objects In general you can use C++ objects within a block. Within a member function, references to member variables and functions are via an implicitly imported this pointer and thus appear mutable. There are two considerations that apply if a block is copied: If you have a __block storage class for what would have been a stack-based C++ object, then the usual copy constructor is used. If you use any other C++ stack-based object from within a block, it must have a const copy constructor. The C++ object is then copied using that constructor. ----- I guess this usage ends up succeeding because forEachProtocolImplementingProtocol doesn't copy the passed-in block. I wonder what the best practice is in a case like this. > Source/JavaScriptCore/API/JSWrapperMap.mm:344 > + forEachProtocolImplementingProtocol(cls, exportProtocol, ^(Protocol *protocol){ > + forEachMethodInProtocol(protocol, YES, YES, ^(SEL selector, const char*){ Need spaces before "{". > Source/JavaScriptCore/API/JSWrapperMap.mm:377 > + // FIXME: Do we need to log here? > + NSLog(@"Too many inits found, abandoning ship."); Let's log, but try to write something that will be meaningful to an app developer: "ERROR: Class @{class} exported more than one init family method through JSExport. Class @{class} will not have a callable constructor function." > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:589 > [m_invocation invoke]; This is a bug in the init case because it doesn't account for init releasing the allocated object and returning a different object in its place (or nil). You should fix this and add a test for it. c.f. <https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html>. > Source/JavaScriptCore/API/ObjCCallbackFunction.mm:591 > + // After calling init we need to autorelease. This comment could say more: "Balance our call to -alloc with a call to -autorelease. We have to do this after calling -init because init family methods are allowed to release the allocated object and return something else in its place."
> This is a dangerous use of __block with C++. If any of our blocks are copied, they will copy initTable, instead of sharing it: Actually, that's only half true: block copy will copy initTable, but all blocks will point to the copy, so I guess we're cool.
Created attachment 215345 [details] Patch
Comment on attachment 215345 [details] Patch Should we add some linked-on-or-after action here?
(In reply to comment #5) > (From update of attachment 215345 [details]) > Should we add some linked-on-or-after action here? Sure. Right now it seems like the shared VM stuff uses the webkit version to decide when it was linked. I'm not sure which WebKit version to use...
Committed r158286: <http://trac.webkit.org/changeset/158286>
<rdar://problem/15147348>
Reopening because I forgot part of the linked-on-or-after stuff.
Created attachment 215536 [details] Patch
Comment on attachment 215536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215536&action=review > Source/JavaScriptCore/API/JSWrapperMap.mm:162 > +static bool shouldSkipMethodWithName(NSString *name) inline? > Source/JavaScriptCore/API/JSWrapperMap.mm:169 > +#if OS(DARWIN) > + if (NSVersionOfLinkTimeLibrary("JavaScriptCore") < webkitFirstVersionWithInitConstructorSupport) > + return false; > +#endif Is this fast enough to re-evaluate every time, or should we cache it in a global? > Source/JavaScriptCore/API/JSWrapperMap.mm:173 > + // Skip over init family methods because we handle those specially > + // for the purposes of hooking up the constructor correctly. > + return [name hasPrefix:@"init"]; Is this the correct definition for “init family”? It would return true for a property named “initials” or “initialCharacter” or “initialSize”.
Created attachment 216193 [details] Patch
(In reply to comment #12) > Created an attachment (id=216193) [details] > Patch I changed the patch significantly enough to warrant another review I think.
Comment on attachment 216193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216193&action=review r=me > Source/JavaScriptCore/API/JSWrapperMap.mm:623 > +#if OS(DARWIN) I think it's fair to assume that all ObjC OS's are DARWIN. So, you can remove this #ifdef.
Committed r158762: <http://trac.webkit.org/changeset/158762>