[webkit-reviews] review granted: [Bug 233915] [iOS] Add initial support for find-in-page SPI : [Attachment 446186] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 233915: [iOS] Add initial support for find-in-page SPI
https://bugs.webkit.org/show_bug.cgi?id=233915

Attachment 446186: Patch

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




--- Comment #4 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 446186
  --> https://bugs.webkit.org/attachment.cgi?id=446186
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.