[webkit-reviews] review denied: [Bug 85410] Need tests for PerfTestRunner.computeStatistics : [Attachment 139867] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Tom Zakrajsek
<tomz at codeaurora.org>'s request for review:
Bug 85410: Need tests for PerfTestRunner.computeStatistics
https://bugs.webkit.org/show_bug.cgi?id=85410

Attachment 139867: Patch
https://bugs.webkit.org/attachment.cgi?id=139867&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139867&action=review


Thanks for the patch! I think we want one more iteration here.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:6
> +  <script src="../../fast/js/resources/js-test-pre.js"></script>
> +  <script src="../../../PerformanceTests/resources/runner.js"></script>
> +  <script type="text/javascript">

I would prefer not indent script elements like this.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:11
> +    /** Find the minimum value in an array of numbers.
> +	*
> +	* @param array	   input array of numbers
> +	* @return	   minimum value in array
> +	*/

We don't normally add per-function comment like this.
It makes the code more verbose than needed.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:12
> +    Array.min = function(array) {

You should probably check that Array doesn't already have min.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:13
> +	 return Math.min.apply(Math, array);

Please use 4-space indentation.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:33
> +	 array.sort( function(a,b) {return a - b;} );

Please add spaces between { and return, and ; and }

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:48
> +	 return Array.sum(array)/array.length;

Space needed around /.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:58
> +	 for (var a in array) {

Please don't use one-letter variable name like a.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:72
> +	 for (var a in array) {

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:95
> +	 for (var a in array) {

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:124
> +    /** Ensure no latent divide by 0's for an odd number of elements. */

Instead of adding comment like this. It's probably more helpful to put it in
debug().

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:138
> +    /** This test will catch if any order dependencies in the data, such as
> +	*  needing to be numerically sorted, are not resolved by the algorithm.

> +	*  This variant covers an odd number of elements.
> +	*/

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:159
> +    /** This test will catch if any order dependencies in the data, such as
> +	*  needing to be numerically sorted, are not resolved by the algorithm.

> +	*  This variant covers an odd number of elements, and negative values.
> +	*/

Ditto.

> LayoutTests/fast/harness/perf-runner-compute-statistics.html:204
> +  <script type="text/javascript">
> +    // runner.js marks this as an async test so we need to call notifyDone.
> +    layoutTestController.notifyDone();
> +  </script>

We can merge this into the script element above, no?