Bug 233915 – [iOS] Add initial support for find-in-page SPI

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

[Aditya Keerthi]

...

[Aditya Keerthi]

rdar://86140501

[Aditya Keerthi]

Created attachment 446133 [details]
Patch

[Aditya Keerthi]

Created attachment 446186 [details]
Patch

[Wenson Hsieh]

Comment on attachment 446186 [details]
Patch

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

r=mews

> Source/WebKit/WebProcess/WebPage/FindController.cpp:305
> +void FindController::findRectsForStringMatches(const String& string, OptionSet<WebKit::FindOptions> options, unsigned maxMatchCount, CompletionHandler<void(Vector<WebCore::FloatRect>&&)>&& completionHandler)

Nit - you can omit the WebKit:: and WebCore:: here.

> Source/WebKit/WebProcess/WebPage/FindController.cpp:307
> +    WebCore::FindOptions coreOptions = core(options);

(Ditto)

> Source/WebKit/WebProcess/WebPage/FindController.cpp:312
> +    Vector<FloatRect> rects;

Nit - it would be slightly more efficient to use `rects.reserveInitialCapacity(m_findMatches.size());` here, or alternately, `Vector::map()`.

> Source/WebKit/WebProcess/WebPage/FindController.cpp:387
> +#if PLATFORM(IOS_FAMILY)
> +    didFindString();
> +#else
>      selectedFrame->selection().revealSelection();
> +#endif

Nit - I think this would be slightly cleaner if `willFindString()` and `didFindString()` were cross-platform (but on non-iOS platforms, `willFindString()` would be empty and `didFindString()` would reveal the frame selection)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4696
> +void WebPage::findRectsForStringMatches(const String& string, OptionSet<WebKit::FindOptions> options, uint32_t maxMatchCount, CompletionHandler<void(Vector<WebCore::FloatRect>&&)>&& completionHandler)

Nit - I think you can omit the WebKit:: and WebCore:: here.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1763
> +    void findRectsForStringMatches(const String&, OptionSet<WebKit::FindOptions>, uint32_t maxMatchCount, CompletionHandler<void(Vector<WebCore::FloatRect>&&)>&&);

Nit - you can omit the WebKit:: here.

[Aditya Keerthi]

Created attachment 446207 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 446186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446186&action=review
> 
> r=mews

Thanks for the review!

> > Source/WebKit/WebProcess/WebPage/FindController.cpp:305
> > +void FindController::findRectsForStringMatches(const String& string, OptionSet<WebKit::FindOptions> options, unsigned maxMatchCount, CompletionHandler<void(Vector<WebCore::FloatRect>&&)>&& completionHandler)
> 
> Nit - you can omit the WebKit:: and WebCore:: here.

Omitted here, as well as the other places you mentioned.

> Nit - it would be slightly more efficient to use
> `rects.reserveInitialCapacity(m_findMatches.size());` here, or alternately,
> `Vector::map()`.

Used `Vector::map()`.
 
> > Source/WebKit/WebProcess/WebPage/FindController.cpp:387
> > +#if PLATFORM(IOS_FAMILY)
> > +    didFindString();
> > +#else
> >      selectedFrame->selection().revealSelection();
> > +#endif
> 
> Nit - I think this would be slightly cleaner if `willFindString()` and
> `didFindString()` were cross-platform (but on non-iOS platforms,
> `willFindString()` would be empty and `didFindString()` would reveal the
> frame selection)

Done. I also removed an additional platform check to support this approach.

`willFindString()` was already empty on non-iOS platforms. I made `didFindString()` reveal the frame selection, but there was one other call site to `didFindString()` that was already revealing selection prior to calling it (FindController::findString).

So I removed the platform check before calling `coreOptions.add(DoNotRevealSelection)`, as `didFindString()` is now responsible for revealing the selection in that method.

[EWS]

Committed r286641 (244954@main): <https://commits.webkit.org/244954@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446207 [details].