Bug 217919 – Rename HighlightMap to HighlightRegister and HighlightRangeGroup to Highlight to match current spec

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

[Megan Gardner]

Rename HighlightMap to HighlightRegister to match current spec

[Megan Gardner]

Created attachment 411788 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411788 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests, no new behavior, rename only.

A link the the spec would be nice here.

[Megan Gardner]

Created attachment 411799 [details]
Patch

[Sam Weinig]

Comment on attachment 411799 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests, no new behavior, rename only.

Shouldn't this have at least broken some tests. The names of the IDL exposed constructors are changed and that is observable.

> Source/WebCore/Modules/highlight/Highlight.h:38
> +class CSSStyleDeclaration;
> +class DOMSetAdapter;
> +class StaticRange;
> +class PropertySetCSSStyleDeclaration;

Please sort these.

> Source/WebCore/Modules/highlight/Highlight.h:42
> +struct HighlightRangeData : RefCounted<HighlightRangeData>, public CanMakeWeakPtr<HighlightRangeData> {
> +    
> +    HighlightRangeData(Ref<StaticRange>&& range)

You don't need this newline.

[Chris Dumez]

Comment on attachment 411799 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests, no new behavior, rename only.
> 
> Shouldn't this have at least broken some tests. The names of the IDL exposed constructors are changed and that is observable.

Agreed, if this does not break tests then this shows lack of test coverage and we should add test coverage for the name of these interfaces.

[Megan Gardner]

Created attachment 411818 [details]
Patch

[Wenson Hsieh]

Comment on attachment 411818 [details]
Patch

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

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text.html:29
> +        let highlight1 = new highlight(new StaticRange({startContainer: textElement.childNodes[0], startOffset: 1, endContainer: textElement.childNodes[0], endOffset: 2}));

`new Highlight`?

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text.html:31
> +        let highlight2 = new highlight(new StaticRange({startContainer: textElement.childNodes[0], startOffset: 3, endContainer: textElement.childNodes[0], endOffset: 4}));

(Ditto)

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text.html:34
> +        let highlight3 = new highlight(new StaticRange({startContainer: textElement.childNodes[0], startOffset: 7, endContainer: textElement.childNodes[0], endOffset: 8}));

(Ditto)

[Ryosuke Niwa]

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

[Megan Gardner]

Created attachment 411867 [details]
Patch

[Megan Gardner]

Created attachment 411869 [details]
Patch

[Megan Gardner]

Created attachment 411877 [details]
Patch

[Megan Gardner]

Created attachment 411878 [details]
Patch

[Megan Gardner]

Created attachment 411890 [details]
Patch

[Megan Gardner]

Created attachment 411932 [details]
Patch for landing

[EWS]

Committed r268774: <https://trac.webkit.org/changeset/268774>

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

[Radar WebKit Bug Importer]

<rdar://problem/70508151>