Bug 217919 - Rename HighlightMap to HighlightRegister and HighlightRangeGroup to Highlight to match current spec
Summary: Rename HighlightMap to HighlightRegister and HighlightRangeGroup to Highlight...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-19 13:30 PDT by Megan Gardner
Modified: 2020-10-20 16:59 PDT (History)
24 users (show)

See Also:


Attachments
Patch (50.48 KB, patch)
2020-10-19 13:34 PDT, Megan Gardner
simon.fraser: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.17 KB, patch)
2020-10-19 14:26 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (74.07 KB, patch)
2020-10-19 17:09 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (74.03 KB, patch)
2020-10-20 08:22 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (74.05 KB, patch)
2020-10-20 08:45 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (74.19 KB, patch)
2020-10-20 09:47 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (74.19 KB, patch)
2020-10-20 09:58 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (74.18 KB, patch)
2020-10-20 11:11 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (74.18 KB, patch)
2020-10-20 16:12 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>