Bug 191918 – [Cocoa] Add WKWebView SPI to trigger and remove data detection

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/36185051>

[Wenson Hsieh]

Created attachment 355532 [details]
Patch

[Wenson Hsieh]

Created attachment 355533 [details]
Fix the GTK build

[Tim Horton]

Comment on attachment 355533 [details]
Fix the GTK build

View in context: https://bugs.webkit.org/attachment.cgi?id=355533&action=review

> Source/WebKit/ChangeLog:10
> +        `-_removeAllDataDetectedContent:`, to allow internal WebKit clients to run data detection and add links to data

Does it actually remove the data detected *content*? or just the links?

Also, should this just be a side effect of turning Data Detectors off via the existing API??

[Wenson Hsieh]

Comment on attachment 355533 [details]
Fix the GTK build

View in context: https://bugs.webkit.org/attachment.cgi?id=355533&action=review

>> Source/WebKit/ChangeLog:10
>> +        `-_removeAllDataDetectedContent:`, to allow internal WebKit clients to run data detection and add links to data
> 
> Does it actually remove the data detected *content*? or just the links?
> 
> Also, should this just be a side effect of turning Data Detectors off via the existing API??

This just removes links added by data detectors, and resets Frame's m_dataDetectionResults.

I had considered making this a side effect of turning data detectors on or off, but it seems that this is only exposed on the web view configuration via dataDetectorTypes. IIUC, changes to the configuration aren't normally propagated to the web view after the web view is already initialized with the configuration.

An alternative is adding an SPI property on either WKWebView or WKPreferences that's something like _dataDetectorTypes, and we would add or remove data detectors in web content when this property is modified. WDYT?

[Tim Horton]

Comment on attachment 355533 [details]
Fix the GTK build

View in context: https://bugs.webkit.org/attachment.cgi?id=355533&action=review

>>> Source/WebKit/ChangeLog:10
>>> +        `-_removeAllDataDetectedContent:`, to allow internal WebKit clients to run data detection and add links to data
>> 
>> Does it actually remove the data detected *content*? or just the links?
>> 
>> Also, should this just be a side effect of turning Data Detectors off via the existing API??
> 
> This just removes links added by data detectors, and resets Frame's m_dataDetectionResults.
> 
> I had considered making this a side effect of turning data detectors on or off, but it seems that this is only exposed on the web view configuration via dataDetectorTypes. IIUC, changes to the configuration aren't normally propagated to the web view after the web view is already initialized with the configuration.
> 
> An alternative is adding an SPI property on either WKWebView or WKPreferences that's something like _dataDetectorTypes, and we would add or remove data detectors in web content when this property is modified. WDYT?

Aha, I see. I didn't realize it was a Configuration thing. No, I don't think you need to add more dataDetectorTypes equivalents. The API one is good.

In that case the only thing I object to is the word "content" in the method name.

[Wenson Hsieh]

Comment on attachment 355533 [details]
Fix the GTK build

View in context: https://bugs.webkit.org/attachment.cgi?id=355533&action=review

>>>> Source/WebKit/ChangeLog:10
>>>> +        `-_removeAllDataDetectedContent:`, to allow internal WebKit clients to run data detection and add links to data
>>> 
>>> Does it actually remove the data detected *content*? or just the links?
>>> 
>>> Also, should this just be a side effect of turning Data Detectors off via the existing API??
>> 
>> This just removes links added by data detectors, and resets Frame's m_dataDetectionResults.
>> 
>> I had considered making this a side effect of turning data detectors on or off, but it seems that this is only exposed on the web view configuration via dataDetectorTypes. IIUC, changes to the configuration aren't normally propagated to the web view after the web view is already initialized with the configuration.
>> 
>> An alternative is adding an SPI property on either WKWebView or WKPreferences that's something like _dataDetectorTypes, and we would add or remove data detectors in web content when this property is modified. WDYT?
> 
> Aha, I see. I didn't realize it was a Configuration thing. No, I don't think you need to add more dataDetectorTypes equivalents. The API one is good.
> 
> In that case the only thing I object to is the word "content" in the method name.

Sound good. How about one of: -_removeDataDetectedLinks: or -_removeDataDetectionResults:? Or...perhaps something more general, such as -_resetDataDetection:?

[Tim Horton]

Comment on attachment 355533 [details]
Fix the GTK build

View in context: https://bugs.webkit.org/attachment.cgi?id=355533&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:197
> +- (void)_removeAllDataDetectedContent:(dispatch_block_t)completion WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Also, why is one available on both platforms and the other only on iOS?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3396
> +void WebPage::removeAllDataDetectedContent(CallbackID callbackID)

Didn't Alex invent some new thing to replace CallbackID? See https://trac.webkit.org/changeset/237294/webkit

[Wenson Hsieh]

(In reply to Tim Horton from comment #7)
> Comment on attachment 355533 [details]
> Fix the GTK build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355533&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:197
> > +- (void)_removeAllDataDetectedContent:(dispatch_block_t)completion WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Also, why is one available on both platforms and the other only on iOS?

I first tried to make both available on macOS/iOS, but realized that WKDataDetectorTypes is iOS-only :/

> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3396
> > +void WebPage::removeAllDataDetectedContent(CallbackID callbackID)
> 
> Didn't Alex invent some new thing to replace CallbackID? See
> https://trac.webkit.org/changeset/237294/webkit

Nice, I'll give this a shot!

[Tim Horton]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 355533 [details]
> Fix the GTK build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355533&action=review
> 
> >>>> Source/WebKit/ChangeLog:10
> >>>> +        `-_removeAllDataDetectedContent:`, to allow internal WebKit clients to run data detection and add links to data
> >>> 
> >>> Does it actually remove the data detected *content*? or just the links?
> >>> 
> >>> Also, should this just be a side effect of turning Data Detectors off via the existing API??
> >> 
> >> This just removes links added by data detectors, and resets Frame's m_dataDetectionResults.
> >> 
> >> I had considered making this a side effect of turning data detectors on or off, but it seems that this is only exposed on the web view configuration via dataDetectorTypes. IIUC, changes to the configuration aren't normally propagated to the web view after the web view is already initialized with the configuration.
> >> 
> >> An alternative is adding an SPI property on either WKWebView or WKPreferences that's something like _dataDetectorTypes, and we would add or remove data detectors in web content when this property is modified. WDYT?
> > 
> > Aha, I see. I didn't realize it was a Configuration thing. No, I don't think you need to add more dataDetectorTypes equivalents. The API one is good.
> > 
> > In that case the only thing I object to is the word "content" in the method name.
> 
> Sound good. How about one of: -_removeDataDetectedLinks: or

Probably that one ^^ (slightly humorous because one of the subtypes is WKDataDetectorTypeLink but I don't think that matters, and the API doc does say "add links around...".

> -_removeDataDetectionResults:? Or...perhaps something more general, such as
> -_resetDataDetection:?

[Wenson Hsieh]

Created attachment 355554 [details]
Patch for EWS

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #8)
> (In reply to Tim Horton from comment #7)
> > Comment on attachment 355533 [details]
> > Fix the GTK build
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355533&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:197
> > > +- (void)_removeAllDataDetectedContent:(dispatch_block_t)completion WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > Also, why is one available on both platforms and the other only on iOS?
> 
> I first tried to make both available on macOS/iOS, but realized that
> WKDataDetectorTypes is iOS-only :/
> 
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3396
> > > +void WebPage::removeAllDataDetectedContent(CallbackID callbackID)
> > 
> > Didn't Alex invent some new thing to replace CallbackID? See
> > https://trac.webkit.org/changeset/237294/webkit
> 
> Nice, I'll give this a shot!

This mostly just worked, but needs a small tweak in WebKit/scripts/webkit/messages.py to support declaring async completion handlers with no arguments, like so:

    DetectDataInAllFrames(uint64_t types) -> () Async
    RemoveDataDetectedLinks() -> () Async

[Wenson Hsieh]

Created attachment 355555 [details]
Patch for EWS

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #11)
> (In reply to Wenson Hsieh from comment #8)
> > (In reply to Tim Horton from comment #7)
> > > Comment on attachment 355533 [details]
> > > Fix the GTK build
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=355533&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:197
> > > > +- (void)_removeAllDataDetectedContent:(dispatch_block_t)completion WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > > 
> > > Also, why is one available on both platforms and the other only on iOS?
> > 
> > I first tried to make both available on macOS/iOS, but realized that
> > WKDataDetectorTypes is iOS-only :/
> > 
> > > 
> > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3396
> > > > +void WebPage::removeAllDataDetectedContent(CallbackID callbackID)
> > > 
> > > Didn't Alex invent some new thing to replace CallbackID? See
> > > https://trac.webkit.org/changeset/237294/webkit
> > 
> > Nice, I'll give this a shot!
> 
> This mostly just worked, but needs a small tweak in
> WebKit/scripts/webkit/messages.py to support declaring async completion
> handlers with no arguments, like so:
> 
>     DetectDataInAllFrames(uint64_t types) -> () Async
>     RemoveDataDetectedLinks() -> () Async

I realized that I actually don't need this messages.py adjustment if I use this async CompletionHandler as an opportunity to just send back a DataDetectionResult, instead of propagating the DataDetectionResult as a separate IPC message. This has the added bonus of eliminating the need for an extra IPC message, but does require a small tweak in DataDetectionResult to make DataDetectionResult use the new decoding format:

    static std::optional<DataDetectionResult> decode(IPC::Decoder&);

instead of:

    static bool decode(IPC::Decoder&, DataDetectionResult&);

[Wenson Hsieh]

Created attachment 355564 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 355564 [details]
Patch for EWS

Clearing flags on attachment: 355564

Committed r238471: <https://trac.webkit.org/changeset/238471>