[webkit-reviews] review denied: [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames : [Attachment 78308] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 36721: Return correct load result to NPAPI plugins calling getUrlNotify on
target frames
https://bugs.webkit.org/show_bug.cgi?id=36721

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

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

Thanks for getting rid of searching by URL! I think that this is getting pretty
close.

> Source/WebCore/loader/FrameLoader.cpp:1355
> +    if (m_pluginToNotify)
> +	   notifyPlugin(ResourceError(errorDomainWebKitInternal, 0,
request.url().string(), ""));

Is request.url() the correct URL to report here? Also, this looks like it
should use FrameLoaderClient::cancelledError().

Is this code path tested?

> Source/WebCore/loader/FrameLoader.cpp:1395
>      Frame* frame = findFrameForNavigation(frameName);
>      if (frame) {
> -	   frame->loader()->load(request, lockHistory);
> +	   frame->loader()->load(request, lockHistory, m_pluginToNotify,
m_pluginRequest);

What clears these member variables in the current frame?

> Source/WebCore/loader/FrameLoader.cpp:2439
> +    if (!m_pluginToNotify)
> +	   return;

I'd assert that m_pluginRequest is null here.

> Source/WebCore/loader/FrameLoader.cpp:3036
> +	   notifyPlugin(ResourceError(errorDomainWebKitInternal, 0,
request.url().string(), ""));

Maybe FrameLoaderClient::interruptForPolicyChangeError()?