Bug 174279

Summary: Web Inspector: Highlight matching CSS canvas clients when hovering contexts in the Resources tab
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, mattbaker, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 173965, 174209    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
mattbaker: review+
Patch
none
Patch none

Description Devin Rousso 2017-07-07 14:42:36 PDT
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.
Comment 1 Devin Rousso 2017-07-08 14:37:23 PDT
Created attachment 314926 [details]
Patch
Comment 2 Build Bot 2017-07-08 16:02:15 PDT
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
Comment 3 Build Bot 2017-07-08 16:02:16 PDT
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
Comment 4 Devin Rousso 2017-07-10 10:00:07 PDT
Created attachment 314994 [details]
Patch
Comment 5 Matt Baker 2017-07-10 10:40:21 PDT
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."
Comment 6 Devin Rousso 2017-07-10 14:09:09 PDT
Created attachment 315023 [details]
Patch
Comment 7 Joseph Pecoraro 2017-07-10 14:16:49 PDT
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.
Comment 8 Matt Baker 2017-07-10 14:53:47 PDT
(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.
Comment 9 Devin Rousso 2017-07-10 15:39:11 PDT
Created attachment 315038 [details]
Patch
Comment 10 WebKit Commit Bot 2017-07-10 17:01:58 PDT
Comment on attachment 315038 [details]
Patch

Clearing flags on attachment: 315038

Committed r219316: <http://trac.webkit.org/changeset/219316>
Comment 11 WebKit Commit Bot 2017-07-10 17:02:02 PDT
All reviewed patches have been landed.  Closing bug.