Bug 156128 – Cleanup DataDetection.mm a little bit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Cleanup DataDetection.mm a little bit

[Sam Weinig]

Created attachment 275460 [details]
Patch

[Sam Weinig]

Created attachment 275473 [details]
Patch

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?

[Sam Weinig]

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

[WebKit Commit Bot]

Comment on attachment 275473 [details]
Patch

Clearing flags on attachment: 275473

Committed r198974: <http://trac.webkit.org/changeset/198974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[Sam Weinig]

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