[webkit-reviews] review denied: [Bug 123380] JSExport doesn't support constructors : [Attachment 215235] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Oct 26 16:20:41 PDT 2013
Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 123380: JSExport doesn't support constructors
https://bugs.webkit.org/show_bug.cgi?id=123380
Attachment 215235: Patch
https://bugs.webkit.org/attachment.cgi?id=215235&action=review
------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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-Transitioni
ngToARC/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."
More information about the webkit-reviews
mailing list