(In reply to Antti Koivisto from comment https://webkit.org/b/146161#c14) > Comment on attachment 383496 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383496&action=review > > > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1286 > > + if (selectorChecker.match(*selector, descendantElement, context, ignoredSpecificity)) { > > + nodes.append(descendantElement); > > + break; > > + } > > Shouldn't this continue instead of break? If you have selector like '.foo, .foo::before' I think the current code will only highlight .foo. > > > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1305 > > + break; > > This too.
Created attachment 383770 [details] Patch
Comment on attachment 383770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383770&action=review Code changes look good. We need to add a test for DOM.highlightSelector, it doesn't need to be super complicated but it should show a progression with this change (or at least exercise the obvious cases this should handle.) > Source/WebCore/ChangeLog:7 > + You should say how it is currently broken. Maybe mention that you need to deduplicate the NodeList to make the given example not return wrong results.
Comment on attachment 383770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383770&action=review (In reply to Brian Burg from comment #2) > We need to add a test for DOM.highlightSelector, it doesn't need to be super complicated but it should show a progression with this change (or at least exercise the obvious cases this should handle.) I'll add something for `DOM.highlightSelector` to ManualTests/inspector/overlay-nodes.html. >> Source/WebCore/ChangeLog:7 >> + > > You should say how it is currently broken. Maybe mention that you need to deduplicate the NodeList to make the given example not return wrong results. I'll add more detail than what I put a few lines below. As far as deduplicating, the only reason we want to do that is so that we don't draw an overlay for the same node twice, which would appear wrong as we use opacity in the overlay, meaning that the overlay for that node would appear less transparent.
Created attachment 383836 [details] Patch
Comment on attachment 383836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383836&action=review > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1264 > + Vector<Ref<Node>> nodeList; > + HashSet<Node*> seenNodes; You only really need a HashSet, you can turn it into a vector at the end with copyToVector. If ordering is important you can use ListHashSet.
Comment on attachment 383836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383836&action=review >> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1264 >> + HashSet<Node*> seenNodes; > > You only really need a HashSet, you can turn it into a vector at the end with copyToVector. > > If ordering is important you can use ListHashSet. I tried doing that, but because we need to use `Ref<Node>`, the items in the `HashSet` aren't copyable.
> I tried doing that, but because we need to use `Ref<Node>`, the items in the > `HashSet` aren't copyable. You could still build up the Vector at the end with a loop from the hash content. It still simplifies the code since every instance of if (seenNodes.add(&descendantElement)) nodeList.append(descendantElement); becomes just nodes.add(&descendantElement);
Comment on attachment 383836 [details] Patch r=me
Comment on attachment 383836 [details] Patch Clearing flags on attachment: 383836 Committed r252682: <https://trac.webkit.org/changeset/252682>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57347533>