Bug 57528 – WebKit2: Link from PDF opens in a new tab instead of in the same tab

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Jer Noble]

The link opens in a new tab. It should open in the same tab, replacing the PDF.

[Jer Noble]

<rdar://problem/8812757>

[Jer Noble]

Created attachment 87675 [details]
Patch

[Darin Adler]

Comment on attachment 87675 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:273
> +    if (!URL)
> +        return;

Is this really needed? Does it get called with a null URL?

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:276
> +    String urlString = [URL absoluteString];
> +    _pdfViewController->page()->loadURL(urlString);

No need for a local variable here.

Do we really just want to call loadURL on the page? What about modifier keys and the policy delegate and such? We certainly don’t just call loadURL when you click on a link in a webpage.

If you look at the code in WebKit1, you’ll see that it turns the current event into a DOM event and sends that event in to the loader.

[Jer Noble]

Comment on attachment 87675 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:273
>> +        return;
> 
> Is this really needed? Does it get called with a null URL?

The documentation for PDFView doesn't say, but I figured since it was there in the WebKit1 code, it was probably there for a reason.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:276
>> +    _pdfViewController->page()->loadURL(urlString);
> 
> No need for a local variable here.
> 
> Do we really just want to call loadURL on the page? What about modifier keys and the policy delegate and such? We certainly don’t just call loadURL when you click on a link in a webpage.
> 
> If you look at the code in WebKit1, you’ll see that it turns the current event into a DOM event and sends that event in to the loader.

Local variable collapsed.

I found no way to pass modifier keys through the UIProcess/WebProcess boundary during a load request.  In the interests of getting the basic fix done and in, I was going to file a bug to add the additional event-handling functionality.

[Jer Noble]

Aww heck, I'll just do it here. :)

[Jer Noble]

Created attachment 87811 [details]
Patch

With this patch, command-click, option-click, and shift-click now all work as intended with links in PDFs.

[Early Warning System Bot]

Attachment 87811 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8320004

[Build Bot]

Attachment 87811 [details] did not build on win:
Build output: http://queues.webkit.org/results/8307019

[Jer Noble]

Looks like the definition of MouseEvent::create changed out from under me.  I'll fix and re-upload.

[Jer Noble]

Created attachment 87832 [details]
Patch

[Early Warning System Bot]

Attachment 87832 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8307098

[Build Bot]

Attachment 87832 [details] did not build on win:
Build output: http://queues.webkit.org/results/8185127

[WebKit Review Bot]

Attachment 87832 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8307132

[Jer Noble]

Created attachment 87860 [details]
Patch

Forgot to update WebCore.exp.in after rebaselining.

[Build Bot]

Attachment 87860 [details] did not build on win:
Build output: http://queues.webkit.org/results/8318212

[Early Warning System Bot]

Attachment 87860 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8319197

[Jer Noble]

Created attachment 87873 [details]
Patch

Uploaded without committing the changes to use the new MouseEvent::create() definition.

[Darin Adler]

Comment on attachment 87832 [details]
Patch

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

> Source/WebKit2/ChangeLog:33
>          Reviewed by NOBODY (OOPS!).
>  
> +        WebKit2: Link from PDF opens in a new tab instead of in the same tab
> +        https://bugs.webkit.org/show_bug.cgi?id=57528
> +
> +        * UIProcess/API/mac/PDFViewController.mm:
> +        (-[WKPDFView PDFViewWillClickOnLink:withURL:]): Handle the delegate
> +            function and override the PDFView default behavior.
> +
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WebKit2: Link from PDF opens in a new tab instead of in the same tab
> +        https://bugs.webkit.org/show_bug.cgi?id=57528
> +
> +        Notify the WebProcess that a link has been clicked so that the normal policy
> +        lookup can occur.
> +
> +        * UIProcess/API/mac/PDFViewController.mm:
> +        (-[WKPDFView PDFViewWillClickOnLink:withURL:]): Handle the delegate
> +            function and override the PDFView default behavior.
> +        * UIProcess/WebPageProxy.cpp:
> +        (WebKit::WebPageProxy::urlSelected): Added.  Send event through to WebPage.
> +        * UIProcess/WebPageProxy.h:
> +        * WebProcess/WebPage/WebPage.cpp:
> +        (WebKit::WebPage::urlSelected): Added.  Call loadFramerequest().
> +        * WebProcess/WebPage/WebPage.h:
> +        * WebProcess/WebPage/WebPage.messages.in: Added URLSelected.
> +
> [phone]  Jer Noble  <[e-mail]>

This is a double change log entry. Please fix that.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:288
> +    switch ([nsEvent type]) {
> +        case NSLeftMouseUp:
> +        case NSRightMouseUp:
> +        case NSOtherMouseUp:
> +            event = WebEventFactory::createWebMouseEvent(nsEvent, sender);
> +    }

WebKit formatting rules say case should not be indented.

Is it correct to leave the event as null when some other event is current? Could you add a brief comment that makes it clear that is intentional and says why it is correct?

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:290
> +    _pdfViewController->page()->urlSelected(_pdfViewController->page()->mainFrame(), [URL absoluteString], event);

I could see making this a function on the PDF view controller object rather than calling directly through to the page. Not a lot better, but perhaps slightly cleaner, to keep this class just doing type conversions and having the controller contain the actual logic.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2811
> +void WebPageProxy::urlSelected(WebFrameProxy* frame, const String& url, const WebMouseEvent& event)

Since we only need to do this for the main frame, I suggest not including a frame pointer and implementing it only for the main frame. We can always add a more powerful version that can work on subframes later if we find we need it. I suspect we won’t.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:447
> +void WebPage::urlSelected(uint64_t frameID, WTF::String url, WebKit::WebMouseEvent event)

Should be const String& and const WebMouseEvent&, not WTF::String and WebKit::WebMouseEvent (see comments below).

> Source/WebKit2/WebProcess/WebPage/WebPage.h:406
> +    void urlSelected(uint64_t frameID, WTF::String url, WebKit::WebMouseEvent);

Other functions that take strings say const String& rather than WTF::String. Could you fix that?

Since this class is inside the WebKit namespace, the class name should just be WebMouseEvent, not WebKit::WebMouseEvent. And it can be const WebMouseEvent&.

Not your fault, but I really hate the name “URL selected”. I would much prefer something like “link clicked” but I suppose we should stay consistent with WebCore.

[WebKit Review Bot]

Attachment 87860 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8321219

[Jer Noble]

(In reply to comment #18)
> (From update of attachment 87832 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87832&action=review
> 
> > Source/WebKit2/ChangeLog:33
> >          Reviewed by NOBODY (OOPS!).
> >  
> > +        WebKit2: Link from PDF opens in a new tab instead of in the same tab
> > +        https://bugs.webkit.org/show_bug.cgi?id=57528
> > +
> > +        * UIProcess/API/mac/PDFViewController.mm:
> > +        (-[WKPDFView PDFViewWillClickOnLink:withURL:]): Handle the delegate
> > +            function and override the PDFView default behavior.
> > +
> > [phone]  Jer Noble  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        WebKit2: Link from PDF opens in a new tab instead of in the same tab
> > +        https://bugs.webkit.org/show_bug.cgi?id=57528
> > +
> > +        Notify the WebProcess that a link has been clicked so that the normal policy
> > +        lookup can occur.
> > +
> > +        * UIProcess/API/mac/PDFViewController.mm:
> > +        (-[WKPDFView PDFViewWillClickOnLink:withURL:]): Handle the delegate
> > +            function and override the PDFView default behavior.
> > +        * UIProcess/WebPageProxy.cpp:
> > +        (WebKit::WebPageProxy::urlSelected): Added.  Send event through to WebPage.
> > +        * UIProcess/WebPageProxy.h:
> > +        * WebProcess/WebPage/WebPage.cpp:
> > +        (WebKit::WebPage::urlSelected): Added.  Call loadFramerequest().
> > +        * WebProcess/WebPage/WebPage.h:
> > +        * WebProcess/WebPage/WebPage.messages.in: Added URLSelected.
> > +
> > [phone]  Jer Noble  <[e-mail]>
> 
> This is a double change log entry. Please fix that.

Sure thing. 

> > Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:288
> > +    switch ([nsEvent type]) {
> > +        case NSLeftMouseUp:
> > +        case NSRightMouseUp:
> > +        case NSOtherMouseUp:
> > +            event = WebEventFactory::createWebMouseEvent(nsEvent, sender);
> > +    }
> 
> WebKit formatting rules say case should not be indented.

Whoops!  (I wonder if I could override Xcode's formatting rules for switch statements; it'd save me lots of headaches.)

> Is it correct to leave the event as null when some other event is current? Could you add a brief comment that makes it clear that is intentional and says why it is correct?

The WebMouseEvent isn't really null, per se.  It's a stack object, so it's initialized but... Wow, WebMouseEvent's default constructor doesn't initialize any of its ivars.  Yeah, totally not safe.  I'll fix this.

> > Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:290
> > +    _pdfViewController->page()->urlSelected(_pdfViewController->page()->mainFrame(), [URL absoluteString], event);
> 
> I could see making this a function on the PDF view controller object rather than calling directly through to the page. Not a lot better, but perhaps slightly cleaner, to keep this class just doing type conversions and having the controller contain the actual logic.

Oh so send a message to the WebPage directly from the PDF controller instead of from WebPageProxy?  Maybe I'm not understanding which 

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2811
> > +void WebPageProxy::urlSelected(WebFrameProxy* frame, const String& url, const WebMouseEvent& event)
> 
> Since we only need to do this for the main frame, I suggest not including a frame pointer and implementing it only for the main frame. We can always add a more powerful version that can work on subframes later if we find we need it. I suspect we won’t.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:447
> > +void WebPage::urlSelected(uint64_t frameID, WTF::String url, WebKit::WebMouseEvent event)
> 
> Should be const String& and const WebMouseEvent&, not WTF::String and WebKit::WebMouseEvent (see comments below).
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:406
> > +    void urlSelected(uint64_t frameID, WTF::String url, WebKit::WebMouseEvent);
> 
> Other functions that take strings say const String& rather than WTF::String. Could you fix that?

Whoops, yes fixed.

> Since this class is inside the WebKit namespace, the class name should just be WebMouseEvent, not WebKit::WebMouseEvent. And it can be const WebMouseEvent&.
> 
> Not your fault, but I really hate the name “URL selected”. I would much prefer something like “link clicked” but I suppose we should stay consistent with WebCore.

The future is our oyster!  Lets change it. :)

[Jer Noble]

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 87832 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87832&action=review
> > 
> > 
> > I could see making this a function on the PDF view controller object rather than calling directly through to the page. Not a lot better, but perhaps slightly cleaner, to keep this class just doing type conversions and having the controller contain the actual logic.
> 
> Oh so send a message to the WebPage directly from the PDF controller instead of from WebPageProxy?  Maybe I'm not understanding which 

I should really learn to complete my

...which logic you'd like moved into the controller?

> > > Source/WebKit2/UIProcess/WebPageProxy.cpp:2811
> > > +void WebPageProxy::urlSelected(WebFrameProxy* frame, const String& url, const WebMouseEvent& event)
> > 
> > Since we only need to do this for the main frame, I suggest not including a frame pointer and implementing it only for the main frame. We can always add a more powerful version that can work on subframes later if we find we need it. I suspect we won’t.

Sure thing.

[Jer Noble]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > (From update of attachment 87832 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=87832&action=review
> > > 
> > > 
> > > I could see making this a function on the PDF view controller object rather than calling directly through to the page. Not a lot better, but perhaps slightly cleaner, to keep this class just doing type conversions and having the controller contain the actual logic.
> > 
> > Oh so send a message to the WebPage directly from the PDF controller instead of from WebPageProxy?  Maybe I'm not understanding which 
> 
> I should really learn to complete my
> 
> ...which logic you'd like moved into the controller?

Oh wait, I see what you're saying.  I'll change it.

[Jer Noble]

Created attachment 87895 [details]
Patch

Addressed most of Darin's comments.

[Darin Adler]

Comment on attachment 87895 [details]
Patch

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

Seems OK. Might want to have Sam or Anders look at the “event with no type” concept to be sure we want it.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:522
> +        case NSLeftMouseUp:
> +        case NSRightMouseUp:
> +        case NSOtherMouseUp:
> +            event = WebEventFactory::createWebMouseEvent(nsEvent, m_pdfView);

WebKit coding style lines the case up with the switch.

I’d like to see a “why” comment explaining why it’s appropriate to pass an empty event if the current event is not a mouse up event.

[Build Bot]

Attachment 87895 [details] did not build on win:
Build output: http://queues.webkit.org/results/8318271

[Jer Noble]

Committed r82733: <http://trac.webkit.org/changeset/82733>