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

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has asked  for review:
Bug 93958: Pageload tests should measure memory usage
https://bugs.webkit.org/show_bug.cgi?id=93958

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

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #10)
> Please add a test to
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/performance_tests/p
erftest_unittest.py#L103

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

Done.

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

Done.

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

Right. Done.

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

Done.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:270
> > +	     for result_class in ordered_results_keys:
> 
> ordered_results_keys is unnecessary. results.keys() would do.

Done.

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

I modified, done.