WKWebView Find-in-page API <rdar://problem/46971112>
Created attachment 382867 [details] WIP patch (still testing)
Created attachment 382950 [details] Patch
Created attachment 382964 [details] Patch
Comment on attachment 382964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382964&action=review > Source/WebKit/UIProcess/API/Cocoa/WKFindConfiguration.h:33 > +typedef NS_ENUM(NSInteger, WKFindDirection) { > + WKFindDirectionForward, > + WKFindDirectionBackward, > +} WK_API_AVAILABLE(macos(10.16), ios(14.0)); What is this for??? > Source/WebKit/UIProcess/API/Cocoa/WKFindConfiguration.h:35 > +WK_CLASS_AVAILABLE(macos(10.16), ios(14.0)) TBA?
Comment on attachment 382964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382964&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:366 > +WK_API_AVAILABLE(macos(10.16)) TBA > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4774 > + I am weirded out by this newline, but also by the unnecessary temporary. > Source/WebKit/UIProcess/WebPageProxy.h:340 > +typedef GenericCallback<bool> BoolCallback; HOW IS THIS THE FIRST TIME
Comment on attachment 382964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382964&action=review > Source/WebKit/ChangeLog:8 > + Covered by existing API tests. The existing tests don't create WKFindConfigurations, so maybe we should have a new API test that tests the new API explicitly rather than relying on the SPI and API behaving the same. > Source/WebKit/UIProcess/API/Cocoa/WKFindConfiguration.mm:38 > + self.backwards = NO; > + self.caseSensitive = NO; > + self.wraps = YES; Usually we set synthesized properties' ivars directly (e.g., _backwards vs. self.backwards). > Source/WebKit/UIProcess/API/Cocoa/WKFindConfiguration.mm:48 > + findConfiguration.backwards = self.backwards; > + findConfiguration.caseSensitive = self.caseSensitive; > + findConfiguration.wraps = self.wraps; Ditto. > Source/WebKit/UIProcess/API/Cocoa/WKFindResult.mm:50 > + findResult->_matchFound = self.matchFound; findResult->_matchFound = _matchFound? > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4756 > + if (!configuration.caseSensitive) > + findOptions |= WebKit::FindOptionsCaseInsensitive; This one makes sense to me (mapping case-sensitive in WKFindConfiguration to case-insensitive in FindOptions). > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4760 > + if (!configuration.backwards) > + findOptions |= WebKit::FindOptionsBackwards; > + if (!configuration.wraps) > + findOptions |= WebKit::FindOptionsWrapAround; ... but these two don't. If backwards is false shouldn't we not be setting WebKit::FindOptionsBackwards, for instance? > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4776 > + _page->findString(string, toFindOptions(configuration), 1, [handler = makeBlockPtr(completionHandler)](bool found, WebKit::CallbackBase::Error error) { > + WKFindResult *result = [[[WKFindResult alloc] _initWithMatchFound:(error == WebKit::CallbackBase::Error::None && found)] autorelease]; > + > + handler(result); > + }); In the SPI version of this method, when there is a _customContentView, we call -web_findString:options:maxCount: instead of WebPage::findString(). I think we want to do that in the API too. Maybe more things can be shared between _findString and findString? > Source/WebKit/UIProcess/WebPageProxy.cpp:3708 > + Optional<CallbackID> callbackID; > + if (callbackFunction) > + callbackID = m_callbacks.put(WTFMove(callbackFunction), m_process->throttler().backgroundActivity("WebPageProxy::findString"_s)); > + > + m_process->send(Messages::WebPage::FindString(string, options, maxMatchCount, callbackID), m_webPageID); If we made the Messages::WebPage::FindString async then we wouldn't need to do the callback dance.
Embarassed myself, as this is actually an earlier version of the patch with lots of obvious issues! Rebaselining actual patch...
(In reply to Andy Estes from comment #6 > > Source/WebKit/UIProcess/WebPageProxy.cpp:3708 > > + Optional<CallbackID> callbackID; > > + if (callbackFunction) > > + callbackID = m_callbacks.put(WTFMove(callbackFunction), m_process->throttler().backgroundActivity("WebPageProxy::findString"_s)); > > + > > + m_process->send(Messages::WebPage::FindString(string, options, maxMatchCount, callbackID), m_webPageID); > > If we made the Messages::WebPage::FindString async then we wouldn't need to > do the callback dance. Not at all clear what you mean. It is already async, and we now need a callback.
(In reply to Brady Eidson from comment #8) > (In reply to Andy Estes from comment #6 > > > Source/WebKit/UIProcess/WebPageProxy.cpp:3708 > > > + Optional<CallbackID> callbackID; > > > + if (callbackFunction) > > > + callbackID = m_callbacks.put(WTFMove(callbackFunction), m_process->throttler().backgroundActivity("WebPageProxy::findString"_s)); > > > + > > > + m_process->send(Messages::WebPage::FindString(string, options, maxMatchCount, callbackID), m_webPageID); > > > > If we made the Messages::WebPage::FindString async then we wouldn't need to > > do the callback dance. > > Not at all clear what you mean. It is already async, and we now need a > callback. Sure it is, he means turn FindString(String string, uint32_t findOptions, unsigned maxMatchCount) into FindString(String string, uint32_t findOptions, unsigned maxMatchCount) -> (bool didFindString) Async and use the autogenerated reply callback
(In reply to Tim Horton from comment #9) > (In reply to Brady Eidson from comment #8) > > (In reply to Andy Estes from comment #6 > > > > Source/WebKit/UIProcess/WebPageProxy.cpp:3708 > > > > + Optional<CallbackID> callbackID; > > > > + if (callbackFunction) > > > > + callbackID = m_callbacks.put(WTFMove(callbackFunction), m_process->throttler().backgroundActivity("WebPageProxy::findString"_s)); > > > > + > > > > + m_process->send(Messages::WebPage::FindString(string, options, maxMatchCount, callbackID), m_webPageID); > > > > > > If we made the Messages::WebPage::FindString async then we wouldn't need to > > > do the callback dance. > > > > Not at all clear what you mean. It is already async, and we now need a > > callback. > > Sure it is, he means turn > > FindString(String string, uint32_t findOptions, unsigned maxMatchCount) > > into > > FindString(String string, uint32_t findOptions, unsigned maxMatchCount) -> > (bool didFindString) Async > > and use the autogenerated reply callback Oh, right. I quite on purpose didn't do that because it's still used by SPIs that don't need a callback.
Created attachment 383134 [details] Patch
Doesn’t the SPI have a didFindString delegate callback that could be folded into this, and save a bunch of code?
(In reply to Tim Horton from comment #12) > Doesn’t the SPI have a didFindString delegate callback that could be folded > into this, and save a bunch of code? One of them, yes! But there might be more SPIs than you realize!
Comment on attachment 383134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383134&action=review > Source/WebKit/UIProcess/API/Cocoa/WKFindResult.mm:33 > + return [self _initWithMatchFound:NO]; Why's this a thing? Should it be NS_UNAVAILABLE and return nil? > Source/WebKit/UIProcess/API/Cocoa/WKFindResultInternal.h:29 > +@interface WKFindResult () { > +} Why the braces?
(In reply to Tim Horton from comment #14) > Comment on attachment 383134 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383134&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKFindResult.mm:33 > > + return [self _initWithMatchFound:NO]; > > Why's this a thing? Should it be NS_UNAVAILABLE and return nil? Sure. > > > Source/WebKit/UIProcess/API/Cocoa/WKFindResultInternal.h:29 > > +@interface WKFindResult () { > > +} > > Why the braces? Compiler made me do it?
Comment on attachment 383134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383134&action=review >>> Source/WebKit/UIProcess/API/Cocoa/WKFindResultInternal.h:29 >>> +} >> >> Why the braces? > > Compiler made me do it? It certainly did not.
Created attachment 383173 [details] Patch
Created attachment 383184 [details] Patch
(In reply to Tim Horton from comment #16) > Comment on attachment 383134 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383134&action=review > > >>> Source/WebKit/UIProcess/API/Cocoa/WKFindResultInternal.h:29 > >>> +} > >> > >> Why the braces? > > > > Compiler made me do it? > > It certainly did not. I added them at one point to fix a compiler error. Removing them now does not reintroduce any compiler error. So... yay.
Comment on attachment 383184 [details] Patch Clearing flags on attachment: 383184 Committed r252303: <https://trac.webkit.org/changeset/252303>
All reviewed patches have been landed. Closing bug.