Bug 137182 – [iOS] Add basic support for link navigation in WKPDFView

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[iOS] Add basic support for link navigation in WKPDFView

[Andy Estes]

Created attachment 238791 [details]
Patch

[Andy Estes]

rdar://problem/18334903

[Tim Horton]

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.

[Andy Estes]

(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.

[Tim Horton]

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!

[Alexey Proskuryakov]

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.

[Andy Estes]

(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!

[Andy Estes]

(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()).

[Andy Estes]

Committed r174072: <http://trac.webkit.org/changeset/174072>