Bug 155569

Summary: Recognize mailto and tel url as data detector links
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: WebKit Misc.Assignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: sam, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch sam: review+

Enrica Casucci
Reported 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
Attachments
Patch (2.09 KB, patch)
2016-03-16 17:08 PDT, Enrica Casucci
sam: review+
Enrica Casucci
Comment 1 2016-03-16 17:08:28 PDT
Sam Weinig
Comment 2 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
Enrica Casucci
Comment 3 2016-03-16 17:56:28 PDT
Committed revision 198311.
Darin Adler
Comment 4 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.
Note You need to log in before you can comment on or make changes to this bug.