Bug 138357 – [iOS] Add long press support for links in WKPDFView

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

[Andy Estes]

[iOS] Add long press support for links in WKPDFView

[Andy Estes]

rdar://problem/18334903

[Andy Estes]

Created attachment 240928 [details]
Patch

[WebKit Commit Bot]

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 = ;

[Andy Estes]

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

[Andy Estes]

Created attachment 240953 [details]
Patch

[WebKit Commit Bot]

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:^ {

^{

[Andy Estes]

Committed r175595: <http://trac.webkit.org/changeset/175595>

[Andy Estes]

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

[Andy Estes]

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

[Andy Estes]

Committed r175607: <http://trac.webkit.org/changeset/175607>