We now have SPI from DD core, to understand how different types of DD links should be handled. rdar://problem/25303631
Created attachment 274719 [details] Patch
Created attachment 274720 [details] Patch2 Fixing iOS build
Attachment 274720 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:18: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 274722 [details] Patch3 One more attempt to fix the build
Attachment 274722 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:18: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 274722 [details] Patch3 View in context: https://bugs.webkit.org/attachment.cgi?id=274722&action=review > Source/WebCore/editing/cocoa/DataDetection.mm:58 > bool DataDetection::isDataDetectorLink(Element* element) Since this dereferences the pointer without checking for null, the argument type should be Element&, not Element*. Same for callers. Or it should do a check that is null-safe, like calling is<> without the *. > Source/WebCore/editing/cocoa/DataDetection.mm:63 > + return [softLink_DataDetectorsCore_DDURLTapAndHoldSchemes() containsObject:(NSString *)downcast<HTMLAnchorElement>(*element).href().protocol()]; What about case folding? Do we need to call convertToASCIILowercase here? > Source/WebCore/editing/cocoa/DataDetection.mm:86 > + if (softLink_DataDetectorsCore_DDShouldImmediatelyShowActionSheetForURL(downcast<HTMLAnchorElement>(*element).href())) > + return true; Does the data detector function do the right thing with scheme/protocol case sensitivity in this function? > Source/WebCore/editing/cocoa/DataDetection.mm:88 > + String resultAttribute = element->getAttribute(dataDetectorsAttributeResultKey); Should be fastGetAttribute. Return type is const AtomicString& and we should not normally use String unless we have a reason. Can just use resultAttribute.string() below. > Source/WebCore/editing/cocoa/DataDetection.mm:102 > + if (softLink_DataDetectorsCore_DDShouldImmediatelyShowActionSheetForResult(result)) > return true; > return false; Normally we prefer "return x" to "if (x) return true; return false;" > Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:102 > +- (DDResultRef) coreResult; Stray space here after ")".
Thanks for the feedback. I've changed all the functions to take a reference. > > Source/WebCore/editing/cocoa/DataDetection.mm:63 > > + return [softLink_DataDetectorsCore_DDURLTapAndHoldSchemes() containsObject:(NSString *)downcast<HTMLAnchorElement>(*element).href().protocol()]; > > What about case folding? Do we need to call convertToASCIILowercase here? > > > Source/WebCore/editing/cocoa/DataDetection.mm:86 I'll check if the DD core function does it properly otherwise I'll add it. > > + if (softLink_DataDetectorsCore_DDShouldImmediatelyShowActionSheetForURL(downcast<HTMLAnchorElement>(*element).href())) > > + return true; > > Does the data detector function do the right thing with scheme/protocol case > sensitivity in this function? > Same here. > > Source/WebCore/editing/cocoa/DataDetection.mm:88 > > + String resultAttribute = element->getAttribute(dataDetectorsAttributeResultKey); > > Should be fastGetAttribute. Return type is const AtomicString& and we should > not normally use String unless we have a reason. Can just use > resultAttribute.string() below. > > > Source/WebCore/editing/cocoa/DataDetection.mm:102 > > + if (softLink_DataDetectorsCore_DDShouldImmediatelyShowActionSheetForResult(result)) > > return true; > > return false; > Sure. > Normally we prefer "return x" to "if (x) return true; return false;" > > > Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:102 > > +- (DDResultRef) coreResult; > > Stray space here after ")". ok.
Created attachment 274799 [details] Patch4 Addresses comments and fixes Mac build.
Attachment 274799 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:18: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 274799 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=274799&action=review > Source/WebCore/editing/cocoa/DataDetection.mm:162 > + return element.getAttribute(dataDetectorsAttributeTypeKey) == "calendar-event"; This should use fastGetAttribute() and probably use equalLettersIgnoringASCIICase(). > Source/WebCore/editing/cocoa/DataDetection.mm:167 > + return element.getAttribute(dataDetectorsAttributeResultKey); This should use fastGetAttribute() and probably use equalLettersIgnoringASCIICase(). > Source/WebCore/editing/cocoa/DataDetection.mm:178 > + const AtomicString& resultAttribute = element.getAttribute(dataDetectorsAttributeResultKey); This should use fastGetAttribute(). > Source/WebCore/editing/cocoa/DataDetection.mm:184 > + DDResultRef result = [[results objectAtIndex:resultIndices[0].toInt()] coreResult]; What is guaranteeing that resultIndices[0] will exist? > Source/WebCore/editing/cocoa/DataDetection.mm:186 > + for (size_t i = 1; i < resultIndices.size(); i++) { This could use a modern for-loop. for (auto& string : resultIndices) ...
(In reply to comment #10) > Comment on attachment 274799 [details] > Patch4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274799&action=review > > > Source/WebCore/editing/cocoa/DataDetection.mm:162 > > + return element.getAttribute(dataDetectorsAttributeTypeKey) == "calendar-event"; > > This should use fastGetAttribute() and probably use > equalLettersIgnoringASCIICase(). > > > Source/WebCore/editing/cocoa/DataDetection.mm:167 > > + return element.getAttribute(dataDetectorsAttributeResultKey); > > This should use fastGetAttribute() and probably use > equalLettersIgnoringASCIICase(). > I'll use fastGetAttribute and declare the constants as QualifiedName. I have full control over case, I create the attributes. > > Source/WebCore/editing/cocoa/DataDetection.mm:184 > > + DDResultRef result = [[results objectAtIndex:resultIndices[0].toInt()] coreResult]; > > What is guaranteeing that resultIndices[0] will exist? I've checked above that this was a DD link with results, therefore I have a result array. > > > Source/WebCore/editing/cocoa/DataDetection.mm:186 > > + for (size_t i = 1; i < resultIndices.size(); i++) { > > This could use a modern for-loop. for (auto& string : resultIndices) ... How can I do that not starting from the first element? My loop starts from 1.
Committed revision 198653.