...
rdar://86140501
Created attachment 446133 [details] Patch
Created attachment 446186 [details] Patch
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.
Created attachment 446207 [details] Patch for landing
(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.
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].