Bug 236693 – Draw highlights for Scroll To Text Fragment

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Draw highlights for Scroll To Text Fragment

[Megan Gardner]

Created attachment 452154 [details]
Patch

[Megan Gardner]

Created attachment 452296 [details]
Patch

[Tim Horton]

Comment on attachment 452296 [details]
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.

[Megan Gardner]

Created attachment 452304 [details]
Patch

[Chris Dumez]

Comment on attachment 452304 [details]
Patch

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

> Source/WebCore/Modules/highlight/HighlightRegister.cpp:67
> +static ASCIILiteral defaultHighlightKey()

We don't really need a function, this could just be a:
constexpr auto defaultHighlightKey = "defaultHighlightKey"_s;

Usually near the top of the cpp file.

> Source/WebCore/Modules/highlight/HighlightRegister.cpp:74
> +    if (m_map.contains(defaultHighlightKey()))

This is doing multiple hash lookups so it is not very efficient. This would be more efficient:
```
auto addResult = m_map.ensure(defaultHighlightKey, [value]() mutable { return Highlight::create(WTFMove(value)); });
if (!addResult.isNewEntry)
    addResult.iterator.value->addToSetLike(WTFMove(value));
```

> Source/WebCore/Modules/highlight/HighlightRegister.h:49
> +

nit: Unnecessary extra line.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:57
> +    return resultRange.collapsed() ? std::nullopt : std::make_optional(resultRange);

We may want to WTFMove(resultRange) here.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:64
> +    for (auto directive : parsedTextDirectives) {

ParsedTextDirective is fairly large (4 Strings). I recommend using auto&.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:68
> +            ranges.append(range.value());

ranges.append(WTFMove(*range));

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:74
> +}

nit: No comment for this one?

> Source/WebCore/dom/FragmentDirectiveRangeFinder.cpp:76
> +} // WebCore

Usually we use
// namespace WebCore

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:29
> +#include "SimpleRange.h"

Can this be forward-declared instead?

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:31
> +#include <wtf/text/WTFString.h>

Should not be needed?

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:37
> +const Vector<SimpleRange> rangesForFragments(Vector<ParsedTextDirective>& parsedTextDirectives, Document&);

Can the first parameter be const?

> Source/WebCore/page/FrameView.cpp:2251
> +            

Too many blank lines.

> Source/WebCore/page/FrameView.cpp:2253
> +            for (auto range : highlightRanges)

SimpleRange doesn't look cheap to copy, I recommend using auto&.

> Source/WebCore/rendering/MarkedText.cpp:135
> +            for (auto& highlight : fragmentHighlightRegister->map()) {

Seems you could iterate over `fragmentHighlightRegister->map().values()` since you're not interested in the keys.

> Source/WebCore/rendering/MarkedText.cpp:136
> +                for (auto& rangeData : highlight.value->rangesData()) {

Then you wouldn't need the .value here.

> Source/WebCore/rendering/RenderReplaced.cpp:188
> +            for (auto& highlight : highlightRegister->map()) {

Could iterate over fragmentHighlightRegister->map().values().

> Source/WebCore/rendering/RenderReplaced.cpp:197
> +                    OptionSet<StyleColorOptions> styleColorOptions = { StyleColorOptions::UseSystemAppearance };

Not sure we really need a local variable for this.

> Source/WebCore/rendering/StyledMarkedText.cpp:75
> +        OptionSet<StyleColorOptions> styleColorOptions = { StyleColorOptions::UseSystemAppearance };

Not sure we really need a local variable for this.

> Source/WebCore/rendering/StyledMarkedText.cpp:78
> +    }

then we can drop the curly brackets.

[Megan Gardner]

Created attachment 452322 [details]
Patch

[Megan Gardner]

Created attachment 452327 [details]
Patch

[Megan Gardner]

Created attachment 452386 [details]
Patch

[Megan Gardner]

Created attachment 452397 [details]
Patch

[Megan Gardner]

Created attachment 452438 [details]
Patch for landing

[Megan Gardner]

Created attachment 452451 [details]
Patch

[EWS]

Committed r290116 (247460@main): <https://commits.webkit.org/247460@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452451 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89132086>

[Darin Adler]

Comment on attachment 452451 [details]
Patch

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

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:28
> +#include "FragmentDirectiveParser.h"

Don’t need ParsedTextDirective definition here, a forward declaration should suffice.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:29
> +#include "SimpleRange.h"

Don’t need SimpleRange definition here, a forward declaration should suffice.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:31
> +#include <wtf/text/WTFString.h>

No reason for this include.

> Source/WebCore/dom/FragmentDirectiveRangeFinder.h:37
> +const Vector<SimpleRange> rangesForFragments(Vector<ParsedTextDirective>& parsedTextDirectives, Document&);

The const on the return value here is not meaningful and should be removed.

The passed-in vector should be const&, I think.