[webkit-reviews] review denied: [Bug 93958] Pageload tests should measure memory usage : [Attachment 167802] proposed patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Zoltan Horvath <zoltan at webkit.org>'s
request for review:
Bug 93958: Pageload tests should measure memory usage
https://bugs.webkit.org/show_bug.cgi?id=93958

Attachment 167802: proposed patch
https://bugs.webkit.org/attachment.cgi?id=167802&action=review

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


Please add a test to
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/performance_tests/p
erftest_unittest.py#L103

> Tools/Scripts/webkitpy/performance_tests/perftest.py:218
> +    def calculate_statistics(self, input_list, unit):

I don't think input_list is a descriptive name. It just tells us the type of
this argument.
I'd rather call this argument "values" since that's the name we've been using.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:219
> +	   sorted_input_list = sorted(input_list)

and this one sorted_values.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:244
> +	   results = {}
> +	   results.setdefault(self.test_name(), {})
> +	   results[self.test_name()]['unit'] = 'ms'
> +	   results[self.test_name()]['results'] = []

Why don't we just do:
results = {self.test_name(): {'unit': 'ms', 'results': []}}
instead?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:263
> +		       results.setdefault(name, {})
> +		       results[name]['results'] = []

You can do:
results.setdefault(name, {'results': []})
instead.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:270
> +	   for result_class in ordered_results_keys:

ordered_results_keys is unnecessary. results.keys() would do.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:271
> +	       results[result_class] =
self.calculate_statistics(results[result_class]['results'],
results[result_class]['unit'])

It's not great to change the structure of results like this.