[iOS] Add long press support for links in WKPDFView
rdar://problem/18334903
Created attachment 240928 [details] Patch
Attachment 240928 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:353: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240928&action=review > Source/WebKit2/ChangeLog:25 > + (-[WKPDFView updatePositionInformation]): Added an empty implementation. Perhaps this delegate method should be optional? > Source/WebKit2/ChangeLog:30 > + (-[WKPDFView startInteractionWithElement:]): Added an empty implementation. > + (-[WKPDFView stopInteraction]): Ditto. Perhaps these delegate method should be optional? > Source/WebKit2/UIProcess/ios/WKPDFView.mm:343 > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, duration * NSEC_PER_SEC), dispatch_get_main_queue(), ^ { Extra space between ^ and { > Source/WebKit2/UIProcess/ios/WKPDFView.mm:359 > + return [NSURL URLWithString:anchorString relativeToURL:documentURL]; You can use +_web_URLWithWTFString:relativeToURL: here. > Source/WebKit2/UIProcess/ios/WKPDFView.mm:-363 > - ASSERT(isMainThread()); Why? > Source/WebKit2/UIProcess/ios/WKPDFView.mm:420 > + _positionInformation.url = [url absoluteString]; url.absoluteString > Source/WebKit2/UIProcess/ios/WKPDFView.mm:446 > + (NSString *)kUTTypeUTF8PlainText : _positionInformation.url, Is this really “UTF-8”? > Source/WebKit2/UIProcess/ios/WKPDFView.mm:450 > + [[UIPasteboard generalPasteboard] setItems:@[representations]]; Need spaces inside the @[], can use .items = ;
(In reply to comment #4) > Comment on attachment 240928 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240928&action=review > > > Source/WebKit2/ChangeLog:25 > > + (-[WKPDFView updatePositionInformation]): Added an empty implementation. > > Perhaps this delegate method should be optional? Yeah. > > > Source/WebKit2/ChangeLog:30 > > + (-[WKPDFView startInteractionWithElement:]): Added an empty implementation. > > + (-[WKPDFView stopInteraction]): Ditto. > > Perhaps these delegate method should be optional? Ok. > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:343 > > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, duration * NSEC_PER_SEC), dispatch_get_main_queue(), ^ { > > Extra space between ^ and { Will fix. > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:359 > > + return [NSURL URLWithString:anchorString relativeToURL:documentURL]; > > You can use +_web_URLWithWTFString:relativeToURL: here. > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:-363 > > - ASSERT(isMainThread()); > > Why? Oops. I meant to move this to _highlightLinkAnnotation:forDuration:completionHandler:, not remove it. > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:420 > > + _positionInformation.url = [url absoluteString]; > > url.absoluteString Ok. > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:446 > > + (NSString *)kUTTypeUTF8PlainText : _positionInformation.url, > > Is this really “UTF-8”? kUTTypeUTF8PlainText is the pasteboard type that's used if you were to call -[UIPasteboard setString:]. > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:450 > > + [[UIPasteboard generalPasteboard] setItems:@[representations]]; > > Need spaces inside the @[], can use .items = ; Ok.
Created attachment 240953 [details] Patch
Attachment 240953 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:355: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240953&action=review > Source/WebKit2/UIProcess/ios/WKPDFView.mm:426 > + [self _highlightLinkAnnotation:linkAnnotation forDuration:.75 completionHandler:^ { ^{
Committed r175595: <http://trac.webkit.org/changeset/175595>
(In reply to comment #4) > Comment on attachment 240928 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240928&action=review > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:359 > > + return [NSURL URLWithString:anchorString relativeToURL:documentURL]; > > You can use +_web_URLWithWTFString:relativeToURL: here. Turns out this method behaves differently than +URLWithString:relativeToURL:. NSURL's method ignores fragments the base URL, but our method doesn't. If the base URL already contains a fragment and then we navigate to another one, we end up appending to the existing fragment rather than replacing it.
(In reply to comment #10) > (In reply to comment #4) > > Comment on attachment 240928 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=240928&action=review > > > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:359 > > > + return [NSURL URLWithString:anchorString relativeToURL:documentURL]; > > > > You can use +_web_URLWithWTFString:relativeToURL: here. > > Turns out this method behaves differently than > +URLWithString:relativeToURL:. NSURL's method ignores fragments the base > URL, but our method doesn't. If the base URL already contains a fragment and > then we navigate to another one, we end up appending to the existing > fragment rather than replacing it. Uh oh. Sorry about giving you bad advice!
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #4) > > > Comment on attachment 240928 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=240928&action=review > > > > > > > Source/WebKit2/UIProcess/ios/WKPDFView.mm:359 > > > > + return [NSURL URLWithString:anchorString relativeToURL:documentURL]; > > > > > > You can use +_web_URLWithWTFString:relativeToURL: here. > > > > Turns out this method behaves differently than > > +URLWithString:relativeToURL:. NSURL's method ignores fragments the base > > URL, but our method doesn't. If the base URL already contains a fragment and > > then we navigate to another one, we end up appending to the existing > > fragment rather than replacing it. > > Uh oh. Sorry about giving you bad advice! That's ok. I'm trying to see who's behavior is correct to see if I need to file a bug. RFC 3986 says "If the base URI is obtained from a URI reference, then that reference must be converted to absolute form and stripped of any fragment component prior to its use as a base URI." in http://tools.ietf.org/html/rfc3986#section-5.1), so it sounds like perhaps KURL should be doing this. Anyway, I'm going to switch back to the NSURL method unless you object, Dan.
(In reply to comment #12) > Anyway, I'm going to switch back to the NSURL method unless you object, Dan. Do it!
Committed r175607: <http://trac.webkit.org/changeset/175607>