[webkit-reviews] review granted: [Bug 103846] Add ObjC API for setting initialization data for the WKWebProcessPlugIn : [Attachment 177163] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 103846: Add ObjC API for setting initialization data for the
WKWebProcessPlugIn
https://bugs.webkit.org/show_bug.cgi?id=103846

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177163&action=review


> Source/WebKit2/UIProcess/API/mac/WKProcessGroup.mm:76
> +    if ([processGroup.delegate
respondsToSelector:@selector(processGroupWillCreateConnectionToWebProcessPlugIn
:)]) {

I suggest using early return here instead of nesting everything inside an if
statement.

> Source/WebKit2/UIProcess/API/mac/WKProcessGroup.mm:78
> +	   RetainPtr<id> initializationUserData = [processGroup.delegate
processGroupWillCreateConnectionToWebProcessPlugIn:processGroup];
> +	   RefPtr<WebKit::ObjCObjectGraph> wkMessageBody =
WebKit::ObjCObjectGraph::create(initializationUserData.get());

I don’t think we need a RetainPtr for the return value from a method that is
just passed on to another function and then dropped. I suggest either using the
type "id" here instead, or not using a local variable at all.

Also, why is the message body called wkMessageBody instead of just body or
messageBody?

> Source/WebKit2/UIProcess/API/mac/WKProcessGroup.mm:79
> +	   return (WKTypeRef)wkMessageBody.release().leakRef();

Can we come up with some type checked cast for use in cases like this rather
than using a straight C cast?

> Source/WebKit2/UIProcess/API/mac/WKProcessGroup.mm:85
> +static void setUpInectedBundleClient(WKProcessGroup *processGroup,
WKContextRef contextRef)

Typo here: "inected".

> Source/WebKit2/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:114
> +	   RetainPtr<id> objCInitializationUserData;
> +	   if (initializationUserData && initializationUserData->type() ==
APIObject::TypeObjCObjectGraph)
> +	       objCInitializationUserData =
static_cast<ObjCObjectGraph*>(initializationUserData)->rootObject();
> +	   [instance webProcessPlugIn:[WKWebProcessPlugInController _shared]
initializeWithObject:objCInitializationUserData.get()];

Use of RetainPtr is OK here if the return type of rootObject is a RetainPtr.
But if not, then I suggest just using "id" rather than "RetainPtr<id>".