Bug 155780

Summary: Adopt new SPI from DataDetectorsCore to decide link behavior
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: WebCore Misc.Assignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, sam, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch2
none
Patch3
none
Patch4 sam: review+

Description Enrica Casucci 2016-03-22 18:01:45 PDT
We now have SPI from DD core, to understand how different types of DD links should be handled.

rdar://problem/25303631
Comment 1 Enrica Casucci 2016-03-22 18:08:04 PDT
Created attachment 274719 [details]
Patch
Comment 2 Enrica Casucci 2016-03-22 18:31:09 PDT
Created attachment 274720 [details]
Patch2

Fixing iOS build
Comment 3 WebKit Commit Bot 2016-03-22 18:32:47 PDT
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.
Comment 4 Enrica Casucci 2016-03-22 18:51:49 PDT
Created attachment 274722 [details]
Patch3

One more attempt to fix the build
Comment 5 WebKit Commit Bot 2016-03-22 18:54:08 PDT
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 6 Darin Adler 2016-03-23 08:40:58 PDT
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 ")".
Comment 7 Enrica Casucci 2016-03-23 16:10:04 PDT
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.
Comment 8 Enrica Casucci 2016-03-23 17:36:57 PDT
Created attachment 274799 [details]
Patch4

Addresses comments and fixes Mac build.
Comment 9 WebKit Commit Bot 2016-03-23 17:38:18 PDT
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 10 Sam Weinig 2016-03-24 16:11:23 PDT
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) ...
Comment 11 Enrica Casucci 2016-03-24 16:22:41 PDT
(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.
Comment 12 Enrica Casucci 2016-03-24 16:57:00 PDT
Committed revision 198653.