[webkit-reviews] review denied: [Bug 106834] Add new API for text search in WebKit2 : [Attachment 182642] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 106834: Add new API for text search in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=106834

Attachment 182642: Patch
https://bugs.webkit.org/attachment.cgi?id=182642&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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?