Summary: | [iOS] Add basic support for link navigation in WKPDFView | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Estes <aestes> | ||||
Component: | New Bugs | Assignee: | Andy Estes <aestes> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, thorton | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Andy Estes
2014-09-27 14:44:42 PDT
Created attachment 238791 [details]
Patch
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> |