Summary: | Web Inspector: Highlight matching CSS canvas clients when hovering contexts in the Resources tab | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | 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
Devin Rousso
2017-07-07 14:42:36 PDT
Created attachment 314926 [details]
Patch
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 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
Created attachment 314994 [details]
Patch
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." Created attachment 315023 [details]
Patch
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. (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. Created attachment 315038 [details]
Patch
Comment on attachment 315038 [details] Patch Clearing flags on attachment: 315038 Committed r219316: <http://trac.webkit.org/changeset/219316> All reviewed patches have been landed. Closing bug. |