Bug 152989

Summary: Add support for DataDetectors in WK (iOS)
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: WebKit2Assignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, commit-queue, ossy, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: iOS 9.0   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201144
Bug Depends on: 153244    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch2
none
Patch3
none
Patch4
thorton: review-
Patch5
none
Patch6
thorton: review+
Patch7
thorton: review+
Patch including detection.
none
Patch with detection and build fixes
none
Patch with detection and build fixes for iOS
none
More build fixes
none
More build fixes and style
none
More build fixes
none
Keep trying to build
none
Patch addressing comments
none
Patch revisited
none
New patch revisited
thorton: review+
Part 3
none
Part 3 with build fixes.
none
Part 3 with more build fixes
none
Patch for final work
none
Final work with build fixes
thorton: review+
Testing the build none

Description Enrica Casucci 2016-01-11 14:54:39 PST
This tracks the work required to support data detectors in WK2 on iOS.
Comment 1 Enrica Casucci 2016-01-11 15:12:09 PST
Created attachment 268717 [details]
Patch
Comment 2 WebKit Commit Bot 2016-01-11 15:14:29 PST
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.
Comment 3 Enrica Casucci 2016-01-11 17:05:37 PST
Created attachment 268732 [details]
Patch2
Comment 4 WebKit Commit Bot 2016-01-11 17:06:39 PST
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 5 Anders Carlsson 2016-01-11 17:09:07 PST
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?
Comment 6 Enrica Casucci 2016-01-11 17:17:14 PST
(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.
Comment 7 Enrica Casucci 2016-01-12 09:19:06 PST
Created attachment 268774 [details]
Patch3

Fixes build issues and addresses feedback.
Comment 8 WebKit Commit Bot 2016-01-12 09:21:00 PST
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.
Comment 9 Enrica Casucci 2016-01-12 10:58:53 PST
Created attachment 268782 [details]
Patch4

One more attempt to fix the build on all the platforms.
Comment 10 WebKit Commit Bot 2016-01-12 11:01:11 PST
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 11 Tim Horton 2016-01-15 10:20:40 PST
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 12 Tim Horton 2016-01-15 10:23:24 PST
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 13 Sam Weinig 2016-01-15 11:30:34 PST
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.
Comment 14 Enrica Casucci 2016-01-15 16:26:55 PST
Created attachment 269118 [details]
Patch5

The new property is now available on OS X and iOS.
Comment 15 WebKit Commit Bot 2016-01-15 16:30:17 PST
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 16 Tim Horton 2016-01-19 10:09:41 PST
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 17 Anders Carlsson 2016-01-19 10:11:07 PST
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.
Comment 18 Enrica Casucci 2016-01-19 10:14:15 PST
(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.
Comment 19 Enrica Casucci 2016-01-19 10:14:52 PST
(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.
Comment 20 Tim Horton 2016-01-19 10:18:51 PST
(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.
Comment 21 Enrica Casucci 2016-01-19 10:45:44 PST
Created attachment 269272 [details]
Patch6
Comment 22 WebKit Commit Bot 2016-01-19 10:48:23 PST
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 23 Tim Horton 2016-01-19 11:17:02 PST
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.
Comment 24 Enrica Casucci 2016-01-19 11:34:50 PST
Committed revision 195300.
Comment 25 Tim Horton 2016-01-19 11:53:20 PST
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)
Comment 26 Enrica Casucci 2016-01-19 14:22:29 PST
Created attachment 269295 [details]
Patch7

Fixes the 32 bit build.
Comment 27 WebKit Commit Bot 2016-01-19 14:23:35 PST
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.
Comment 28 Enrica Casucci 2016-01-19 14:59:28 PST
Build fixed before landing.
Committed revision 195317
Comment 29 Csaba Osztrogonác 2016-01-19 23:34:34 PST
(In reply to comment #28)
> Build fixed before landing.
> Committed revision 195317

It broke the Apple Mac cmae build.
Comment 30 Csaba Osztrogonác 2016-01-19 23:35:48 PST
(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
Comment 31 Csaba Osztrogonác 2016-01-20 02:59:56 PST
cmake fix landed in https://trac.webkit.org/changeset/195348
Comment 32 Csaba Osztrogonác 2016-01-20 04:57:28 PST
(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.
Comment 33 Alex Christensen 2016-01-20 11:28:35 PST
http://trac.webkit.org/changeset/195358 should fix that.
Comment 34 Enrica Casucci 2016-01-20 17:02:25 PST
Created attachment 269407 [details]
Patch including detection.
Comment 35 Tim Horton 2016-01-20 17:06:07 PST
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
Comment 36 Enrica Casucci 2016-01-20 19:06:48 PST
(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.
Comment 37 Enrica Casucci 2016-01-20 19:25:25 PST
Created attachment 269415 [details]
Patch with detection and build fixes
Comment 38 WebKit Commit Bot 2016-01-20 19:27:15 PST
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.
Comment 39 Enrica Casucci 2016-01-21 09:21:57 PST
Created attachment 269457 [details]
Patch with detection and build fixes for iOS
Comment 40 WebKit Commit Bot 2016-01-21 09:24:58 PST
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.
Comment 41 Enrica Casucci 2016-01-21 09:51:00 PST
Created attachment 269460 [details]
More build fixes
Comment 42 WebKit Commit Bot 2016-01-21 09:53:59 PST
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.
Comment 43 Enrica Casucci 2016-01-21 09:56:21 PST
Created attachment 269462 [details]
More build fixes and style
Comment 44 Enrica Casucci 2016-01-21 10:15:15 PST
Created attachment 269465 [details]
More build fixes
Comment 45 Tim Horton 2016-01-21 10:28:25 PST
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.
Comment 46 Enrica Casucci 2016-01-21 10:30:54 PST
Created attachment 269466 [details]
Keep trying to build
Comment 47 Enrica Casucci 2016-01-21 13:36:03 PST
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.
Comment 48 Enrica Casucci 2016-01-21 13:37:57 PST
Created attachment 269486 [details]
Patch addressing comments
Comment 49 Tim Horton 2016-01-21 13:53:05 PST
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.
Comment 50 Enrica Casucci 2016-01-21 18:06:29 PST
Created attachment 269526 [details]
Patch revisited

Addressed your latest comments and fixed some bugs.
Comment 51 WebKit Commit Bot 2016-01-21 18:09:39 PST
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 52 Tim Horton 2016-01-21 21:32:45 PST
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).
Comment 53 Csaba Osztrogonác 2016-01-22 04:12:52 PST
(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
Comment 54 Enrica Casucci 2016-01-22 13:52:02 PST
Created attachment 269603 [details]
New patch revisited

I've addressed the latest comments.
Comment 55 WebKit Commit Bot 2016-01-22 13:53:46 PST
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 56 Enrica Casucci 2016-01-22 13:55:38 PST
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 57 Tim Horton 2016-01-22 16:02:28 PST
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).
Comment 58 Enrica Casucci 2016-01-22 16:24:43 PST
Committed revision 195494.
Comment 59 Enrica Casucci 2016-01-22 18:15:13 PST
Created attachment 269629 [details]
Part 3
Comment 60 WebKit Commit Bot 2016-01-22 18:17:18 PST
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.
Comment 61 Enrica Casucci 2016-01-22 18:34:02 PST
Created attachment 269630 [details]
Part 3 with build fixes.
Comment 62 WebKit Commit Bot 2016-01-22 18:36:19 PST
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.
Comment 63 Enrica Casucci 2016-01-23 10:54:35 PST
Created attachment 269674 [details]
Part 3 with more build fixes
Comment 64 WebKit Commit Bot 2016-01-23 10:57:29 PST
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 65 Darin Adler 2016-01-24 12:24:55 PST
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.
Comment 66 Enrica Casucci 2016-01-25 13:59:52 PST
Created attachment 269789 [details]
Patch for final work
Comment 67 WebKit Commit Bot 2016-01-25 14:02:09 PST
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.
Comment 68 Enrica Casucci 2016-01-25 15:38:07 PST
Created attachment 269807 [details]
Final work with build fixes
Comment 69 WebKit Commit Bot 2016-01-25 15:39:08 PST
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 70 Tim Horton 2016-01-25 15:48:49 PST
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?
Comment 71 Enrica Casucci 2016-01-25 17:40:20 PST
Created attachment 269828 [details]
Testing the build
Comment 72 WebKit Commit Bot 2016-01-25 17:42:25 PST
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.
Comment 73 Enrica Casucci 2016-01-25 18:18:32 PST
Committed revision 195574.
Comment 74 Chris Dumez 2016-03-02 19:15:45 PST
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.