Summary: | Web Inspector: Audit: attempt to re-link DOM nodes for imported results | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bburg, commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 190754 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Devin Rousso
2018-10-29 15:10:57 PDT
Created attachment 353454 [details]
Patch
Comment on attachment 353454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353454&action=review r=me. Please rebase and run through EWS before landing. > Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:79 > + if (window.DOMAgent && Array.isArray(data.domNodes)) { I don't think we should be showing audits tab if DOMAgent is unavailable (i.e. JSContext inspection). So this check seems unnecessary. > Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:87 > + })); Very nice. > Source/WebInspectorUI/UserInterface/Models/AuditTestGroup.js:60 > + tests = await Promise.all(tests.map(async (test) => await WI.AuditTestCase.fromPayload(test) || await WI.AuditTestGroup.fromPayload(test))); Could you add some newlines here? It's difficult to follow. > Source/WebInspectorUI/UserInterface/Models/AuditTestGroupResult.js:55 > + results = await Promise.all(results.map(async (test) => await WI.AuditTestCaseResult.fromPayload(test) || await WI.AuditTestGroupResult.fromPayload(test))); Could you add some newlines here? It's difficult to follow. Comment on attachment 353454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353454&action=review >> Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:79 >> + if (window.DOMAgent && Array.isArray(data.domNodes)) { > > I don't think we should be showing audits tab if DOMAgent is unavailable (i.e. JSContext inspection). So this check seems unnecessary. I disagree. Right now there's no reason why a developer couldn't write a JS-only audit (e.g. a perf test). I don't want to limit this entire feature to contexts with DOM, since it's not really focusing on that exclusively. Comment on attachment 353454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353454&action=review >>> Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:79 >>> + if (window.DOMAgent && Array.isArray(data.domNodes)) { >> >> I don't think we should be showing audits tab if DOMAgent is unavailable (i.e. JSContext inspection). So this check seems unnecessary. > > I disagree. Right now there's no reason why a developer couldn't write a JS-only audit (e.g. a perf test). I don't want to limit this entire feature to contexts with DOM, since it's not really focusing on that exclusively. OK Created attachment 353509 [details]
Patch
Created attachment 353516 [details]
Patch
Fix test failures
Created attachment 353522 [details]
Patch
Rebase
Created attachment 353525 [details]
Patch
Comment on attachment 353525 [details] Patch Clearing flags on attachment: 353525 Committed r237656: <https://trac.webkit.org/changeset/237656> All reviewed patches have been landed. Closing bug. |