Bug 155569 - Recognize mailto and tel url as data detector links
Summary: Recognize mailto and tel url as data detector links
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-16 17:05 PDT by Enrica Casucci
Modified: 2016-03-17 08:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.09 KB, patch)
2016-03-16 17:08 PDT, Enrica Casucci
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2016-03-16 17:05:09 PDT
When we check if the element is a data detector link, we should return true also for URL with mailto: and tel: scheme.

rdar://problem/24836185
Comment 1 Enrica Casucci 2016-03-16 17:08:28 PDT
Created attachment 274242 [details]
Patch
Comment 2 Sam Weinig 2016-03-16 17:20:41 PDT
Comment on attachment 274242 [details]
Patch

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

We really need to find a way to test this stuff.

> Source/WebCore/editing/cocoa/DataDetection.mm:58
>  bool DataDetection::isDataDetectorLink(Element* element)

Maybe this function should be called isHandledByDataDetectors.  In my mind, isDataDetectorLink sounds like a link created by data detectors.

> Source/WebCore/editing/cocoa/DataDetection.mm:65
> +    String protocol = downcast<HTMLAnchorElement>(*element).href().protocol();
> +    return element->getAttribute(dataDetectorsURLScheme) == "true" || protocol == "mailto" || protocol == "tel";

This is slightly inefficient since you are computing the protocol prior to needing it.  I would break it up:

if (element->getAttribute(dataDetectorsURLScheme) == "true")
    return true;

URL url = downcast<HTMLAnchorElement>(*element).href();
return url.protocolIs("mailto") || url.protocolIs("tel");

> Source/WebCore/editing/cocoa/DataDetection.mm:84
> +    // FIXME: We should be able to retrieve this informatio from DataDetectorsCore when rdar://problem/25169133 is fixed.

information -> information
Comment 3 Enrica Casucci 2016-03-16 17:56:28 PDT
Committed revision 198311.
Comment 4 Darin Adler 2016-03-17 08:56:57 PDT
Comment on attachment 274242 [details]
Patch

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

>> Source/WebCore/editing/cocoa/DataDetection.mm:58
>>  bool DataDetection::isDataDetectorLink(Element* element)
> 
> Maybe this function should be called isHandledByDataDetectors.  In my mind, isDataDetectorLink sounds like a link created by data detectors.

This function always dereferences the pointer, so a null pointer is not allowed. Thus, the function argument should be Element& rather than Element*. Same applies to many other functions in this file, I think.

>> Source/WebCore/editing/cocoa/DataDetection.mm:65
>> +    return element->getAttribute(dataDetectorsURLScheme) == "true" || protocol == "mailto" || protocol == "tel";
> 
> This is slightly inefficient since you are computing the protocol prior to needing it.  I would break it up:
> 
> if (element->getAttribute(dataDetectorsURLScheme) == "true")
>     return true;
> 
> URL url = downcast<HTMLAnchorElement>(*element).href();
> return url.protocolIs("mailto") || url.protocolIs("tel");

Should use the poorly named but almost always better to use fastGetAttribute.

Almost everywhere we check if a DOM attribute is true, we do it in a case folding way: equalLettersIgnoringASCIICase(x, "true"). It’s unusual to check in a case sensitive way; there are only 1 or 2 cases of that in all of WebCore, and 20 or more cases of the case folding version. But since this is something we are invented, I suppose we can make it case sensitive and it would be arbitrary to change it now.