Long pressing a phone number with spaces in it results in a link sheet instead of a data detectors sheet

[Tim Horton]

Long pressing a phone number with spaces in it results in a link sheet instead of a data detectors sheet

[Tim Horton]

Created attachment 326832 [details]
Patch

[Tim Horton]

<rdar://problem/35337288>

[Darin Adler]

Comment on attachment 326832 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326832&action=review

> Source/WebCore/editing/cocoa/DataDetection.h:65
> +    WEBCORE_EXPORT static bool URLCanBePresentedByDataDetectors(const URL&);

I don’t think this function needs the word “URL” in its name. Either linkCanBePresentefdByDataDetectors or canBePresentedByDataDetectors seems fine to me.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:559
> +    NSURL *targetURL = [NSURL _web_URLWithWTFString:_positionInformation->url];

Why isn’t this just:

    NSURL *targetURL = _positionInformation->url;

Does the _web_URLWithWTFString method do something special that the NSURL operator from WebCore::URL does not?

[Wenson Hsieh]

I think writing a test for this wouldn't be too bad — you can find similar tests in ActionSheetTests. Checking that the resulting _WKActivatedElementInfo's type is not a link, image or attachment after simulating a long press would cover this.

[Tim Horton]

(In reply to Darin Adler from comment #3)
> Comment on attachment 326832 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326832&action=review
> 
> > Source/WebCore/editing/cocoa/DataDetection.h:65
> > +    WEBCORE_EXPORT static bool URLCanBePresentedByDataDetectors(const URL&);
> 
> I don’t think this function needs the word “URL” in its name. Either
> linkCanBePresentefdByDataDetectors or canBePresentedByDataDetectors seems
> fine to me.

Sure!

> > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:559
> > +    NSURL *targetURL = [NSURL _web_URLWithWTFString:_positionInformation->url];
> 
> Why isn’t this just:
> 
>     NSURL *targetURL = _positionInformation->url;
> 
> Does the _web_URLWithWTFString method do something special that the NSURL
> operator from WebCore::URL does not?

I think somebody got confused and thought _positionInformation->url was a string. I’ll fix it.

(In reply to Wenson Hsieh from comment #4)
> I think writing a test for this wouldn't be too bad — you can find similar
> tests in ActionSheetTests. Checking that the resulting
> _WKActivatedElementInfo's type is not a link, image or attachment after
> simulating a long press would cover this.

Good idea!

[Tim Horton]

> > > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:559
> > > +    NSURL *targetURL = [NSURL _web_URLWithWTFString:_positionInformation->url];
> > 
> > Why isn’t this just:
> > 
> >     NSURL *targetURL = _positionInformation->url;
> > 
> > Does the _web_URLWithWTFString method do something special that the NSURL
> > operator from WebCore::URL does not?
> 
> I think somebody got confused and thought _positionInformation->url was a
> string. I’ll fix it.

Actually, it *used to be* a string, until https://trac.webkit.org/changeset/216152/webkit, but I missed some cleanup!

[Tim Horton]

Created attachment 326857 [details]
Patch

[Tim Horton]

Created attachment 326858 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326858 [details]
Patch

Clearing flags on attachment: 326858

Committed r224819: <https://trac.webkit.org/changeset/224819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.