155569 2016-03-16 17:05:09 -0700 Recognize mailto and tel url as data detector links 2016-03-17 08:56:57 -0700 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 enrica enrica sam thorton oldest_to_newest 1175573 0 enrica 2016-03-16 17:05:09 -0700 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 1175578 1 274242 enrica 2016-03-16 17:08:28 -0700 Created attachment 274242 Patch 1175583 2 274242 sam 2016-03-16 17:20:41 -0700 Comment on attachment 274242 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 1175589 3 enrica 2016-03-16 17:56:28 -0700 Committed revision 198311. 1175741 4 274242 darin 2016-03-17 08:56:57 -0700 Comment on attachment 274242 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. 274242 2016-03-16 17:08:28 -0700 2016-03-16 17:20:41 -0700 Patch mailpreview.txt text/plain 2138 enrica SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5ODMwOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE2LTAzLTE2ICBFbnJpY2Eg Q2FzdWNjaSAgPGVucmljYUBhcHBsZS5jb20+CisKKyAgICAgICAgUmVjb2duaXplIG1haWx0byBh bmQgdGVsIHVybCBhcyBkYXRhIGRldGVjdG9yIGxpbmtzLgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU1NTY5CisgICAgICAgIHJkYXI6Ly9wcm9ibGVt LzI0ODM2MTg1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgV2hlbiB3ZSBjaGVjayBpZiB0aGUgZWxlbWVudCBpcyBhIGRhdGEgZGV0ZWN0b3IgbGluaywK KyAgICAgICAgd2Ugc2hvdWxkIHJldHVybiB0cnVlIGFsc28gZm9yIFVSTHMgd2l0aCBtYWlsdG86 IGFuZCB0ZWw6IHNjaGVtZS4KKworICAgICAgICAqIGVkaXRpbmcvY29jb2EvRGF0YURldGVjdGlv bi5tbToKKyAgICAgICAgKFdlYkNvcmU6OkRhdGFEZXRlY3Rpb246OmlzRGF0YURldGVjdG9yTGlu ayk6CisKIDIwMTYtMDMtMTYgIEJldGggRGFraW4gIDxiZGFraW5AYXBwbGUuY29tPgogCiAgICAg ICAgIFByb3ZpZGUgTlNTcGVsbENoZWNrZXIgc3BlbGxDaGVja2luZyBtZXRob2RzIHdpdGggdGhl IGN1cnJlbnQgaW5zZXJ0aW9uIHBvaW50CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9lZGl0aW5nL2Nv Y29hL0RhdGFEZXRlY3Rpb24ubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvZWRpdGlu Zy9jb2NvYS9EYXRhRGV0ZWN0aW9uLm1tCShyZXZpc2lvbiAxOTgzMDgpCisrKyBTb3VyY2UvV2Vi Q29yZS9lZGl0aW5nL2NvY29hL0RhdGFEZXRlY3Rpb24ubW0JKHdvcmtpbmcgY29weSkKQEAgLTU3 LDcgKzU3LDEyIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAogYm9vbCBEYXRhRGV0ZWN0aW9uOjpp c0RhdGFEZXRlY3RvckxpbmsoRWxlbWVudCogZWxlbWVudCkKIHsKLSAgICByZXR1cm4gZWxlbWVu dC0+Z2V0QXR0cmlidXRlKGRhdGFEZXRlY3RvcnNVUkxTY2hlbWUpID09ICJ0cnVlIjsKKyAgICAv LyBGSVhNRTogV2Ugc2hvdWxkIGJlIGFibGUgdG8gYXNrIHRoaXMgZnJvbSBEYXRhRGV0ZWN0b3Jz Q29yZSB3aGVuIHJkYXI6Ly9wcm9ibGVtLzI1MjA2MDYyIGlzIGZpeGVkLgorICAgIGlmICghaXM8 SFRNTEFuY2hvckVsZW1lbnQ+KCplbGVtZW50KSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworCisg ICAgU3RyaW5nIHByb3RvY29sID0gZG93bmNhc3Q8SFRNTEFuY2hvckVsZW1lbnQ+KCplbGVtZW50 KS5ocmVmKCkucHJvdG9jb2woKTsKKyAgICByZXR1cm4gZWxlbWVudC0+Z2V0QXR0cmlidXRlKGRh dGFEZXRlY3RvcnNVUkxTY2hlbWUpID09ICJ0cnVlIiB8fCBwcm90b2NvbCA9PSAibWFpbHRvIiB8 fCBwcm90b2NvbCA9PSAidGVsIjsKIH0KIAogYm9vbCBEYXRhRGV0ZWN0aW9uOjpyZXF1aXJlc0V4 dGVuZGVkQ29udGV4dChFbGVtZW50KiBlbGVtZW50KQpAQCAtNzYsNyArODEsNyBAQCBib29sIERh dGFEZXRlY3Rpb246OnNob3VsZENhbmNlbERlZmF1bHRBCiAgICAgVU5VU0VEX1BBUkFNKGVsZW1l bnQpOwogICAgIHJldHVybiBmYWxzZTsKICNlbHNlCi0gICAgLy8gRklYTUU6IFdlIHNob3VsZCBh bHNvIGNvbXB1dGUgdGhlIEREUmVzdWx0UmVmIGFuZCBjaGVjayB0aGUgcmVzdWx0IGNhdGVnb3J5 LgorICAgIC8vIEZJWE1FOiBXZSBzaG91bGQgYmUgYWJsZSB0byByZXRyaWV2ZSB0aGlzIGluZm9y bWF0aW8gZnJvbSBEYXRhRGV0ZWN0b3JzQ29yZSB3aGVuIHJkYXI6Ly9wcm9ibGVtLzI1MTY5MTMz IGlzIGZpeGVkLgogICAgIGlmICghaXM8SFRNTEFuY2hvckVsZW1lbnQ+KCplbGVtZW50KSkKICAg ICAgICAgcmV0dXJuIGZhbHNlOwogICAgIGlmIChlbGVtZW50LT5nZXRBdHRyaWJ1dGUoZGF0YURl dGVjdG9yc1VSTFNjaGVtZSkgIT0gInRydWUiKQo=