Add support for DataDetectors in WK (iOS)

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

This tracks the work required to support data detectors in WK2 on iOS.

[Enrica Casucci]

Created attachment 268717 [details]
Patch

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 268732 [details]
Patch2

[WebKit Commit Bot]

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.

[Anders Carlsson]

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?

[Enrica Casucci]

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

[Enrica Casucci]

Created attachment 268774 [details]
Patch3

Fixes build issues and addresses feedback.

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 268782 [details]
Patch4

One more attempt to fix the build on all the platforms.

[WebKit Commit Bot]

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.

[Tim Horton]

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.

[Tim Horton]

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.

[Sam Weinig]

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.

[Enrica Casucci]

Created attachment 269118 [details]
Patch5

The new property is now available on OS X and iOS.

[WebKit Commit Bot]

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.

[Tim Horton]

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.

[Anders Carlsson]

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.

[Enrica Casucci]

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

[Enrica Casucci]

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

[Tim Horton]

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

[Enrica Casucci]

Created attachment 269272 [details]
Patch6

[WebKit Commit Bot]

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.

[Tim Horton]

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.

[Enrica Casucci]

Committed revision 195300.

[Tim Horton]

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)

[Enrica Casucci]

Created attachment 269295 [details]
Patch7

Fixes the 32 bit build.

[WebKit Commit Bot]

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.

[Enrica Casucci]

Build fixed before landing.
Committed revision 195317

[Csaba Osztrogonác]

(In reply to comment #28)
> Build fixed before landing.
> Committed revision 195317

It broke the Apple Mac cmae build.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

cmake fix landed in https://trac.webkit.org/changeset/195348

[Csaba Osztrogonác]

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

[Alex Christensen]

http://trac.webkit.org/changeset/195358 should fix that.

[Enrica Casucci]

Created attachment 269407 [details]
Patch including detection.

[Tim Horton]

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

[Enrica Casucci]

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

[Enrica Casucci]

Created attachment 269415 [details]
Patch with detection and build fixes

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 269457 [details]
Patch with detection and build fixes for iOS

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 269460 [details]
More build fixes

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 269462 [details]
More build fixes and style

[Enrica Casucci]

Created attachment 269465 [details]
More build fixes

[Tim Horton]

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.

[Enrica Casucci]

Created attachment 269466 [details]
Keep trying to build

[Enrica Casucci]

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.

[Enrica Casucci]

Created attachment 269486 [details]
Patch addressing comments

[Tim Horton]

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.

[Enrica Casucci]

Created attachment 269526 [details]
Patch revisited

Addressed your latest comments and fixed some bugs.

[WebKit Commit Bot]

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.

[Tim Horton]

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

[Csaba Osztrogonác]

(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

[Enrica Casucci]

Created attachment 269603 [details]
New patch revisited

I've addressed the latest comments.

[WebKit Commit Bot]

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.

[Enrica Casucci]

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.

[Tim Horton]

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

[Enrica Casucci]

Committed revision 195494.

[Enrica Casucci]

Created attachment 269629 [details]
Part 3

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 269630 [details]
Part 3 with build fixes.

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 269674 [details]
Part 3 with more build fixes

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Enrica Casucci]

Created attachment 269789 [details]
Patch for final work

[WebKit Commit Bot]

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.

[Enrica Casucci]

Created attachment 269807 [details]
Final work with build fixes

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Enrica Casucci]

Created attachment 269828 [details]
Testing the build

[WebKit Commit Bot]

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.

[Enrica Casucci]

Committed revision 195574.

[Chris Dumez]

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.