Bug 155780 – Adopt new SPI from DataDetectorsCore to decide link behavior

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

[Enrica Casucci]

We now have SPI from DD core, to understand how different types of DD links should be handled.

rdar://problem/25303631

[Enrica Casucci]

Created attachment 274719 [details]
Patch

[Enrica Casucci]

Created attachment 274720 [details]
Patch2

Fixing iOS build

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 274722 [details]
Patch3

One more attempt to fix the build

[WebKit Commit Bot]

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.

[Darin Adler]

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 ")".

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 274799 [details]
Patch4

Addresses comments and fixes Mac build.

[WebKit Commit Bot]

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.

[Sam Weinig]

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) ...

[Enrica Casucci]

(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.

[Enrica Casucci]

Committed revision 198653.