Bug 203872 – WKWebView Find-in-page API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brady Eidson]

WKWebView Find-in-page API

<rdar://problem/46971112>

[Brady Eidson]

Created attachment 382867 [details]
WIP patch (still testing)

[Brady Eidson]

Created attachment 382950 [details]
Patch

[Brady Eidson]

Created attachment 382964 [details]
Patch

[Tim Horton]

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?

[Tim Horton]

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

[Andy Estes]

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.

[Brady Eidson]

Embarassed myself, as this is actually an earlier version of the patch with lots of obvious issues!

Rebaselining actual patch...

[Brady Eidson]

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

[Tim Horton]

(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

[Brady Eidson]

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

[Brady Eidson]

Created attachment 383134 [details]
Patch

[Tim Horton]

Doesn’t the SPI have a didFindString delegate callback that could be folded into this, and save a bunch of code?

[Brady Eidson]

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

[Tim Horton]

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?

[Brady Eidson]

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

[Tim Horton]

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.

[Brady Eidson]

Created attachment 383173 [details]
Patch

[Brady Eidson]

Created attachment 383184 [details]
Patch

[Brady Eidson]

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

[WebKit Commit Bot]

Comment on attachment 383184 [details]
Patch

Clearing flags on attachment: 383184

Committed r252303: <https://trac.webkit.org/changeset/252303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.