Web Inspector: Highlight matching CSS canvas clients when hovering contexts in the Resources tab

[Devin Rousso]

Since the canvas element of a -webkit-canvas is not attached, it won't highlight anything when hovered in the Resources tab.  Instead, we should highlight all nodes that are using that canvas, similar to how they are listed in the Canvas details sidebar.

[Devin Rousso]

Created attachment 314926 [details]
Patch

[Build Bot]

Comment on attachment 314926 [details]
Patch

Attachment 314926 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4079568

New failing tests:
inspector/dom/highlightNodeList.html

[Build Bot]

Created attachment 314927 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 314994 [details]
Patch

[Matt Baker]

Comment on attachment 314994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314994&action=review

r=me, with nits.

> Source/JavaScriptCore/ChangeLog:9
> +         - Add `highlightNodeList` command that will highlight each node in the given list.

Nit: drop the ` - `

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1168
> +            ASSERT(errorString.length());

This assert is unnecessary. `errorString` is always set.

> LayoutTests/inspector/dom/highlightNodeList.html:47
> +        description: "Should not be a highlight yet.",

What about: "Check that highlight list is initially empty."

> LayoutTests/inspector/dom/highlightNodeList.html:50
> +                InspectorTest.expectEqual(highlightObjectPayload.length, 0, "Should not be a highlight yet.");

What about: "Highlight should not exist."

[Devin Rousso]

Created attachment 315023 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 315023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315023&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1167
> +        Node* node = assertNode(errorString, nodeId);
> +        if (!node)
> +            return;

I wonder if here we should `continue` instead of `return`. The communication between the frontend and backend is async. So maybe by the time the frontend says "Highlight Nodes [A, B, C]" the backend as removed Node B. We should still highlight A and C, but here we would bail.

[Matt Baker]

(In reply to Joseph Pecoraro from comment #7)
> Comment on attachment 315023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315023&action=review
> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:1167
> > +        Node* node = assertNode(errorString, nodeId);
> > +        if (!node)
> > +            return;
> 
> I wonder if here we should `continue` instead of `return`. The communication
> between the frontend and backend is async. So maybe by the time the frontend
> says "Highlight Nodes [A, B, C]" the backend as removed Node B. We should
> still highlight A and C, but here we would bail.

I noticed this but forgot to mention it during my review. `continue` makes sense.

[Devin Rousso]

Created attachment 315038 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315038 [details]
Patch

Clearing flags on attachment: 315038

Committed r219316: <http://trac.webkit.org/changeset/219316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.