Bug 106834 – Add new API for text search in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

This is a new API that implements text search without any user interface and provides all the necessary information to implement a custom UI.

[Enrica Casucci]

<rdar://problem/12597159>

[Enrica Casucci]

Created attachment 182642 [details]
Patch

[WebKit Review Bot]

Attachment 182642 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/FindController.h:76:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/WebPage/FindController.h:76:  The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/C/WKPage.h:331:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 182642 [details]
Patch

Attachment 182642 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15875707

[Benjamin Poulain]

Comment on attachment 182642 [details]
Patch

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

No clue about the patch, just some small comments:

> Source/WebCore/page/Page.cpp:575
> +    if (target.isEmpty() || !mainFrame())

It is safe to call Editor::countMatchesForText() with an empty target.

Since it is uncommon, you may not want the branch here.

> Source/WebCore/page/Page.cpp:646
> +        matches += frame->editor()->countMatchesForText(target, 0, options, limit ? (limit - matches) : 0, true, 0);

Naming the two null literals would be nice.

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:201
> +    if (string.isEmpty()) {
> +        m_webPage->send(Messages::WebPageProxy::DidFindStringMatches(string, Vector<Vector<IntRect> >(), 0));
> +        return;
> +    }

I have the feeling this would be better handled on the WebPageProxy to avoid a round trip by the WebProcess.

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:210
> +    if (m_findMatches.isEmpty()) {
> +        m_webPage->send(Messages::WebPageProxy::DidFindStringMatches(string, Vector<Vector<IntRect> >(), 0));
> +        return;
> +    }

Is this branch really needed? Shouldn't the code bellow give you the exact same outcome?

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:285
> +    Frame* frame = m_findMatches[matchIndex]->startContainer()->document()->frame();
> +    if (!frame)
> +        return;
> +    frame->selection()->setSelection(VisibleSelection(m_findMatches[matchIndex].get()));

if (Frame* frame = m_findMatches[matchIndex]->startContainer()->document()->frame())
    frame->selection()->setSelection(VisibleSelection(m_findMatches[matchIndex].get()));

?

[Simon Fraser (smfr)]

Comment on attachment 182642 [details]
Patch

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

It's mostly good but I'd like to see one more round.

> Source/WebCore/editing/Editor.cpp:2745
> +unsigned Editor::countMatchesForText(const String& target, Range* range, FindOptions options, unsigned limit, bool markMatches, Vector<RefPtr<Range> >* matches)

Could Range* be a const Range*, to make it clear that it's an "in" param and unchanged by this function?

> Source/WebCore/page/Page.cpp:579
> +    Frame* selectedFrame = 0;

I think this would be clearer as frameWithSelection.

> Source/WebCore/page/Page.cpp:585
> +    do {
> +        frame->editor()->countMatchesForText(target, 0, options, limit ? (limit - matchRanges->size()) : 0, true, matchRanges);
> +        if (frame->selection()->isRange())
> +            selectedFrame = frame;
> +        frame = incrementFrame(frame, true, false);
> +    } while (frame);

frame = incrementFrame(frame, true, false); is pretty unreadable. I think it would be clearer to use the FrameTree functions directly in a for loop.

> Source/WebCore/page/Page.cpp:599
> +        for (size_t i = 0; i < matchRanges->size(); ++i) {
> +            ExceptionCode ec;
> +            if (selectedRange->compareBoundaryPoints(Range::START_TO_END, matchRanges->at(i).get(), ec) < 0) {
> +                indexForSelection = i;
> +                break;
> +            }
> +        }

Not sure what this is doing. I'm confused by the fact that matchRanges may contain Ranges from different frames. Why compareBoundaryPoints() < 0?

> Source/WebCore/page/Page.h:239
> +        void findStringMatchingRanges(const String&, FindOptions, int maxCount, Vector<RefPtr<Range> >*, int& indexForSelection);

indexForSelection is an index into the Vector<RefPtr<Range> >? Would be nice to clarify that with a comment or clearer naming.

> Source/WebKit2/UIProcess/WebFindClient.h:49
> +    void didFindStringMatches(WebPageProxy*, const String&, ImmutableArray*, int);
> +    void didGetImageForMatchResult(WebPageProxy*, WebImage*, uint32_t);

Why int for one and uint32_t for the other? What does this index mean?

> Source/WebKit2/UIProcess/API/C/WKPage.h:469
>  WK_EXPORT void WKPageCountStringMatches(WKPageRef page, WKStringRef string, WKFindOptions findOptions, unsigned maxMatchCount);
> +WK_EXPORT void WKPageFindStringMatches(WKPageRef page, WKStringRef string, WKFindOptions findOptions, unsigned maxMatchCount);

The fact that these have the same signature makes me wonder if we should just piggyback on WKPageCountStringMatches, and use a WKFindOption, or presence of a FindMatchesClient, to control the behavior?

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:271
> +    if (!getFindIndicatorBitmapAndRect(frame, handle, selectionRect))
> +        return;

Don't you want to reset the selection before returning, and maybe send the didGetImage message with a null image?

> Source/WebKit2/WebProcess/WebPage/FindController.h:55
> +    void findStringMatches(const String&, FindOptions, unsigned maxMatchCount);
> +    void getImageForFindMatch(uint32_t matchIndex);

Why the different unsigned types?

> Source/WebKit2/WebProcess/WebPage/FindController.h:88
> +    Vector<RefPtr<WebCore::Range> > m_findMatches;

How long-lived is the FindController? Is it OK to hold onto these Ranges for its lifetime? Should hideFindUI or something clear them?

[Enrica Casucci]

Comment on attachment 182642 [details]
Patch

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

I'll post a new patch that addresses the feedback.

>> Source/WebCore/editing/Editor.cpp:2745
>> +unsigned Editor::countMatchesForText(const String& target, Range* range, FindOptions options, unsigned limit, bool markMatches, Vector<RefPtr<Range> >* matches)
> 
> Could Range* be a const Range*, to make it clear that it's an "in" param and unchanged by this function?

Sure. It was already like this I just added the matches parameter.

>> Source/WebCore/page/Page.cpp:575
>> +    if (target.isEmpty() || !mainFrame())
> 
> It is safe to call Editor::countMatchesForText() with an empty target.
> 
> Since it is uncommon, you may not want the branch here.

Ok.

>> Source/WebCore/page/Page.cpp:579
>> +    Frame* selectedFrame = 0;
> 
> I think this would be clearer as frameWithSelection.

No problem.

>> Source/WebCore/page/Page.cpp:585
>> +    } while (frame);
> 
> frame = incrementFrame(frame, true, false); is pretty unreadable. I think it would be clearer to use the FrameTree functions directly in a for loop.

I used the same style used in findString that used incrementFrame.

>> Source/WebCore/page/Page.cpp:599
>> +        }
> 
> Not sure what this is doing. I'm confused by the fact that matchRanges may contain Ranges from different frames. Why compareBoundaryPoints() < 0?

I want to return the first matching range after the current selection.

>> Source/WebCore/page/Page.cpp:646
>> +        matches += frame->editor()->countMatchesForText(target, 0, options, limit ? (limit - matches) : 0, true, 0);
> 
> Naming the two null literals would be nice.

These are two null pointers. What can I do here?

>> Source/WebCore/page/Page.h:239
>> +        void findStringMatchingRanges(const String&, FindOptions, int maxCount, Vector<RefPtr<Range> >*, int& indexForSelection);
> 
> indexForSelection is an index into the Vector<RefPtr<Range> >? Would be nice to clarify that with a comment or clearer naming.

This value represents the default selected find match. This is 0 if there is no user selection or it is the index of the first matching range after the user selection. If there are no matching ranges after the user selection the value is -1. There is an explanation in the WebKit2/ChangeLog but I'll add a comment in the code as well.

>> Source/WebKit2/UIProcess/WebFindClient.h:49
>> +    void didGetImageForMatchResult(WebPageProxy*, WebImage*, uint32_t);
> 
> Why int for one and uint32_t for the other? What does this index mean?

I use int for the case where the value can be -1 (see comment above) . When the value is passed in to indicate a find match it can never be negative.

>> Source/WebKit2/UIProcess/API/C/WKPage.h:331
>> +enum { kWKPageFindMatchesClientCurrentVersion = 0 };
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

I followed the style of the other existing enums in this file.

>> Source/WebKit2/UIProcess/API/C/WKPage.h:469
>> +WK_EXPORT void WKPageFindStringMatches(WKPageRef page, WKStringRef string, WKFindOptions findOptions, unsigned maxMatchCount);
> 
> The fact that these have the same signature makes me wonder if we should just piggyback on WKPageCountStringMatches, and use a WKFindOption, or presence of a FindMatchesClient, to control the behavior?

I thought about this, but I decided against, since this API also requires a different client to be used to receive callbacks.

>> Source/WebKit2/WebProcess/WebPage/FindController.cpp:201
>> +    }
> 
> I have the feeling this would be better handled on the WebPageProxy to avoid a round trip by the WebProcess.

Sure.

>> Source/WebKit2/WebProcess/WebPage/FindController.cpp:210
>> +    }
> 
> Is this branch really needed? Shouldn't the code bellow give you the exact same outcome?

Yes.

>> Source/WebKit2/WebProcess/WebPage/FindController.cpp:271
>> +        return;
> 
> Don't you want to reset the selection before returning, and maybe send the didGetImage message with a null image?

Makes sense.

>> Source/WebKit2/WebProcess/WebPage/FindController.cpp:285
>> +    frame->selection()->setSelection(VisibleSelection(m_findMatches[matchIndex].get()));
> 
> if (Frame* frame = m_findMatches[matchIndex]->startContainer()->document()->frame())
>     frame->selection()->setSelection(VisibleSelection(m_findMatches[matchIndex].get()));
> 
> ?

Not sure I understand what you mean here...

>> Source/WebKit2/WebProcess/WebPage/FindController.h:55
>> +    void getImageForFindMatch(uint32_t matchIndex);
> 
> Why the different unsigned types?

See my comment above.

>>> Source/WebKit2/WebProcess/WebPage/FindController.h:76
>>> +    bool getFindIndicatorBitmapAndRect(WebCore::Frame* frame, ShareableBitmap::Handle& handle, WebCore::IntRect& selectionRect);
>> 
>> The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]

Already fixed.

>> Source/WebKit2/WebProcess/WebPage/FindController.h:88
>> +    Vector<RefPtr<WebCore::Range> > m_findMatches;
> 
> How long-lived is the FindController? Is it OK to hold onto these Ranges for its lifetime? Should hideFindUI or something clear them?

Yes. Its lifetime is the same as the page.

[Simon Fraser (smfr)]

(In reply to comment #7)

> >> Source/WebCore/page/Page.h:239
> >> +        void findStringMatchingRanges(const String&, FindOptions, int maxCount, Vector<RefPtr<Range> >*, int& indexForSelection);
> > 
> > indexForSelection is an index into the Vector<RefPtr<Range> >? Would be nice to clarify that with a comment or clearer naming.
> 
> This value represents the default selected find match. This is 0 if there is no user selection 
> >> Source/WebKit2/WebProcess/WebPage/FindController.h:88
> >> +    Vector<RefPtr<WebCore::Range> > m_findMatches;
> > 
> > How long-lived is the FindController? Is it OK to hold onto these Ranges for its lifetime? Should hideFindUI or something clear them?
> 
> Yes. Its lifetime is the same as the page.

That worries me a bit. I think we should try to clear these Ranges when the is done finished. Otherwise they might entrain expensive Nodes (e.g. video element) for a long time.

[Enrica Casucci]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > >> Source/WebCore/page/Page.h:239
> > >> +        void findStringMatchingRanges(const String&, FindOptions, int maxCount, Vector<RefPtr<Range> >*, int& indexForSelection);
> > > 
> > > indexForSelection is an index into the Vector<RefPtr<Range> >? Would be nice to clarify that with a comment or clearer naming.
> > 
> > This value represents the default selected find match. This is 0 if there is no user selection 
> > >> Source/WebKit2/WebProcess/WebPage/FindController.h:88
> > >> +    Vector<RefPtr<WebCore::Range> > m_findMatches;
> > > 
> > > How long-lived is the FindController? Is it OK to hold onto these Ranges for its lifetime? Should hideFindUI or something clear them?
> > 
> > Yes. Its lifetime is the same as the page.
> 
> That worries me a bit. I think we should try to clear these Ranges when the is done finished. Otherwise they might entrain expensive Nodes (e.g. video element) for a long time.

Sorry, maybe I wasn't clear enough. The ranges are clears whenever we call WKPageHideFindUI, which signals we are done with search.

[Enrica Casucci]

Created attachment 182817 [details]
Patch2

[WebKit Review Bot]

Attachment 182817 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/UIProcess/WebPageProxy.cpp:1694:  Missing space after ,  [whitespace/comma] [3]
Source/WebKit2/UIProcess/API/C/WKPage.h:331:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 182817 [details]
Patch2

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

> Source/WebCore/page/Page.cpp:578
> +    // find all the Ranges for the matching text.
> +    // Upon return, indexForSelection will be one of the following:
> +    // 0 if there is no user selection
> +    // the index of the first range after the user selection
> +    // -1 if there is no matching text after the user selection.

I think this comment is more appropriate for the header. You could use an enum for the special values.

[EFL EWS Bot]

Comment on attachment 182817 [details]
Patch2

Attachment 182817 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15873984

[Enrica Casucci]

Committed revision 139796.

[Darin Adler]

Comment on attachment 182817 [details]
Patch2

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3069
> +void WebPageProxy::didFindStringMatches(const String& string, Vector<Vector<WebCore::IntRect> > matchRects, int32_t firstIndexAfterSelection)

Normally we would pass a const Vector& rather than a Vector to a function like this; that avoids one additional unwanted copy of the vector.

[Gyuyoung Kim]

Fix efl port build break on http://trac.webkit.org/changeset/139816