Bug 191044 – Web Inspector: Audit: attempt to re-link DOM nodes for imported results

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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).

[Radar WebKit Bug Importer]

<rdar://problem/45687364>

[Devin Rousso]

Created attachment 353454 [details]
Patch

[Blaze Burg]

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 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 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]

Created attachment 353509 [details]
Patch

[Devin Rousso]

Created attachment 353516 [details]
Patch

Fix test failures

[Devin Rousso]

Created attachment 353522 [details]
Patch

Rebase

[Devin Rousso]

Created attachment 353525 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353525 [details]
Patch

Clearing flags on attachment: 353525

Committed r237656: <https://trac.webkit.org/changeset/237656>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.