Bug 101647 – PDFPlugin should support clicking on external links

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

Links within the PDF work, external links (to other URLs) don't. I have a patch.

<rdar://problem/12555222>

[Tim Horton]

Created attachment 173126 [details]
patch

[Tim Horton]

Comment on attachment 173126 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173126&action=review

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:603
> +void PDFPlugin::loadURL(NSURL *url)

Maybe this isn't a good name.

[Alexey Proskuryakov]

Comment on attachment 173126 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173126&action=review

I'd like to know what happens with a javascript: URL.

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:603
>> +void PDFPlugin::loadURL(NSURL *url)
> 
> Maybe this isn't a good name.

Perhaps something based on delegate name would work better.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:611
> +    frame->loader()->loadFrameRequest(FrameLoadRequest(frame->document()->securityOrigin(), ResourceRequest(url, frame->loader()->outgoingReferrer())), false, false, coreEvent.get(), 0, MaybeSendReferrer);

I'm not sure if it's always correct behavior to navigate current frame. What did WebKit1 do?

[Tim Horton]

(In reply to comment #3)
> (From update of attachment 173126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173126&action=review
> 
> I'd like to know what happens with a javascript: URL.

It gets blocked, but I don't know how or why, so I'm going to investigate further.

> >> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:603
> >> +void PDFPlugin::loadURL(NSURL *url)
> > 
> > Maybe this isn't a good name.
> 
> Perhaps something based on delegate name would work better.

Sure!

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:611
> > +    frame->loader()->loadFrameRequest(FrameLoadRequest(frame->document()->securityOrigin(), ResourceRequest(url, frame->loader()->outgoingReferrer())), false, false, coreEvent.get(), 0, MaybeSendReferrer);
> 
> I'm not sure if it's always correct behavior to navigate current frame. What did WebKit1 do?

WebKit1's PDF view navigates the current frame (unless you command-click, etc.). So, same as this.

[Tim Horton]

Created attachment 173231 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 173231 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173231&action=review

Looks good to me, except for IPC that I don't understand the need for.

> Source/WebKit2/ChangeLog:16
> +        (WebKit::PDFPlugin::loadURL): Request a load given the new URL.

Please update the ChangeLog with new name.

> Source/WebKit2/PluginProcess/PluginControllerProxy.cpp:534
> +void PluginControllerProxy::shouldAllowScripting(bool& allowed)

I don't understand why WebProcess can't make this decision. Can you please explain the rationale?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:611
> +    frame->loader()->urlSelected(url, "", coreEvent.get(), false, false, MaybeSendReferrer);

You can use emptyString for target.

[Tim Horton]

Created attachment 173349 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 173349 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173349&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1641
> +    if (coreFrame->document() && coreFrame->document()->isPluginDocument()) {

No need to check coreFrame->document(), every frame has a document.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1643
> +        PluginView* pluginView = static_cast<PluginView*>(pluginDocument->pluginWidget());

This cast seems suspicious. If every return value of pluginWidget() were a PluginView, why doesn't this function just return that? Or perhaps we know because we are in WebKit2?

[Tim Horton]

Created attachment 173354 [details]
patch

[Tim Horton]

Created attachment 173372 [details]
pdf with some links in it

[Alexey Proskuryakov]

Comment on attachment 173354 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173354&action=review

> Source/WebKit2/PluginProcess/PluginControllerProxy.h:142
> +    void shouldAllowScripting(bool&);

Is this still needed?

> Source/WebKit2/WebProcess/Plugins/Plugin.h:197
> +    // Ask the plug-in whether it should be allowed to execute JavaScript or navigate to JavaScript URLs.

This comment seems a bit misleading, we have no intention to ask plug-ins about such things.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1647
> +            if (pluginView && !pluginView->shouldAllowScripting())

The first null check is redundant.

[Tim Horton]

(In reply to comment #11)
> (From update of attachment 173354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173354&action=review
> 
> > Source/WebKit2/PluginProcess/PluginControllerProxy.h:142
> > +    void shouldAllowScripting(bool&);
> 
> Is this still needed?

Negative.

> > Source/WebKit2/WebProcess/Plugins/Plugin.h:197
> > +    // Ask the plug-in whether it should be allowed to execute JavaScript or navigate to JavaScript URLs.
> 
> This comment seems a bit misleading, we have no intention to ask plug-ins about such things.

Hmm, technically we are asking the Plugin (NetscapePlugin and SimplePDFPlugin respond) these things.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1647
> > +            if (pluginView && !pluginView->shouldAllowScripting())
> 
> The first null check is redundant.

Sure.

Thanks!

[Tim Horton]

http://trac.webkit.org/changeset/134131