[webkit-reviews] review granted: [Bug 204934] Fill HighlightRangeGroup and HighlightMap with values from JavaScript : [Attachment 385043] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 204934: Fill HighlightRangeGroup and HighlightMap with values from
JavaScript
https://bugs.webkit.org/show_bug.cgi?id=204934

Attachment 385043: Patch

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




--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 385043
  --> https://bugs.webkit.org/attachment.cgi?id=385043
Patch

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

> Source/WebCore/Modules/highlight/HighlightMap.cpp:58
> +HighlightRangeGroup* HighlightMap::getGroupForStyle(const String& key)

Should return a RefPtr<>

> Source/WebCore/Modules/highlight/HighlightMap.h:49
> +    HighlightRangeGroup* getGroupForStyle(const String&);

getRangeGroupForKey (or ForName) - pick one!

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:38
>  HighlightRangeGroup::HighlightRangeGroup(StaticRange& range)

StaticRange is RefCounted so you should be passing a Ref<StaticRange>&& and
WTFMoving it.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:50
> +    set.add<IDLInterface<StaticRange>>(m_ranges[0]);

I think this needs to add all ranges.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:53
> +bool HighlightRangeGroup::removeFromSetLike(const StaticRange& range)

Needs to take a Ref<StaticRange>, since this function might destroy the only
reference and then the caller is using a reference to a deleted object.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:57
> +    return m_ranges.removeFirstMatching([&range](const Ref<StaticRange>&
current) {
> +	   return current.get() == range;
> +    });

It remove all, to emulate set behavior. Solved when using the HashSet.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:65
> +bool HighlightRangeGroup::addToSetLike(StaticRange& range)

Should take a Ref<StaticRange>&&

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:68
> +    if (notFound == m_ranges.findMatching([&range](const Ref<StaticRange>&
current) {
> +	   return current.get() == range; })) {

Some weird wrapping here.

Might be more clearly written with an early return on the "found" case first.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.h:43
> +    void clearFromSetLike();

"clear from" is weird naming. Maybe just clear()?

> Source/WebCore/Modules/highlight/HighlightRangeGroup.h:53
> +    Vector<Ref<StaticRange>> m_ranges;

Add a comment saying this should be a HashSet (pointing to a bug).

> Source/WebCore/dom/StaticRange.h:59
> +    bool operator==(const StaticRange& other) const;

Remove "other".

> LayoutTests/ChangeLog:9
> +	   * highlight/highlight-map-and-group-expected.txt: Added.
> +	   * highlight/highlight-map-and-group.html: Added.

Can we start writing web platform tests instead? Or is the StaticRange thing
going to prevent that?

> LayoutTests/highlight/highlight-map-and-group.html:9
> +	   background-color: yellow;
> +	   color:blue;

These styles aren't being used by anything in the test.

> LayoutTests/highlight/highlight-map-and-group.html:21
> +    <meta name="viewport" content="initial-scale=1">

Remove.

> LayoutTests/highlight/highlight-map-and-group.html:45
> +	   shouldBe("CSS.highlights.size","3");

I think you should also read one or more range groups back and their their
static ranges.