Bug 131776 – Find on page - extend API by providing highlighted match index

• Component: WebKit2
• Status: RESOLVED FIXED

[Alice Liu]

The API of WebPage provides a way to find and visually indicate a string in the webpage, which is the "findString" function. In response, the WebProcess responds via a "DidFindString" message containing the string, and number of matches.  We could expand the data in this response to also include, if applicable, the index of the matched string among all matches.  Since each subsequent call to "findString" results in a different occurrence being highlighted, this highlighted index data is relevant to the response.

[Alice Liu]

Created attachment 229506 [details]
Patch

[Tim Horton]

Comment on attachment 229506 [details]
Patch

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

> Source/WebKit2/ChangeLog:31
> +            Pull out the calculation of matchCount because it was only being done if the highlight or overlay flag bits

This was intentional, I believe. We should first understand whether there is any significant performance impact for clients who specify neither of those flags *and* don't need the total match count (and also whether there are any such clients). If we decide that there are and it is a significant impact, we could add a new find option that means "I really want an accurate count, not just the next string"

> Source/WebKit2/ChangeLog:36
> +        to be less than match count. In other words this is like performing a mathematical modulo operation, except it's
> +        not as inefficient or undefined for negative numbers as "%" operator is.

this last sentence of commentary is a bit unnecessary :)

> Source/WebKit2/UIProcess/API/C/WKPageFindClient.h:35
> -typedef void (*WKPageDidFindStringCallback)(WKPageRef page, WKStringRef string, unsigned matchCount, const void* clientInfo);
> +typedef void (*WKPageDidFindStringCallback)(WKPageRef page, WKStringRef string, unsigned matchCount, int matchIndex, const void* clientInfo);

I don't think we can change the C API willy-nilly like this.

I wonder if we should add a new client callback/WKFindDelegate callback (DidIndicateMatch or something) instead of changing the old one. In any case you'll have to version this somehow.

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:134
> +        if (pluginView)

aforementioned perf concerns

> Source/WebKit2/WebProcess/WebPage/FindController.h:99
> +    // Index value is -1 if not showing find indicator.
> +    int m_foundStringMatchIndex;

The name doesn't sound like it has anything to do with the indicator. Also, isn't it updated and send correctly even if not using the indicator?

[Alice Liu]

(In reply to comment #2)
> (From update of attachment 229506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229506&action=review
> 
> > Source/WebKit2/ChangeLog:31
> > +            Pull out the calculation of matchCount because it was only being done if the highlight or overlay flag bits
> 
> This was intentional, I believe. We should first understand whether there is any significant performance impact for clients who specify neither of those flags *and* don't need the total match count (and also whether there are any such clients). If we decide that there are and it is a significant impact, we could add a new find option that means "I really want an accurate count, not just the next string"

I like the idea of adding a new FindOption, but i'm not sure if describing it as "really wanting an accurate count" is the best way to say it.  That flag wouldn't mean much or result in an accurate count if the actual count exceeds the client-provided maxCount. I think we want something like FindOptionDetermineMatchIndex? It would fit with the other find options that describe actions. 

> 
> > Source/WebKit2/UIProcess/API/C/WKPageFindClient.h:35
> > -typedef void (*WKPageDidFindStringCallback)(WKPageRef page, WKStringRef string, unsigned matchCount, const void* clientInfo);
> > +typedef void (*WKPageDidFindStringCallback)(WKPageRef page, WKStringRef string, unsigned matchCount, int matchIndex, const void* clientInfo);
> 
> I don't think we can change the C API willy-nilly like this.
> 
> I wonder if we should add a new client callback/WKFindDelegate callback (DidIndicateMatch or something) instead of changing the old one. In any case you'll have to version this somehow.

ah, yes, i'll version.  Do you prefer adding DidIndicateMatch or altering DidFindString?  I somewhat prefer altering DidFindString because the data provided is pertinent to the found string.  The FindOptionsShowFindIndicator flag need not be on for the matchIndex data to be applicable.  

> 
> > Source/WebKit2/WebProcess/WebPage/FindController.cpp:134
> > +        if (pluginView)
> 
> aforementioned perf concerns

Unintentional perf concerns would be eliminated by a new FindOption opting into tracking total matches in my response above. 

> 
> > Source/WebKit2/WebProcess/WebPage/FindController.h:99
> > +    // Index value is -1 if not showing find indicator.
> > +    int m_foundStringMatchIndex;
> 
> The name doesn't sound like it has anything to do with the indicator. Also, isn't it updated and send correctly even if not using the indicator?

True, and yes.  It would be more accurate to say the value is -1 if not found or if match number of matches exceed max.  Regarding the variable name, since the match index is trackable regardless of whether the match is being indicated, I would say the variable name doesn't need to have "indicate" in its name. 


New patch coming.  Thanks Tim.

[Alice Liu]

Created attachment 229619 [details]
Patch

[Tim Horton]

Comment on attachment 229619 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:708
> -        virtual void didFindString(WebPageProxy* page, const String& string, uint32_t matchCount) override
> +        virtual void didFindString(WebPageProxy* page, const String& string, uint32_t matchCount, int32_t matchIndex) override

You don't seem to have revved the C API. That said, mitz pointed out that we can probably just ignore the C API (don't change it to support the new parameter at all) for this new feature.

> Source/WebKit2/UIProcess/API/Cocoa/_WKFindDelegate.h:37
> +- (void)_webView:(WKWebView *)webView didFindMatches:(NSUInteger)matches forString:(NSString *)string indicatingMatchIndex:(NSInteger)matchIndex;

should this selector name still have "indicating" in it, now that it's possible to DetermineMatchIndex without ShowFindIndicator?

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:-138
> -                matchCount = pluginView->countFindMatches(string, core(options), maxMatchCount + 1);

Should you still be removing this? Does it not break PDF find-in-page on desktop (since I assume DetermineMatchIndex is not passed in).

[Alice Liu]

Created attachment 229661 [details]
Patch

[Tim Horton]

Comment on attachment 229661 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:136
> +            if (pluginView)
> +                matchCount = pluginView->countFindMatches(string, core(options), maxMatchCount + 1);

I'm not sure this comment from last time got fixed (below) but I can't tell because your patch doesn't let me expand context for some reason :D

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 229661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229661&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/FindController.cpp:136
> > +            if (pluginView)
> > +                matchCount = pluginView->countFindMatches(string, core(options), maxMatchCount + 1);
> 
> I'm not sure this comment from last time got fixed (below) but I can't tell because your patch doesn't let me expand context for some reason :D

It did, I just misread. Now we countFindMatches twice in the PluginView case, though.

Also, WKPage is still touched; is that intentional? Seems wrong.

[Tim Horton]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 229661 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=229661&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/FindController.cpp:136
> > > +            if (pluginView)
> > > +                matchCount = pluginView->countFindMatches(string, core(options), maxMatchCount + 1);
> > 
> > I'm not sure this comment from last time got fixed (below) but I can't tell because your patch doesn't let me expand context for some reason :D
> 
> It did, I just misread. Now we countFindMatches twice in the PluginView case, though.
> 
> Also, WKPage is still touched; is that intentional? Seems wrong.

No, actually, that's OK.

[Tim Horton]

http://trac.webkit.org/changeset/167519