Bug 233915 - [iOS] Add initial support for find-in-page SPI
Summary: [iOS] Add initial support for find-in-page SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks: 233957
  Show dependency treegraph
 
Reported: 2021-12-06 23:51 PST by Aditya Keerthi
Modified: 2021-12-07 21:32 PST (History)
9 users (show)

See Also:


Attachments
Patch (27.49 KB, patch)
2021-12-07 00:03 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (27.53 KB, patch)
2021-12-07 09:23 PST, Aditya Keerthi
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch for landing (28.61 KB, patch)
2021-12-07 11:26 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2021-12-06 23:51:20 PST
...
Comment 1 Aditya Keerthi 2021-12-06 23:51:34 PST
rdar://86140501
Comment 2 Aditya Keerthi 2021-12-07 00:03:13 PST
Created attachment 446133 [details]
Patch
Comment 3 Aditya Keerthi 2021-12-07 09:23:38 PST
Created attachment 446186 [details]
Patch
Comment 4 Wenson Hsieh 2021-12-07 09:43:04 PST
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.
Comment 5 Aditya Keerthi 2021-12-07 11:26:53 PST
Created attachment 446207 [details]
Patch for landing
Comment 6 Aditya Keerthi 2021-12-07 11:32:56 PST
(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.
Comment 7 EWS 2021-12-07 21:32:07 PST
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].