Bug 204306 – Web Inspector: DOM.highlightSelector should work for "div, div::before"

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

(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.

[Devin Rousso]

Created attachment 383770 [details]
Patch

[Blaze Burg]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 383836 [details]
Patch

[Antti Koivisto]

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.

[Devin Rousso]

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.

[Antti Koivisto]

> 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);

[Blaze Burg]

Comment on attachment 383836 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 383836 [details]
Patch

Clearing flags on attachment: 383836

Committed r252682: <https://trac.webkit.org/changeset/252682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57347533>