Bug 138357 - [iOS] Add long press support for links in WKPDFView
Summary: [iOS] Add long press support for links in WKPDFView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-11-04 08:56 PST by Andy Estes
Modified: 2014-11-04 20:56 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.27 KB, patch)
2014-11-04 09:19 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (15.41 KB, patch)
2014-11-04 14:52 PST, Andy Estes
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2014-11-04 08:56:12 PST
[iOS] Add long press support for links in WKPDFView
Comment 1 Andy Estes 2014-11-04 08:57:35 PST
rdar://problem/18334903
Comment 2 Andy Estes 2014-11-04 09:19:05 PST
Created attachment 240928 [details]
Patch
Comment 3 WebKit Commit Bot 2014-11-04 09:21:12 PST
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 4 mitz 2014-11-04 09:48:16 PST
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 = ;
Comment 5 Andy Estes 2014-11-04 14:08:36 PST
(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.
Comment 6 Andy Estes 2014-11-04 14:52:31 PST
Created attachment 240953 [details]
Patch
Comment 7 WebKit Commit Bot 2014-11-04 14:53:50 PST
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 8 mitz 2014-11-04 15:17:19 PST
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:^ {

^{
Comment 9 Andy Estes 2014-11-04 17:24:12 PST
Committed r175595: <http://trac.webkit.org/changeset/175595>
Comment 10 Andy Estes 2014-11-04 20:30:07 PST
(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.
Comment 11 mitz 2014-11-04 20:31:45 PST
(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!
Comment 12 Andy Estes 2014-11-04 20:35:29 PST
(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.
Comment 13 mitz 2014-11-04 20:36:20 PST
(In reply to comment #12)

> Anyway, I'm going to switch back to the NSURL method unless you object, Dan.

Do it!
Comment 14 Andy Estes 2014-11-04 20:56:47 PST
Committed r175607: <http://trac.webkit.org/changeset/175607>