Bug 57528

Summary: WebKit2: Link from PDF opens in a new tab instead of in the same tab
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: PDFAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
URL: http://arxiv.org/pdf/1008.1209
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Jer Noble 2011-03-31 00:29:08 PDT
The link opens in a new tab. It should open in the same tab, replacing the PDF.
Comment 1 Jer Noble 2011-03-31 00:29:31 PDT
<rdar://problem/8812757>
Comment 2 Jer Noble 2011-03-31 00:35:53 PDT
Created attachment 87675 [details]
Patch
Comment 3 Darin Adler 2011-03-31 07:42:34 PDT
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.
Comment 4 Jer Noble 2011-03-31 09:51:18 PDT
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.
Comment 5 Jer Noble 2011-03-31 09:56:10 PDT
Aww heck, I'll just do it here. :)
Comment 6 Jer Noble 2011-03-31 17:48:46 PDT
Created attachment 87811 [details]
Patch

With this patch, command-click, option-click, and shift-click now all work as intended with links in PDFs.
Comment 7 Early Warning System Bot 2011-03-31 17:57:47 PDT
Attachment 87811 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8320004
Comment 8 Build Bot 2011-03-31 18:12:08 PDT
Attachment 87811 [details] did not build on win:
Build output: http://queues.webkit.org/results/8307019
Comment 9 Jer Noble 2011-03-31 23:23:19 PDT
Looks like the definition of MouseEvent::create changed out from under me.  I'll fix and re-upload.
Comment 10 Jer Noble 2011-04-01 01:12:55 PDT
Created attachment 87832 [details]
Patch
Comment 11 Early Warning System Bot 2011-04-01 01:24:41 PDT
Attachment 87832 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8307098
Comment 12 Build Bot 2011-04-01 01:52:42 PDT
Attachment 87832 [details] did not build on win:
Build output: http://queues.webkit.org/results/8185127
Comment 13 WebKit Review Bot 2011-04-01 04:54:12 PDT
Attachment 87832 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8307132
Comment 14 Jer Noble 2011-04-01 08:27:08 PDT
Created attachment 87860 [details]
Patch

Forgot to update WebCore.exp.in after rebaselining.
Comment 15 Build Bot 2011-04-01 08:50:35 PDT
Attachment 87860 [details] did not build on win:
Build output: http://queues.webkit.org/results/8318212
Comment 16 Early Warning System Bot 2011-04-01 08:53:18 PDT
Attachment 87860 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8319197
Comment 17 Jer Noble 2011-04-01 09:57:10 PDT
Created attachment 87873 [details]
Patch

Uploaded without committing the changes to use the new MouseEvent::create() definition.
Comment 18 Darin Adler 2011-04-01 09:57:36 PDT
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.
> +
> +2011-03-31  Jer Noble  <jer.noble@apple.com>
> +
> +        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.
> +
> +2011-03-31  Jer Noble  <jer.noble@apple.com>

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.
Comment 19 WebKit Review Bot 2011-04-01 10:21:13 PDT
Attachment 87860 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8321219
Comment 20 Jer Noble 2011-04-01 10:24:27 PDT
(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.
> > +
> > +2011-03-31  Jer Noble  <jer.noble@apple.com>
> > +
> > +        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.
> > +
> > +2011-03-31  Jer Noble  <jer.noble@apple.com>
> 
> 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. :)
Comment 21 Jer Noble 2011-04-01 10:59:39 PDT
(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.
Comment 22 Jer Noble 2011-04-01 12:04:00 PDT
(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.
Comment 23 Jer Noble 2011-04-01 12:17:15 PDT
Created attachment 87895 [details]
Patch

Addressed most of Darin's comments.
Comment 24 Darin Adler 2011-04-01 12:20:05 PDT
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.
Comment 25 Build Bot 2011-04-01 12:44:06 PDT
Attachment 87895 [details] did not build on win:
Build output: http://queues.webkit.org/results/8318271
Comment 26 Jer Noble 2011-04-01 15:54:10 PDT
Committed r82733: <http://trac.webkit.org/changeset/82733>