Bug 204934

Summary:

Fill HighlightRangeGroup and HighlightMap with values from JavaScript

Product:

WebKit

Reporter:

Megan Gardner <megan_gardner>

Component:

New Bugs

Assignee:

Megan Gardner <megan_gardner>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, kondapallykalyan, simon.fraser, webkit-bug-importer, wenson_hsieh, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

Megan Gardner

Reported 2019-12-05 19:09:03 PST

Fill HighlightRangeGroup and HighlightMap with values from JavaScript

Attachments
Patch (11.99 KB, patch) 2019-12-05 19:12 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (15.54 KB, patch) 2019-12-06 14:26 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (16.05 KB, patch) 2019-12-09 13:44 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Patch for landing (16.04 KB, patch) 2019-12-09 16:09 PST, Megan Gardner	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Megan Gardner

Comment 1 2019-12-05 19:12:34 PST

Created attachment 384987 [details] Patch

Megan Gardner

Comment 2 2019-12-05 19:13:02 PST

Patch under development.

Radar WebKit Bug Importer

Comment 3 2019-12-05 19:14:20 PST

<rdar://problem/57686335>

youenn fablet

Comment 4 2019-12-05 23:51:41 PST

Comment on attachment 384987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384987&action=review > Source/WebCore/Modules/highlight/HighlightMap.cpp:29 > +#include "IDLTypes.h" Might want to add: #include "JSHighlightRangeGroup.h" to uncomment the for each loop > Source/WebCore/Modules/highlight/HighlightMap.h:57 > HighlightMap() = default; m_map could take a Ref<HighlightRangeGroup> > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:30 > +#include "JSDOMSetLike.h" Might want to add: #include "JSStaticRange.h" to uncomment set.add. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:53 > + Vector<Ref<StaticRange>> m_ranges; We could use a HashSet<Ref<>> to simplify addToSetLike/removeFromSetLike? > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:54 > HighlightRangeGroup(StaticRange&); explicit

Megan Gardner

Comment 5 2019-12-06 14:26:28 PST

Created attachment 385043 [details] Patch

Wenson Hsieh

Comment 6 2019-12-06 16:34:06 PST

Comment on attachment 385043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385043&action=review > LayoutTests/highlight/highlight-map-and-group.html:3 > +<body> Nit - there appears to be three body tags in this test.

Simon Fraser (smfr)

Comment 7 2019-12-06 16:37:14 PST

Comment on attachment 385043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385043&action=review > Source/WebCore/Modules/highlight/HighlightMap.cpp:58 > +HighlightRangeGroup* HighlightMap::getGroupForStyle(const String& key) Should return a RefPtr<> > Source/WebCore/Modules/highlight/HighlightMap.h:49 > + HighlightRangeGroup* getGroupForStyle(const String&); getRangeGroupForKey (or ForName) - pick one! > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:38 > HighlightRangeGroup::HighlightRangeGroup(StaticRange& range) StaticRange is RefCounted so you should be passing a Ref<StaticRange>&& and WTFMoving it. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:50 > + set.add<IDLInterface<StaticRange>>(m_ranges[0]); I think this needs to add all ranges. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:53 > +bool HighlightRangeGroup::removeFromSetLike(const StaticRange& range) Needs to take a Ref<StaticRange>, since this function might destroy the only reference and then the caller is using a reference to a deleted object. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:57 > + return m_ranges.removeFirstMatching([&range](const Ref<StaticRange>& current) { > + return current.get() == range; > + }); It remove all, to emulate set behavior. Solved when using the HashSet. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:65 > +bool HighlightRangeGroup::addToSetLike(StaticRange& range) Should take a Ref<StaticRange>&& > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:68 > + if (notFound == m_ranges.findMatching([&range](const Ref<StaticRange>& current) { > + return current.get() == range; })) { Some weird wrapping here. Might be more clearly written with an early return on the "found" case first. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:43 > + void clearFromSetLike(); "clear from" is weird naming. Maybe just clear()? > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:53 > + Vector<Ref<StaticRange>> m_ranges; Add a comment saying this should be a HashSet (pointing to a bug). > Source/WebCore/dom/StaticRange.h:59 > + bool operator==(const StaticRange& other) const; Remove "other". > LayoutTests/ChangeLog:9 > + * highlight/highlight-map-and-group-expected.txt: Added. > + * highlight/highlight-map-and-group.html: Added. Can we start writing web platform tests instead? Or is the StaticRange thing going to prevent that? > LayoutTests/highlight/highlight-map-and-group.html:9 > + background-color: yellow; > + color:blue; These styles aren't being used by anything in the test. > LayoutTests/highlight/highlight-map-and-group.html:21 > + <meta name="viewport" content="initial-scale=1"> Remove. > LayoutTests/highlight/highlight-map-and-group.html:45 > + shouldBe("CSS.highlights.size","3"); I think you should also read one or more range groups back and their their static ranges.

Megan Gardner

Comment 8 2019-12-09 13:44:09 PST

Created attachment 385184 [details] Patch

Megan Gardner

Comment 9 2019-12-09 16:09:13 PST

Created attachment 385201 [details] Patch for landing

WebKit Commit Bot

Comment 10 2019-12-09 16:53:30 PST

Comment on attachment 385201 [details] Patch for landing Clearing flags on attachment: 385201 Committed r253309: <https://trac.webkit.org/changeset/253309>

WebKit Commit Bot

Comment 11 2019-12-09 16:53:32 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.