[webkit-reviews] review granted: [Bug 196710] Web Inspector: Audit: there should be a default test for WebInspectorAudit.Resources functionality : [Attachment 370948] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 196710: Web Inspector: Audit: there should be a default test for
WebInspectorAudit.Resources functionality
https://bugs.webkit.org/show_bug.cgi?id=196710

Attachment 370948: Patch

https://bugs.webkit.org/attachment.cgi?id=370948&action=review




--- Comment #25 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 370948
  --> https://bugs.webkit.org/attachment.cgi?id=370948
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370948&action=review

rs=me

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:285
> +			   } catch (e) {

Style: No need for the `(e)` if it is not used.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:287
> +			       if (key !== "__proto__")

Can we just skip __proto__ earlier, I don't think we'd want to set it.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:136
> +		   if (key === "domNodes" || key === "domAttributes" || key ===
"errors") {

Might be useful to make a static function for special attributes?

    AuditTestCaseResult.isSpecialAttribute(key)

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:36
> +	   this._resultDataNonSpecialContainer = null;

Instead of "NonSpecial" maybe "General" or "CustomData" would be a better name.

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:141
> +	       let nonSpecialData = Object.filter(resultData, (key) => key !==
"domNodes" && key !== "errors");

domAttributes?

> LayoutTests/inspector/unit-tests/object-utilities-expected.txt:26
> +PASS: filter should remove all entries where the key isn't in ["a","b","c"]

Nit: These normally end in a period.