[webkit-reviews] review granted: [Bug 235692] Add support for decorating and scrolling to ranges in WebFoundTextRangeController : [Attachment 450251] 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 235692: Add support for decorating and scrolling to ranges in
WebFoundTextRangeController
https://bugs.webkit.org/show_bug.cgi?id=235692

Attachment 450251: Patch

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




--- Comment #5 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 450251
  --> https://bugs.webkit.org/attachment.cgi?id=450251
Patch

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

> Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:109
> +    if (style == FindDecorationStyle::Normal) {
> +	   simpleRange->start.document().markers().removeMarkers(*simpleRange,
WebCore::DocumentMarker::TextMatch);
> +    } else if (style == FindDecorationStyle::Found) {
> +	   simpleRange->start.document().markers().addMarker(*simpleRange,
WebCore::DocumentMarker::TextMatch);
> +    } else if (style == FindDecorationStyle::Highlighted) {

I think the { } here around single-line if statements should be omitted (unless
that rule has changed recently?)

> Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:136
> +    OptionSet<WebCore::TemporarySelectionOption> temporarySelectionOptions {
WebCore::TemporarySelectionOption::DelegateMainFrameScroll,
WebCore::TemporarySelectionOption::RevealSelectionBounds };

Nit - can you replace "OptionSet<WebCore::TemporarySelectionOption>" with just
OptionSet? (I think that type deduction should be sufficient to infer the type
here)

> Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:259
> +    auto* mainFrameView = m_webPage->corePage()->mainFrame().view();

I think we should be protecting these with RefPtr (especially since the logic
below involves more than just simple getters)

> Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:288
> +    for (auto* frame =
m_webPage->corePage()->mainFrame().document()->topDocument().frame(); frame;
frame = frame->tree().traverseNext()) {
> +	   if (frame->tree().uniqueName() == range.frameIdentifier)
> +	       return frame->document();
> +    }

Can we simplify this logic a bit using `FrameTree::find()`?