Bug 191044

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

190754

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Devin Rousso

Reported 2018-10-29 15:10:57 PDT

Instead of showing a verbose CSS selector for nodes in imported results, we should attempt to see if they exist on the page (assuming that the metadata url either matches the current page or was not supplied).

Attachments
Patch (8.07 KB, patch) 2018-10-30 18:50 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (7.83 KB, patch) 2018-10-31 11:10 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (11.35 KB, patch) 2018-10-31 12:01 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (11.36 KB, patch) 2018-10-31 12:57 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (11.96 KB, patch) 2018-10-31 13:47 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-10-30 17:55:52 PDT

<rdar://problem/45687364>

Devin Rousso

Comment 2 2018-10-30 18:50:34 PDT

Created attachment 353454 [details] Patch

Blaze Burg

Comment 3 2018-10-31 09:23:36 PDT

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.

Devin Rousso

Comment 4 2018-10-31 10:31:10 PDT

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.

Blaze Burg

Comment 5 2018-10-31 10:47:47 PDT

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

Devin Rousso

Comment 6 2018-10-31 11:10:26 PDT

Created attachment 353509 [details] Patch

Devin Rousso

Comment 7 2018-10-31 12:01:10 PDT

Created attachment 353516 [details] Patch Fix test failures

Devin Rousso

Comment 8 2018-10-31 12:57:52 PDT

Created attachment 353522 [details] Patch Rebase

Devin Rousso

Comment 9 2018-10-31 13:47:56 PDT

Created attachment 353525 [details] Patch