Bug 137182 - [iOS] Add basic support for link navigation in WKPDFView
Summary: [iOS] Add basic support for link navigation in WKPDFView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-09-27 14:44 PDT by Andy Estes
Modified: 2014-09-29 11:05 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.72 KB, patch)
2014-09-27 15:16 PDT, Andy Estes
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2014-09-27 14:44:42 PDT
[iOS] Add basic support for link navigation in WKPDFView
Comment 1 Andy Estes 2014-09-27 15:16:30 PDT
Created attachment 238791 [details]
Patch
Comment 2 Andy Estes 2014-09-27 15:17:21 PDT
rdar://problem/18334903
Comment 3 Tim Horton 2014-09-27 19:26:39 PDT
Comment on attachment 238791 [details]
Patch

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

Thank you!

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1108
> +    const int numberOfClicks = 1;

Ehh.
Comment 4 Andy Estes 2014-09-27 20:26:02 PDT
(In reply to comment #3)
> (From update of attachment 238791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238791&action=review
> 
> Thank you!
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1108
> > +    const int numberOfClicks = 1;
> 
> Ehh.

Well, it wasn't immediately obvious to me what the detail argument represented so I wanted to document the magic number. I don't mind removing it though.
Comment 5 Tim Horton 2014-09-27 20:38:55 PDT
Comment on attachment 238791 [details]
Patch

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

>>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1108
>>> +    const int numberOfClicks = 1;
>> 
>> Ehh.
> 
> Well, it wasn't immediately obvious to me what the detail argument represented so I wanted to document the magic number. I don't mind removing it though.

Ah, I didn't see the odd argument name. Leave it!
Comment 6 Alexey Proskuryakov 2014-09-28 11:34:22 PDT
Comment on attachment 238791 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:53
> +static const int64_t linkFollowingDelayInNS = 200000000; // .2 seconds

Can something from std::chrono be used here to avoid the annoying InNS suffix, as well as potential for mixing up the units?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:315
> +    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, linkFollowingDelayInNS), dispatch_get_main_queue(), ^ {

Does this move execution to a different thread? If it does, then there are thread safety issues here. Notably, implicit capturing of urlString creates a race between its destruction in original thread and in the main thread.

I don't understand the delay. Is it just to show highlight? It seems worth a comment.

>>>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1108
>>>> +    const int numberOfClicks = 1;
>>> 
>>> Ehh.
>> 
>> Well, it wasn't immediately obvious to me what the detail argument represented so I wanted to document the magic number. I don't mind removing it though.
> 
> Ah, I didn't see the odd argument name. Leave it!

Maybe "singleClick" or "simulateSingleClick" would be a better name? Even before seeing Tim's comment, I didn't like the name, but wasn't sure if it was just me.
Comment 7 Andy Estes 2014-09-29 00:47:14 PDT
(In reply to comment #6)
> (From update of attachment 238791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238791&action=review
> 
> > Source/WebKit2/UIProcess/ios/WKPDFView.mm:53
> > +static const int64_t linkFollowingDelayInNS = 200000000; // .2 seconds
> 
> Can something from std::chrono be used here to avoid the annoying InNS suffix, as well as potential for mixing up the units?

Yes, good idea. I'll use std::chrono::nanoseconds.

> 
> > Source/WebKit2/UIProcess/ios/WKPDFView.mm:315
> > +    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, linkFollowingDelayInNS), dispatch_get_main_queue(), ^ {
> 
> Does this move execution to a different thread? If it does, then there are thread safety issues here. Notably, implicit capturing of urlString creates a race between its destruction in original thread and in the main thread.

No, CorePDF delivers the delegate message on the main thread. I can add a ASSERT_CURRENT_DISPATCH_QUEUE_IS(dispatch_get_main_queue()).

> 
> I don't understand the delay. Is it just to show highlight? It seems worth a comment.
> 

Yes, it's to show a highlight. I'll add a comment.

> >>>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1108
> >>>> +    const int numberOfClicks = 1;
> >>> 
> >>> Ehh.
> >> 
> >> Well, it wasn't immediately obvious to me what the detail argument represented so I wanted to document the magic number. I don't mind removing it though.
> > 
> > Ah, I didn't see the odd argument name. Leave it!
> 
> Maybe "singleClick" or "simulateSingleClick" would be a better name? Even before seeing Tim's comment, I didn't like the name, but wasn't sure if it was just me.

Ok, I think singleClick is good.

Thanks for the feedback!
Comment 8 Andy Estes 2014-09-29 01:40:27 PDT
(In reply to comment #7)
> 
> No, CorePDF delivers the delegate message on the main thread. I can add a ASSERT_CURRENT_DISPATCH_QUEUE_IS(dispatch_get_main_queue()).

Oh, we don't have ASSERT_CURRENT_DISPATCH_QUEUE_IS(), but I guess I just need ASSERT(isMainThread()).
Comment 9 Andy Estes 2014-09-29 11:05:44 PDT
Committed r174072: <http://trac.webkit.org/changeset/174072>