[webkit-reviews] review granted: [Bug 123586] Add a WKRemoteObject class : [Attachment 215669] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 123586: Add a WKRemoteObject class
https://bugs.webkit.org/show_bug.cgi?id=123586

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

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=215669&action=review


> Source/WebKit2/Shared/API/Cocoa/WKRemoteObject.h:26
> +#import <WebKit2/WKFoundation.h>

I prefer that we use ""-style imports in internal headers such as this one.

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObject.mm:33
> +#if WK_API_ENABLED

I normally put this between the #import blocks.

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObject.mm:54
> +	   return true;

YES

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObject.mm:56
> +    return protocol_conformsToProtocol([_interface protocol], protocol);

I’d write the whole thing in a single line:
    return [super conformsToProtocol:protocol] ||
protocol_conformsToProtocol([_interface protocol], protocol);

Is this really the optimal order to check these conditions?

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObject.mm:94
> +

I would expect to see overrides of -respondsToSelector:, -isKindOfClass: and
-isMemberOfClass: as well.

> Source/WebKit2/Shared/API/Cocoa/WKRemoteObjectRegistry.mm:68
> +    return [remoteObject.leakRef() autorelease];

This entire method is not thread-safe :-(