Bug 204306 - Web Inspector: DOM.highlightSelector should work for "div, div::before"
Summary: Web Inspector: DOM.highlightSelector should work for "div, div::before"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 146161
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-18 11:42 PST by Devin Rousso
Modified: 2019-11-19 20:32 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.79 KB, patch)
2019-11-18 11:46 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2019-11-18 19:52 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-11-18 11:42:36 PST
(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.
Comment 1 Devin Rousso 2019-11-18 11:46:33 PST
Created attachment 383770 [details]
Patch
Comment 2 BJ Burg 2019-11-18 15:54:07 PST
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 3 Devin Rousso 2019-11-18 16:51:11 PST
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.
Comment 4 Devin Rousso 2019-11-18 19:52:40 PST
Created attachment 383836 [details]
Patch
Comment 5 Antti Koivisto 2019-11-19 00:23:45 PST
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 6 Devin Rousso 2019-11-19 10:16:34 PST
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.
Comment 7 Antti Koivisto 2019-11-19 10:23:52 PST
> 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 8 BJ Burg 2019-11-19 18:47:18 PST
Comment on attachment 383836 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2019-11-19 20:31:28 PST
Comment on attachment 383836 [details]
Patch

Clearing flags on attachment: 383836

Committed r252682: <https://trac.webkit.org/changeset/252682>
Comment 10 WebKit Commit Bot 2019-11-19 20:31:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-11-19 20:32:20 PST
<rdar://problem/57347533>