[webkit-reviews] review granted: [Bug 32982] plugins/get-url-with-iframe-target.html fails on SnowLeopard (64-bit) : [Attachment 45548] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 32982: plugins/get-url-with-iframe-target.html fails on SnowLeopard
(64-bit)
https://bugs.webkit.org/show_bug.cgi?id=32982

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +    void webFrameDidFinishLoadWithReason(WebFrame*, NPReason);

Style nit: should be "WebFrame *". I'm not sure if the changes in your local
copy are in right or wrong direction - ObjC classes should have detached stars.


> -    NetscapePluginInstanceProxy(NetscapePluginHostProxy*,
WebHostedNetscapePluginView *, bool fullFramePlugin);
> +    NetscapePluginInstanceProxy(NetscapePluginHostProxy*,
WebHostedNetscapePluginView*, bool fullFramePlugin);

Same issue (old code was correct).

> -    NPError loadRequest(NSURLRequest *, const char* cTarget, bool
currentEventIsUserGesture, uint32_t& streamID);
> +    NPError loadRequest(NSURLRequest*, const char* cTarget, bool
currentEventIsUserGesture, uint32_t& streamID);

Same issue.

> +    FrameLoadMap  m_pendingFrameLoads;

Two spaces instead of one.

> +	// Check if another plug-in view or even this view is waiting for the
frame to load.
> +	   // If it is, tell it that the load was cancelled because it will be
anyway.

Tab here.

> +void NetscapePluginInstanceProxy::webFrameDidFinishLoadWithReason(WebFrame*
webFrame, NPReason reason)

Misplaced star again (I probably missed some instances anyway).

> +    RefPtr<PluginRequest> pluginRequest = m_pendingFrameLoads.get(webFrame);


Why does this need to be a RefPtr?

> -	   PluginRequest* pluginRequest = new PluginRequest(requestID, request,
target, allowPopups);
> +	   RefPtr<PluginRequest> pluginRequest =
PluginRequest::create(requestID, request, target, allowPopups);
>	   m_pluginRequests.append(pluginRequest);

This is probably not performance critical code, but I think that a release()
would prevent refcount thrash.

r=me with style fixes.