Summary: | Rename HighlightMap to HighlightRegister and HighlightRangeGroup to Highlight to match current spec | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Megan Gardner
2020-10-19 13:30:03 PDT
Created attachment 411788 [details]
Patch
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. Created attachment 411799 [details]
Patch
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 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. Created attachment 411818 [details]
Patch
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 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 Created attachment 411867 [details]
Patch
Created attachment 411869 [details]
Patch
Created attachment 411877 [details]
Patch
Created attachment 411878 [details]
Patch
Created attachment 411890 [details]
Patch
Created attachment 411932 [details]
Patch for landing
Committed r268774: <https://trac.webkit.org/changeset/268774> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411932 [details]. |