Fill HighlightRangeGroup and HighlightMap with values from JavaScript
Created attachment 384987 [details] Patch
Patch under development.
<rdar://problem/57686335>
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
Created attachment 385043 [details] Patch
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.
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.
Created attachment 385184 [details] Patch
Created attachment 385201 [details] Patch for landing
Comment on attachment 385201 [details] Patch for landing Clearing flags on attachment: 385201 Committed r253309: <https://trac.webkit.org/changeset/253309>
All reviewed patches have been landed. Closing bug.