RESOLVED FIXED 204934
Fill HighlightRangeGroup and HighlightMap with values from JavaScript 
https://bugs.webkit.org/show_bug.cgi?id=204934
Summary Fill HighlightRangeGroup and HighlightMap with values from JavaScript
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
DetailsFormatted DiffDiff
Patch (15.54 KB, patch)
2019-12-06 14:26 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (16.05 KB, patch)
2019-12-09 13:44 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch for landing (16.04 KB, patch)
2019-12-09 16:09 PST, Megan Gardner 		no flags
DetailsFormatted DiffDiff
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.