[webkit-reviews] review granted: [Bug 184071] AX: Audit Tab should have an Audit Manager : [Attachment 344826] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Aaron Chu <aaron_chu at apple.com>'s
request for review:
Bug 184071: AX: Audit Tab should have an Audit Manager
https://bugs.webkit.org/show_bug.cgi?id=184071

Attachment 344826: Patch

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




--- Comment #19 from Brian Burg <bburg at apple.com> ---
Comment on attachment 344826
  --> https://bugs.webkit.org/attachment.cgi?id=344826
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:38
> +	       if (newTestSuite instanceof WI.AuditTestSuite)

This is a programming error if suite is of the wrong instance, but as written
this will fill in `undefined` elements into this._testSuites. You should throw
an exception in this case instead.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:47
> +	   return [...this._reports.values()];

Newlines not necessary.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:87
> +	       // Making AuditReport read-only after all the AuditResults have
been received.

Nit: 'Make'

> Source/WebInspectorUI/UserInterface/Main.html:865
> +    <script src="Controllers/AuditManager.js"></script>

Nit: please alphabetize this if it does not really need to be ordered after
these other files.

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:40
> +    get representedTestCase() { return this._representedTestCases.slice(); }

This returns an array but the getter sounds like its singular. Please make it
plural?

> Source/WebInspectorUI/UserInterface/Models/AuditReport.js:67
> +	   return this._results.slice().filter(result => result.failed);

Unless you are using this elsewhere, please just inline it into get
failedCount().

> Source/WebInspectorUI/UserInterface/Test.html:231
>  

Ditto the above comment.

> LayoutTests/inspector/audit/audit-manager.html:37
> +	       auditManager.addTestSuite(testSuiteFixture1);

I would prefer that this throw an exception and not be allowed, as it seems to
only be possible as a programming error.

> LayoutTests/inspector/audit/audit-report.html:5
> +<script src="./resources/audit-test-fixtures.js"></script>

Nit: don't need the leading ./

> LayoutTests/inspector/audit/audit-report.html:9
> +    

Nit: extra newline

> LayoutTests/inspector/audit/audit-test-case.html:9
> +

Ditto.