This tracks the work required to support data detectors in WK2 on iOS.
Created attachment 268717 [details] Patch
Attachment 268717 [details] did not pass style-queue: ERROR: Source/WebCore/editing/cocoa/DataDetection.h:55: The parameter name "types" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:53: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 268732 [details] Patch2
Attachment 268732 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:53: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268732 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=268732&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:60 > +typedef NS_OPTIONS(NSUInteger, WKDataDetectorTypes) { > + WKDataDetectorTypeNone = 0, > + WKDataDetectorTypePhoneNumber = 1 << 0, > + WKDataDetectorTypeLink = 1 << 1, > + WKDataDetectorTypeAddress = 1 << 2, > + WKDataDetectorTypeCalendarEvent = 1 << 3, > + WKDataDetectorTypeAll = 0xFFFF > +} WK_ENUM_AVAILABLE_IOS(9_3); Are we sure we want to expose these as API under 9.3? If so, they need to be documented. I also think that WKDataDetectorTypeAll should be NSUIntegerMax. > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:135 > +@property (nonatomic) WKDataDetectorTypes dataDetectorTypes; This needs availability annotations. Also, do we want to expose this as API?
(In reply to comment #5) > Comment on attachment 268732 [details] > Patch2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268732&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:60 > > +typedef NS_OPTIONS(NSUInteger, WKDataDetectorTypes) { > > + WKDataDetectorTypeNone = 0, > > + WKDataDetectorTypePhoneNumber = 1 << 0, > > + WKDataDetectorTypeLink = 1 << 1, > > + WKDataDetectorTypeAddress = 1 << 2, > > + WKDataDetectorTypeCalendarEvent = 1 << 3, > > + WKDataDetectorTypeAll = 0xFFFF > > +} WK_ENUM_AVAILABLE_IOS(9_3); > Actually is should be 10_0. > Are we sure we want to expose these as API under 9.3? If so, they need to be > documented. > > I also think that WKDataDetectorTypeAll should be NSUIntegerMax. > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:135 > > +@property (nonatomic) WKDataDetectorTypes dataDetectorTypes; > > This needs availability annotations. Also, do we want to expose this as API? I'll add the availability annotations. I believe we do, since we have them as API in UIWebView.
Created attachment 268774 [details] Patch3 Fixes build issues and addresses feedback.
Attachment 268774 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:66: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 268782 [details] Patch4 One more attempt to fix the build on all the platforms.
Attachment 268782 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:66: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 268782 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=268782&action=review > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:114 > +ENABLE_IOS_DATADETECTION[sdk=iphone*] = ENABLE_IOS_DATADETECTION; Feels like there should be another underscore: ENABLE_IOS_DATA_DETECTION. Surely in camel-case it would be DataDetection not Datadetection. > Source/WebCore/ChangeLog:12 > + have been moved under cocoa, since they are no longer OSX specific. space between OS X > Source/WebCore/loader/FrameLoader.cpp:49 > +#if PLATFORM(COCOA) This goes in its own block, not in the middle of these others. > Source/WebCore/page/Settings.h:30 > +#if PLATFORM(COCOA) This goes in its own block below, not here.
Comment on attachment 268782 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=268782&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2936 > + settings.setDataDetectorTypes(static_cast<DataDetectorTypes>(store.getUInt32ValueForKey(WebPreferencesKey::dataDetectorTypesKey()))); I don't think you can do this. The values aren't the same (they probably should be, though!!), and we usually use explicit functions that cast between them using the names instead of just praying that the values are the same.
Comment on attachment 268782 [details] Patch4 Broad question here. Why is this iOS only? Can this form of data detection not work on Mac? I would much rather see this as cross platform.
Created attachment 269118 [details] Patch5 The new property is now available on OS X and iOS.
Attachment 269118 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:67: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269118 [details] Patch5 View in context: https://bugs.webkit.org/attachment.cgi?id=269118&action=review > Source/WTF/wtf/FeatureDefines.h:110 > +#define ENABLE_DATA_DETECTION 1 This is now sitting in a PLATFORM(IOS) only place. But isn't it now PLATFORM(COCOA)? > Source/WebCore/editing/cocoa/DataDetection.mm:144 > +void DataDetection::detectContentInRange(RefPtr<Range>&, DataDetectorTypes) > +{ > +} This doesn't have a return value; is that because it's going to do linkification? In that case, it should probably have a different name. Also possible that detection and linkification should be in different functions. > Source/WebCore/loader/FrameLoader.cpp:128 > +#if PLATFORM(DATA_DETECTION) I don't think PLATFORM(DATA_DETECTION) is a thing > Source/WebCore/page/Settings.h:46 > +#include "DataDetection.h" Really? This file is included a lot of places, I think. We should see if there's any way to avoid this.
Comment on attachment 269118 [details] Patch5 View in context: https://bugs.webkit.org/attachment.cgi?id=269118&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:66 > + @discussion An example of how this property may affect the content loaded in the WKWebView is that content like > + 'Visit apple.com on July 4th or call 1 800 555-5545' will be transformed to add links around 'apple.com', 'July 4th' and '1 800 555-5545' > + if the dataDetectorTypes property is set to WKDataDetectorTypePhoneNumber | WKDataDetectorTypeLink | WKDataDetectorTypeCalendarEvent. > + */ Maybe this should be mentioned in the documentation about the dataDetectorTypes property instead.
(In reply to comment #16) > Comment on attachment 269118 [details] > Patch5 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269118&action=review > > > Source/WTF/wtf/FeatureDefines.h:110 > > +#define ENABLE_DATA_DETECTION 1 > > This is now sitting in a PLATFORM(IOS) only place. But isn't it now > PLATFORM(COCOA)? For now this is enabled only for iOS. The API is defined for COCOA. > > > Source/WebCore/editing/cocoa/DataDetection.mm:144 > > +void DataDetection::detectContentInRange(RefPtr<Range>&, DataDetectorTypes) > > +{ > > +} > > This doesn't have a return value; is that because it's going to do > linkification? In that case, it should probably have a different name. Also > possible that detection and linkification should be in different functions. > It will have a return value in the next patch. This is just a placeholder now. > > Source/WebCore/loader/FrameLoader.cpp:128 > > +#if PLATFORM(DATA_DETECTION) > > I don't think PLATFORM(DATA_DETECTION) is a thing > My bad. > > Source/WebCore/page/Settings.h:46 > > +#include "DataDetection.h" > > Really? This file is included a lot of places, I think. We should see if > there's any way to avoid this. I don't know how.
(In reply to comment #17) > Comment on attachment 269118 [details] > Patch5 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269118&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:66 > > + @discussion An example of how this property may affect the content loaded in the WKWebView is that content like > > + 'Visit apple.com on July 4th or call 1 800 555-5545' will be transformed to add links around 'apple.com', 'July 4th' and '1 800 555-5545' > > + if the dataDetectorTypes property is set to WKDataDetectorTypePhoneNumber | WKDataDetectorTypeLink | WKDataDetectorTypeCalendarEvent. > > + */ > > Maybe this should be mentioned in the documentation about the > dataDetectorTypes property instead. will do.
(In reply to comment #18) > (In reply to comment #16) > > Comment on attachment 269118 [details] > > Patch5 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=269118&action=review > > > > > Source/WTF/wtf/FeatureDefines.h:110 > > > +#define ENABLE_DATA_DETECTION 1 > > > > This is now sitting in a PLATFORM(IOS) only place. But isn't it now > > PLATFORM(COCOA)? > For now this is enabled only for iOS. The API is defined for COCOA. That's not true in the xcconfig... > > > Source/WebCore/editing/cocoa/DataDetection.mm:144 > > > +void DataDetection::detectContentInRange(RefPtr<Range>&, DataDetectorTypes) > > > +{ > > > +} > > > > This doesn't have a return value; is that because it's going to do > > linkification? In that case, it should probably have a different name. Also > > possible that detection and linkification should be in different functions. > > > It will have a return value in the next patch. This is just a placeholder > now. Okay. > > > Source/WebCore/page/Settings.h:46 > > > +#include "DataDetection.h" > > > > Really? This file is included a lot of places, I think. We should see if > > there's any way to avoid this. > I don't know how. Yeah, I guess not.
Created attachment 269272 [details] Patch6
Attachment 269272 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:64: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269272 [details] Patch6 View in context: https://bugs.webkit.org/attachment.cgi?id=269272&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:55 > + @abstract The type of data we want to detect. I don't think the "we" is right here. But again, I don't really know how these are supposed to sound. > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:60 > + @constant WKDataDetectorTypeCalendarEvent Dates and time that are in the future are detected and turned into links. "Time" -> "Times" > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:61 > + @constant WKDataDetectorTypeAll All of the above data types are turned into links when detected. Chosing this value will "Chosing" -> "Choosing" > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:117 > + An example of how this property may affect the content loaded in the WKWebView is that content like Not sure about the "an example of how" wording for this, but I don't really know how these comments are supposed to sound.
Committed revision 195300.
Comment on attachment 269118 [details] Patch5 View in context: https://bugs.webkit.org/attachment.cgi?id=269118&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:387 > + pageConfiguration->preferenceValues().set(WebKit::WebPreferencesKey::dataDetectorTypesKey(), WebKit::WebPreferencesStore::Value(static_cast<uint32_t>([_configuration dataDetectorTypes]))); Move fromWKDataDetectorTypes here (and set the pref to the WebCore value)
Created attachment 269295 [details] Patch7 Fixes the 32 bit build.
Attachment 269295 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:64: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/editing/cocoa/DataDetection.h:46: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/editing/cocoa/DataDetection.h:47: One space before end of line comments [whitespace/comments] [5] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build fixed before landing. Committed revision 195317
(In reply to comment #28) > Build fixed before landing. > Committed revision 195317 It broke the Apple Mac cmae build.
(In reply to comment #29) > (In reply to comment #28) > > Build fixed before landing. > > Committed revision 195317 > > It broke the Apple Mac cmae build. cmae -> cmake
cmake fix landed in https://trac.webkit.org/changeset/195348
(In reply to comment #31) > cmake fix landed in https://trac.webkit.org/changeset/195348 still broken: /Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebCore/page/Settings.h:46:10: fatal error: 'DataDetection.h' file not found #include "DataDetection.h" ^ 1 error generated.
http://trac.webkit.org/changeset/195358 should fix that.
Created attachment 269407 [details] Patch including detection.
Comment on attachment 269407 [details] Patch including detection. View in context: https://bugs.webkit.org/attachment.cgi?id=269407&action=review This is getting to the point where it really needs tests (and should be easy to add tests). There's a lot here, I'll look at it in a bit later. > Source/WebCore/editing/cocoa/DataDetection.mm:51 > +#import <DataDetectorsCore/DDBinderKeys_Private.h> > +#import <DataDetectorsCore/DDScannerResult.h> > +#import <DataDetectorsCore/DDURLifier.h> > +#import <DataDetectorsUI/DDUIBase.h> You're going to need some SPI headers. > Source/WebCore/editing/cocoa/DataDetection.mm:63 > +- (NSString *) dataDetectorStringValue No space after the ) > Source/WebCore/editing/cocoa/DataDetection.mm:65 > + NSUInteger length = [self length]; Dot notation
(In reply to comment #35) > Comment on attachment 269407 [details] > Patch including detection. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269407&action=review > > This is getting to the point where it really needs tests (and should be easy > to add tests). > > There's a lot here, I'll look at it in a bit later. > > > Source/WebCore/editing/cocoa/DataDetection.mm:51 > > +#import <DataDetectorsCore/DDBinderKeys_Private.h> > > +#import <DataDetectorsCore/DDScannerResult.h> > > +#import <DataDetectorsCore/DDURLifier.h> > > +#import <DataDetectorsUI/DDUIBase.h> > > You're going to need some SPI headers. > Yes, I'm fixing those since the build fails on non internal SDK. > > Source/WebCore/editing/cocoa/DataDetection.mm:63 > > +- (NSString *) dataDetectorStringValue > > No space after the ) > > > Source/WebCore/editing/cocoa/DataDetection.mm:65 > > + NSUInteger length = [self length]; > > Dot notation OK.
Created attachment 269415 [details] Patch with detection and build fixes
Attachment 269415 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269457 [details] Patch with detection and build fixes for iOS
Attachment 269457 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:104: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:105: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:106: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:107: The parameter name "scanQuery" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:107: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:107: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:107: The parameter name "mode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:107: The parameter name "coalescing" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:108: The parameter name "scanQuery" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:108: The parameter name "coalescing" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:109: The parameter name "scanQuery" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:110: The parameter name "query" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 12 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269460 [details] More build fixes
Attachment 269460 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:72: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:108: Extra space between CFIndex and queryIndex [whitespace/declaration] [3] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:109: Extra space between CFIndex and offset [whitespace/declaration] [3] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:113: Extra space between DDQueryOffset and start [whitespace/declaration] [3] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:114: Extra space between DDQueryOffset and end [whitespace/declaration] [3] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:130: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:135: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:136: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:137: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:138: The parameter name "scanQuery" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:138: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:138: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:138: The parameter name "mode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:138: The parameter name "coalescing" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:139: The parameter name "scanQuery" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:139: The parameter name "coalescing" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:140: The parameter name "scanQuery" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:141: The parameter name "query" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:142: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:143: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:144: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 21 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269462 [details] More build fixes and style
Created attachment 269465 [details] More build fixes
Comment on attachment 269465 [details] More build fixes View in context: https://bugs.webkit.org/attachment.cgi?id=269465&action=review > Source/WebCore/editing/cocoa/DataDetection.mm:50 > +const NSString *kDataDetectorsURLScheme = @"x-apple-data-detectors"; > +const NSString *kDataDetectorsAttributeTypeKey = @"x-apple-data-detectors-type"; > +const NSString *kDataDetectorsAttributeResultKey = @"x-apple-data-detectors-result"; Not sure our constants should have this prefix. > Source/WebCore/editing/cocoa/DataDetection.mm:400 > + RetainPtr<NSMutableArray> allResults = adoptNS([[NSMutableArray alloc] init]); Why is this NSMutableArray instead of Vector<RetainPtr<>>? It never gets handed to anything that expects a Foundation type, and we're building it up here ourselves. > Source/WebCore/editing/cocoa/DataDetection.mm:401 > + RetainPtr<NSMutableArray> indexPaths = adoptNS([[NSMutableArray alloc] init]); Ditto. > Source/WebCore/editing/cocoa/DataDetection.mm:414 > + CFArrayRef subresults = DDResultGetSubResults(result); > + [allResults addObjectsFromArray:(NSArray *)subresults]; > + > + for (NSUInteger i = 0 ; i < [(NSArray *)subresults count] ; i++) > + [indexPaths addObject:[indexPath indexPathByAddingIndex:i]]; Any reason not to do the cast in the assignment instead of twice later? > Source/WebCore/editing/cocoa/DataDetection.mm:438 > + while (iteratorCount < iteratorTargetAdvanceCount) { > + iterator.advance(); > + iteratorCount++; > + } Maybe we should consider adding an advanceBy, like you said? > Source/WebCore/editing/cocoa/DataDetection.mm:473 > + NSString *identifier = [[indexPaths objectAtIndex:resultIndex] dataDetectorStringValue]; > + NSString *correspondingURL = constructURLStringForResult(coreResult, identifier, referenceDate, (NSTimeZone *)tz, types); It's fairly surprising to me that in WebCore we're building strings and URLs with Foundation instead of WebCore types. dataDetectorStringValue, too. > Source/WebCore/editing/cocoa/DataDetection.mm:518 > + anchorElement->setDir("ltr"); !?!? that seems rather presumptuous > Source/WebCore/editing/cocoa/DataDetection.mm:528 > + anchorElement->setAttribute(QualifiedName(nullAtom, (NSString *)kDataDetectorsURLScheme, nullAtom), "true"); > + anchorElement->setAttribute(QualifiedName(nullAtom, (NSString *)kDataDetectorsAttributeTypeKey, nullAtom), dataDetectorTypeForCategory(DDResultGetCategory(coreResult))); > + anchorElement->setAttribute(QualifiedName(nullAtom, (NSString *)kDataDetectorsAttributeResultKey, nullAtom), identifier); If we are defining these constants, why are we making them NSString instead of String, if they're just going to something that wants a String (presumably)? > Source/WebCore/page/Frame.h:193 > + void setDataDetectionResults(NSArray *results) { m_dataDetectionResults = results; } Who calls this? I can't find a caller, but maybe that's just find-in-page being broken.
Created attachment 269466 [details] Keep trying to build
Comment on attachment 269465 [details] More build fixes View in context: https://bugs.webkit.org/attachment.cgi?id=269465&action=review >> Source/WebCore/editing/cocoa/DataDetection.mm:400 >> + RetainPtr<NSMutableArray> allResults = adoptNS([[NSMutableArray alloc] init]); > > Why is this NSMutableArray instead of Vector<RetainPtr<>>? It never gets handed to anything that expects a Foundation type, and we're building it up here ourselves. Just because I was lazy and wanted to be able to add all the sub results with one call :-). I can change it though . >> Source/WebCore/editing/cocoa/DataDetection.mm:414 >> + [indexPaths addObject:[indexPath indexPathByAddingIndex:i]]; > > Any reason not to do the cast in the assignment instead of twice later? No reason, just copy/paste. I'll fix it. >> Source/WebCore/editing/cocoa/DataDetection.mm:438 >> + } > > Maybe we should consider adding an advanceBy, like you said? I'll do it later. >> Source/WebCore/editing/cocoa/DataDetection.mm:473 >> + NSString *correspondingURL = constructURLStringForResult(coreResult, identifier, referenceDate, (NSTimeZone *)tz, types); > > It's fairly surprising to me that in WebCore we're building strings and URLs with Foundation instead of WebCore types. dataDetectorStringValue, too. constructURLStringForResult uses some DD functions to build the URL and we are just returning the value returned by them. dataDetectorStringValue is and extension of NSIndexPath and I think it makes sense to use NSString as return value. >> Source/WebCore/editing/cocoa/DataDetection.mm:518 >> + anchorElement->setDir("ltr"); > > !?!? that seems rather presumptuous Discussed in person. >> Source/WebCore/editing/cocoa/DataDetection.mm:528 >> + anchorElement->setAttribute(QualifiedName(nullAtom, (NSString *)kDataDetectorsAttributeResultKey, nullAtom), identifier); > > If we are defining these constants, why are we making them NSString instead of String, if they're just going to something that wants a String (presumably)? Done. >> Source/WebCore/page/Frame.h:193 >> + void setDataDetectionResults(NSArray *results) { m_dataDetectionResults = results; } > > Who calls this? I can't find a caller, but maybe that's just find-in-page being broken. It was missing the call in FrameLoader.cpp. I've fixed it.
Created attachment 269486 [details] Patch addressing comments
Comment on attachment 269486 [details] Patch addressing comments View in context: https://bugs.webkit.org/attachment.cgi?id=269486&action=review > Source/WebCore/editing/cocoa/DataDetection.mm:191 > +static NSString *constructURLStringForResult(DDResultRef currentResult, NSString *resultIdentifier, NSDate * referenceDate, NSTimeZone * referenceTimeZone, DataDetectorTypes detectionTypes) Some of these stars got detached from the right place. > Source/WebCore/editing/cocoa/DataDetection.mm:252 > + Weird extra newline > Source/WebCore/editing/cocoa/DataDetection.mm:263 > + node = startNode->parentNode(); > + while (node) { This should use the ancestor iterators that I believe we have (you can find them around, or I think Antti or Andreas added them). Might have to adopt them in a few places. > Source/WebCore/editing/cocoa/DataDetection.mm:339 > + BOOL containsOnlyWhiteSpace = YES; Pretty surprising to use BOOL instead of bool in WebCore. > Source/WebCore/editing/cocoa/DataDetection.mm:407 > + for (CFIndex i = 0; i < resultCount; i++) { > + DDResultRef result = (DDResultRef)CFArrayGetValueAtIndex(scannerResults.get(), i); I wonder if this could use for (..in..) (with a cast to NSArray) > Source/WebCore/editing/cocoa/DataDetection.mm:412 > + for (NSUInteger i = 0 ; i < [subresults count] ; i++) { Very much do not like the shadowed variable names. Please call one or both of them something better. > Source/WebCore/editing/cocoa/DataDetection.mm:434 > + for (CFIndex i = 0; i < resultCount; i++) { > + DDResultRef result = allResults[i].get(); It doesn't look like we use the index for anything except retrieving the result so this should use modern iteration over allResults.
Created attachment 269526 [details] Patch revisited Addressed your latest comments and fixed some bugs.
Attachment 269526 [details] did not pass style-queue: ERROR: Source/WebCore/editing/cocoa/DataDetection.mm:405: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269526 [details] Patch revisited View in context: https://bugs.webkit.org/attachment.cgi?id=269526&action=review > Source/WebCore/editing/cocoa/DataDetection.mm:58 > +- (NSString *)dataDetectorStringValue You could make this not a category but instead a static function that takes a NSIndexPath and and returns an NSString then you could use WebCore mechanisms instead of NSString and NSArray and stuff :D It would make me happier. But it's fine as is too :) > Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:91 > ++ (NSArray *) resultsFromCoreResults:(CFArrayRef)coreResults; no space after ) > Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:106 > +inline CFComparisonResult DDQueryOffsetCompare(DDQueryOffset o1, DDQueryOffset o2) Seems like this could cause trouble if DD ever made this non-inline. Please rename. Or something. > Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:152 > +SOFT_LINK_PRIVATE_FRAMEWORK_OPTIONAL(DataDetectorsCore) > +SOFT_LINK_CLASS(DataDetectorsCore, DDScannerResult) For some reason I thought you said that it was safe to link DDCore directly. Is it not? Or are we avoiding it for some performance-y reason? I think we prefer not soft-linking in most cases where we don't absolutely have to, and I think we have more performant alternatives (Darin at least has mentioned some in the past, but I don't remember the details).
(In reply to comment #33) > http://trac.webkit.org/changeset/195358 should fix that. No, the build is still broken, see https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/1814
Created attachment 269603 [details] New patch revisited I've addressed the latest comments.
Attachment 269603 [details] did not pass style-queue: ERROR: Source/WebCore/editing/cocoa/DataDetection.mm:413: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269603 [details] New patch revisited View in context: https://bugs.webkit.org/attachment.cgi?id=269603&action=review > Source/WebCore/platform/spi/cocoa/DataDetectorsCoreSPI.h:133 > +DDQueryRange DDResultGetQueryRangeForURLification(DDResultRef); Now that we are soft linking I probably don't need these here.
Comment on attachment 269603 [details] New patch revisited View in context: https://bugs.webkit.org/attachment.cgi?id=269603&action=review > Source/WebCore/ChangeLog:10 > + the dom by adding data detector links as appropriate. DOM > Source/WebCore/editing/cocoa/DataDetection.mm:183 > +static void removeResultLinksFromAnchor(Node *node, Node *nodeParent) Stars are on the wrong side. > Source/WebCore/editing/cocoa/DataDetection.mm:209 > + Node *child = children->item(0); // children change due to insertion, always use idx 0 This comment could use some love (at the very least capitalization and punctuation, maybe rewording).
Committed revision 195494.
Created attachment 269629 [details] Part 3
Attachment 269629 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269630 [details] Part 3 with build fixes.
Attachment 269630 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269674 [details] Part 3 with more build fixes
Attachment 269674 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269674 [details] Part 3 with more build fixes WebPageProxy.h includes "InteractionInformationAtPosition.h" unconditionally, it seems, but it’s now an iOS-only header.
Created attachment 269789 [details] Patch for final work
Attachment 269789 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 269807 [details] Final work with build fixes
Attachment 269807 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:139: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 269807 [details] Final work with build fixes View in context: https://bugs.webkit.org/attachment.cgi?id=269807&action=review > Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:138 > + result.dataDetectorResults = [unarchiver decodeObjectOfClasses:[NSSet setWithArray:@[ [NSArray class], getDDScannerResultClass()]] forKey:@"dataDetectorResults"]; Add a space before the ]. > Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:140 > + LOG_ERROR("Failed to decode NSArry of DDScanResult: %@", exception); Array. > Source/WebKit2/UIProcess/ios/WKActionSheetAssistant.mm:422 > + NSURL *targetURL = [NSURL URLWithString:positionInformation.url]; I think this should use our category method _web_URLWithString or whatever it's called. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3481 > + NSDictionary* context = nil; Star's on the wrong side :D > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3581 > + NSDictionary* context = nil; Ditto > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3590 > + if (dataForPreview) I think you're meaning to check ddResult here?
Created attachment 269828 [details] Testing the build
Attachment 269828 [details] did not pass style-queue: ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:144: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/Shared/ios/InteractionInformationAtPosition.mm:144: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed revision 195574.
Comment on attachment 269603 [details] New patch revisited View in context: https://bugs.webkit.org/attachment.cgi?id=269603&action=review > Source/WebCore/editing/cocoa/DataDetection.mm:194 > + RefPtr<NodeList> children = node->childNodes(); Note that this returns a "live" NodeList so it gets updated as the DOM gets modified. > Source/WebCore/editing/cocoa/DataDetection.mm:195 > + unsigned childCount = children->length(); Note that this traverses all the children and caches them in a Vector. However, that vector/cache gets invalidated as you traverse due to the DOM tree mutations. > Source/WebCore/editing/cocoa/DataDetection.mm:199 > + removeResultLinksFromAnchor(child, node); I may be misunderstanding but this loop seems wrong: 1. if child is a DDAnchor, removeResultLinksFromAnchor() will replace 'child' with the children of 'child'. Therefore, the list of children you are iterating on will change and your for loop will iterate over the children you have just moved up. 2. Also, since you have cached childCount, you will actually not loop over ALL the children. 3. Note that since removeResultLinksFromAnchor() causes DOM tree mutations and fires synchronous JS events. Nothing prevents JavaScript code from altering the DOM either and messing with the children you are iterating on AFAIK. > Source/WebCore/editing/cocoa/DataDetection.mm:204 > + childCount = children->length(); This is very inefficient. childNodes() returns a "live" NodeList. When you call length(), we have to traverse all the children to compute the length. Also, we cache the children during traversal into a Vector as we assume you are going to want to iterate over them next. However, as you keep modifying the DOM tree below, we keep invalidating the NodeList cache (e.g. the initial Vector has to be cleared). Since you always access the first child, this should probably be written like so: while(node->firstChild()) nodeParent->insertBefore(node->firstChild(), node, ASSERT_NO_EXCEPTION); > Source/WebCore/editing/cocoa/DataDetection.mm:210 > + nodeParent->insertBefore(child, node, ASSERT_NO_EXCEPTION); Also note that insertBefore() causes synchronous JS events to be fired (because the DOM tree is modified). Therefore, JavaScript code can be executed and mess with the children. If the JS were to remove children, you'd be in trouble because you cached childCount. This means that at some point, item(0) would return null.