Bug 203872

Summary: WKWebView Find-in-page API
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit APIAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, commit-queue, ews-watchlist, jiewen_tan, kc, thorton
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=227364
Attachments:
Description Flags
WIP patch (still testing)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brady Eidson 2019-11-05 17:01:24 PST
WKWebView Find-in-page API

<rdar://problem/46971112>
Comment 1 Brady Eidson 2019-11-05 17:02:14 PST
Created attachment 382867 [details]
WIP patch (still testing)
Comment 2 Brady Eidson 2019-11-06 12:57:22 PST
Created attachment 382950 [details]
Patch
Comment 3 Brady Eidson 2019-11-06 15:07:32 PST
Created attachment 382964 [details]
Patch
Comment 4 Tim Horton 2019-11-06 16:59:59 PST
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 5 Tim Horton 2019-11-06 17:01:38 PST
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 6 Andy Estes 2019-11-06 17:10:13 PST
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.
Comment 7 Brady Eidson 2019-11-07 15:49:24 PST
Embarassed myself, as this is actually an earlier version of the patch with lots of obvious issues!

Rebaselining actual patch...
Comment 8 Brady Eidson 2019-11-07 15:51:21 PST
(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.
Comment 9 Tim Horton 2019-11-07 16:10:22 PST
(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
Comment 10 Brady Eidson 2019-11-08 09:31:08 PST
(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.
Comment 11 Brady Eidson 2019-11-08 09:33:40 PST
Created attachment 383134 [details]
Patch
Comment 12 Tim Horton 2019-11-08 09:38:51 PST
Doesn’t the SPI have a didFindString delegate callback that could be folded into this, and save a bunch of code?
Comment 13 Brady Eidson 2019-11-08 10:29:05 PST
(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 14 Tim Horton 2019-11-08 12:55:01 PST
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?
Comment 15 Brady Eidson 2019-11-08 13:50:14 PST
(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 16 Tim Horton 2019-11-08 13:55:42 PST
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.
Comment 17 Brady Eidson 2019-11-08 15:40:26 PST
Created attachment 383173 [details]
Patch
Comment 18 Brady Eidson 2019-11-08 17:19:25 PST
Created attachment 383184 [details]
Patch
Comment 19 Brady Eidson 2019-11-08 17:22:06 PST
(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 20 WebKit Commit Bot 2019-11-08 20:07:05 PST
Comment on attachment 383184 [details]
Patch

Clearing flags on attachment: 383184

Committed r252303: <https://trac.webkit.org/changeset/252303>
Comment 21 WebKit Commit Bot 2019-11-08 20:07:07 PST
All reviewed patches have been landed.  Closing bug.