Bug 217919

Summary: Rename HighlightMap to HighlightRegister and HighlightRangeGroup to Highlight to match current spec
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, kangil.han, kondapallykalyan, macpherson, menard, mmaxfield, pdr, rniwa, ryuan.choi, sam, sergio, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
simon.fraser: review+, ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Megan Gardner 2020-10-19 13:30:03 PDT
Rename HighlightMap to HighlightRegister to match current spec
Comment 1 Megan Gardner 2020-10-19 13:34:14 PDT
Created attachment 411788 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-10-19 13:42:44 PDT
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.
Comment 3 Megan Gardner 2020-10-19 14:26:24 PDT
Created attachment 411799 [details]
Patch
Comment 4 Sam Weinig 2020-10-19 14:36:22 PDT
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.
Comment 5 Chris Dumez 2020-10-19 14:38:54 PDT
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.
Comment 6 Megan Gardner 2020-10-19 17:09:53 PDT
Created attachment 411818 [details]
Patch
Comment 7 Wenson Hsieh 2020-10-19 17:25:49 PDT
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)
Comment 8 Ryosuke Niwa 2020-10-19 21:10:49 PDT
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
Comment 9 Megan Gardner 2020-10-20 08:22:23 PDT
Created attachment 411867 [details]
Patch
Comment 10 Megan Gardner 2020-10-20 08:45:11 PDT
Created attachment 411869 [details]
Patch
Comment 11 Megan Gardner 2020-10-20 09:47:05 PDT
Created attachment 411877 [details]
Patch
Comment 12 Megan Gardner 2020-10-20 09:58:35 PDT
Created attachment 411878 [details]
Patch
Comment 13 Megan Gardner 2020-10-20 11:11:46 PDT
Created attachment 411890 [details]
Patch
Comment 14 Megan Gardner 2020-10-20 16:12:37 PDT
Created attachment 411932 [details]
Patch for landing
Comment 15 EWS 2020-10-20 16:58:07 PDT
Committed r268774: <https://trac.webkit.org/changeset/268774>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411932 [details].
Comment 16 Radar WebKit Bug Importer 2020-10-20 16:59:17 PDT
<rdar://problem/70508151>