[webkit-reviews] review granted: [Bug 236693] Draw highlights for Scroll To Text Fragment : [Attachment 452296] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 236693: Draw highlights for Scroll To Text Fragment
https://bugs.webkit.org/show_bug.cgi?id=236693

Attachment 452296: Patch

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




--- Comment #3 from Tim Horton <thorton at apple.com> ---
Comment on attachment 452296
  --> https://bugs.webkit.org/attachment.cgi?id=452296
Patch

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

> Source/WebCore/dom/Document.h:1629
> +    HighlightRegister* fragmentHighlightRegisterIfExists() { return
m_fragmentHighlightRegister.get(); }

There's a whole lot of highlights members and methods on document now, perhaps
in the future we can shuffle them off into their own class?

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:72
> +		   TemporarySelectionChange selectionChange(document, {
range.value() }, { TemporarySelectionOption::DelegateMainFrameScroll,
TemporarySelectionOption::SmoothScroll,
TemporarySelectionOption::RevealSelectionBounds });

`rangesForFragments` sounds like it finds ranges, not that it does scrolling.
I'd expect this to be tucked up in the caller.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:74
> +		   // FIXME: add a textIndicator after the scroll has
completed.

This, too, shouldn't be in here.

> Source/WebCore/page/FrameView.cpp:2254
> +	       // FIXME: Scroll to and bouce the first highlight

"bouce". Also, I think this is where you want the scrolling code from above.

> Source/WebCore/rendering/RenderReplaced.cpp:190
> +		       if (!highlightData.setRenderRange(rangeData))

I definitely don't follow why "calculate color" is calling a setter on the
highlight (you'd think it would just look up the color?) but this is just
semi-duplicating pre-existing code so... ok.