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

Devin Rousso
Reported 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.
Attachments
Patch (17.55 KB, patch)
2017-07-08 14:37 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews115 for mac-elcapitan (1.81 MB, application/zip)
2017-07-08 16:02 PDT, Build Bot 		no flags
Details
Patch (17.56 KB, patch)
2017-07-10 10:00 PDT, Devin Rousso 		mattbaker: review+
DetailsFormatted DiffDiff
Patch (16.80 KB, patch)
2017-07-10 14:09 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (17.81 KB, patch)
2017-07-10 15:39 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2017-07-08 14:37:23 PDT
Created attachment 314926 [details] Patch
Build Bot
Comment 2 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
Build Bot
Comment 3 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
Devin Rousso
Comment 4 2017-07-10 10:00:07 PDT
Created attachment 314994 [details] Patch
Matt Baker
Comment 5 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."
Devin Rousso
Comment 6 2017-07-10 14:09:09 PDT
Created attachment 315023 [details] Patch
Joseph Pecoraro
Comment 7 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.
Matt Baker
Comment 8 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.
Devin Rousso
Comment 9 2017-07-10 15:39:11 PDT
Created attachment 315038 [details] Patch
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2017-07-10 17:02:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.