Summary: | [Cocoa] Add WKWebView SPI to trigger and remove data detection | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aestes, bdakin, commit-queue, thorton, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Other | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2018-11-22 19:09:21 PST
Created attachment 355532 [details]
Patch
Created attachment 355533 [details]
Fix the GTK build
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?? 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? 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. 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:? 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 (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! (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:? Created attachment 355554 [details]
Patch for EWS
(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 Created attachment 355555 [details]
Patch for EWS
(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&); Created attachment 355564 [details]
Patch for EWS
Comment on attachment 355564 [details] Patch for EWS Clearing flags on attachment: 355564 Committed r238471: <https://trac.webkit.org/changeset/238471> |