Cleanup DataDetection.mm a little bit
Created attachment 275460 [details] Patch
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?
(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 on attachment 275473 [details] Patch Clearing flags on attachment: 275473 Committed r198974: <http://trac.webkit.org/changeset/198974>
All reviewed patches have been landed. Closing bug.
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 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 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.)
(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.