[webkit-reviews] review granted: [Bug 217919] Rename HighlightMap to HighlightRegister and HighlightRangeGroup to Highlight to match current spec : [Attachment 411818] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 217919: Rename HighlightMap to HighlightRegister and HighlightRangeGroup to
Highlight to match current spec
https://bugs.webkit.org/show_bug.cgi?id=217919

Attachment 411818: Patch

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




--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 411818
  --> https://bugs.webkit.org/attachment.cgi?id=411818
Patch

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

Looks ike you also need to fix highlight/highlight-world-leak.html

> Source/WebCore/Modules/highlight/Highlight.cpp:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

2019-2020?

> Source/WebCore/Modules/highlight/Highlight.cpp:61
> +    auto* startNode = &range.startContainer();
> +    auto* endNode = &range.endContainer();

Please use makeRefPtr.

> Source/WebCore/Modules/highlight/Highlight.cpp:74
> +    auto node = startNode;
> +    while (node != endNode) {

Use intersectingNodes?

> Source/WebCore/Modules/highlight/Highlight.h:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

2019-2020?

> Source/WebCore/Modules/highlight/Highlight.h:41
> +    

Nit: Whitespace.

> Source/WebCore/Modules/highlight/Highlight.h:46
> +    

Nit: Whitespace.

> Source/WebCore/Modules/highlight/Highlight.h:51
> +    

Nit: Whitespace.

> Source/WebCore/Modules/highlight/Highlight.h:60
> +    

Nit: Whitespace.

> Source/WebCore/Modules/highlight/Highlight.h:65
> +    

Nit: Whitespace.

> Source/WebCore/Modules/highlight/Highlight.h:69
> +    

Nit: Whitespace.

> LayoutTests/highlight/highlight-interfaces.html:16
> +shouldBeDefined('new HighlightRegister().set("foo-styling",new
HighlightRegister(new StaticRange({startContainer: document.body, startOffset:
1, endContainer: document.body, endOffset: 2})))');

This appears to be failing on Mojave. Surely, the second argument must one a
Highlight, not HighlightRegister.

-PASS new HighlightRegister().set("foo-styling",new Highlight(new
StaticRange({startContainer: document.body, startOffset: 1, endContainer:
document.body, endOffset: 2}))) is defined.
+FAIL new HighlightRegister().set("foo-styling",new HighlightRegister(new
StaticRange({startContainer: document.body, startOffset: 1, endContainer:
document.body, endOffset: 2}))) should be defined. Threw exception TypeError:
Argument 2 ('value') to HighlightRegister.set must be an instance of Highlight
 PASS CSS.highlights is defined.
-PASS CSS.highlights.set("foo-styling",new Highlight(new
StaticRange({startContainer: document.body, startOffset: 1, endContainer:
document.body, endOffset: 2}))) is CSS.highlights
+FAIL CSS.highlights.set("foo-styling",new HighlightRegister(new
StaticRange({startContainer: document.body, startOffset: 1, endContainer:
document.body, endOffset: 2}))) should be [object HighlightRegister]. Threw
exception TypeError: Argument 2 ('value') to HighlightRegister.set must be an
instance of Highlight
 PASS successfullyParsed is true