[iOS] Add basic support for link navigation in WKPDFView
Created attachment 238791 [details] Patch
rdar://problem/18334903
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.
(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 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 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.
(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!
(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()).
Committed r174072: <http://trac.webkit.org/changeset/174072>