Bug 155569 – Recognize mailto and tel url as data detector links

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Enrica Casucci]

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

[Enrica Casucci]

Created attachment 274242 [details]
Patch

[Sam Weinig]

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

[Enrica Casucci]

Committed revision 198311.

[Darin Adler]

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.