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
Created attachment 274242 [details] Patch
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
Committed revision 198311.
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.