Bug 156128

Summary: Cleanup DataDetection.mm a little bit
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Sam Weinig 2016-04-01 21:14:48 PDT
Cleanup DataDetection.mm a little bit
Comment 1 Sam Weinig 2016-04-01 21:20:10 PDT
Created attachment 275460 [details]
Patch
Comment 2 Sam Weinig 2016-04-02 11:22:33 PDT
Created attachment 275473 [details]
Patch
Comment 3 mitz 2016-04-02 11:28:22 PDT
Comment on attachment 275473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275473&action=review

> Source/WebCore/editing/cocoa/DataDetection.mm:49
> +#import <wtf/text/StringBuilder.h>
> +
> +#import "DataDetectorsCoreSoftLink.h"

Why is this separate? Some ordering requirement?

> Source/WebCore/editing/cocoa/DataDetection.mm:339
> +        return String::number((unsigned long)[path indexAtPosition:0]);

Is the cast needed here?

> Source/WebCore/editing/cocoa/DataDetection.mm:346
> +    case 2: {
> +        StringBuilder stringBuilder;
> +        stringBuilder.appendNumber((unsigned long)[path indexAtPosition:0]);
> +        stringBuilder.append('/');
> +        stringBuilder.appendNumber((unsigned long)[path indexAtPosition:1]);
> +        return stringBuilder.toString();
> +    }

What are we gaining from special-casing 2?
Comment 4 Sam Weinig 2016-04-02 11:29:56 PDT
(In reply to comment #3)
> Comment on attachment 275473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275473&action=review
> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:49
> > +#import <wtf/text/StringBuilder.h>
> > +
> > +#import "DataDetectorsCoreSoftLink.h"
> 
> Why is this separate? Some ordering requirement?

The style checker yelled at me for it. I actually don't know why it wants it there.

> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:339
> > +        return String::number((unsigned long)[path indexAtPosition:0]);
> 
> Is the cast needed here?

Yes, the compiler didn't like it otherwise.

> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:346
> > +    case 2: {
> > +        StringBuilder stringBuilder;
> > +        stringBuilder.appendNumber((unsigned long)[path indexAtPosition:0]);
> > +        stringBuilder.append('/');
> > +        stringBuilder.appendNumber((unsigned long)[path indexAtPosition:1]);
> > +        return stringBuilder.toString();
> > +    }
> 
> What are we gaining from special-casing 2?

Unsure, but since it was already there, I did't remove it.
Comment 5 WebKit Commit Bot 2016-04-02 13:01:30 PDT
Comment on attachment 275473 [details]
Patch

Clearing flags on attachment: 275473

Committed r198974: <http://trac.webkit.org/changeset/198974>
Comment 6 WebKit Commit Bot 2016-04-02 13:01:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 David Kilzer (:ddkilzer) 2016-04-03 09:51:12 PDT
Comment on attachment 275473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275473&action=review

> Source/WebCore/editing/cocoa/DataDetection.mm:-52
> -const char* dataDetectorsURLScheme = "x-apple-data-detectors";

This is a URL scheme, not an HTML attribute.  Seems odd to add it to HTMLAttributeNames.in, doesn't it?

> Source/WebCore/html/HTMLAttributeNames.in:385
> +x-apple-data-detectors

Again, this is a URL scheme.  It probably shouldn't have been added to HTMLAttributeNames.in.
Comment 8 David Kilzer (:ddkilzer) 2016-04-03 09:53:51 PDT
Comment on attachment 275473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275473&action=review

>>> Source/WebCore/editing/cocoa/DataDetection.mm:49
>>> +#import "DataDetectorsCoreSoftLink.h"
>> 
>> Why is this separate? Some ordering requirement?
> 
> The style checker yelled at me for it. I actually don't know why it wants it there.

This is the correct place for the header, and I updated the style checker to enforce this ordering.

SoftLinking headers include some #define macros that can interfere with other headers included after it, so it must go last.
Comment 9 David Kilzer (:ddkilzer) 2016-04-03 09:55:30 PDT
Comment on attachment 275473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275473&action=review

>> Source/WebCore/editing/cocoa/DataDetection.mm:-52
>> -const char* dataDetectorsURLScheme = "x-apple-data-detectors";
> 
> This is a URL scheme, not an HTML attribute.  Seems odd to add it to HTMLAttributeNames.in, doesn't it?

Nevermind, this is actually used as an attribute apparently.  Very confusing.

>> Source/WebCore/html/HTMLAttributeNames.in:385
>> +x-apple-data-detectors
> 
> Again, this is a URL scheme.  It probably shouldn't have been added to HTMLAttributeNames.in.

Nevermind.  (See above.)
Comment 10 Sam Weinig 2016-04-03 11:22:09 PDT
(In reply to comment #7)
> Comment on attachment 275473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275473&action=review
> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:-52
> > -const char* dataDetectorsURLScheme = "x-apple-data-detectors";
> 
> This is a URL scheme, not an HTML attribute.  Seems odd to add it to
> HTMLAttributeNames.in, doesn't it?
> 

It's not a URL scheme. The variable name was simply wrong.