[webkit-reviews] review denied: [Bug 73079] Web Inspector: chromium: I'd like to add a script for running perf tests for WebInspector. : [Attachment 116727] [patch] third version. style fix.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 73079: Web Inspector: chromium: I'd like to add a script for running perf
tests for WebInspector.
https://bugs.webkit.org/show_bug.cgi?id=73079

Attachment 116727: [patch] third version. style fix.
https://bugs.webkit.org/attachment.cgi?id=116727&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116727&action=review


> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:42
> +_perf_tests_base_dir = 'PerformanceTests'

Seems this should be moved onto the class too.

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:44
> +_result_regex = re.compile('^RESULT .*$')

This should be moved onto the class.

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:49
> +def run(perf_tests_dir, regular_output=sys.stderr,
buildbot_output=sys.stdout, port=None):
> +    runner = PerfTestsRunner(perf_tests_dir, regular_output,
buildbot_output, port)
> +    return runner.run()

Free functions are almost never right.

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:52
> +class PerfTestsRunner:

All modern python objects should inherit from object.  So this should read
"class PerfTestsRunner(object):"

Also, normally files match the name of the class they contain.	Not always. 
But it seems that run_perf_tests is needlessly different from perftestrunner.py


>> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:58
>> +	    self._port = port or
Host().port_factory.get(self._options.platform, self._options)
> 
> Eric should comment as to whether he's happy with this use of the Host()
constructor.

This is also an incomplete initialization of a Host object.  You'll end up w/o
a proper SCM.

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:87
> +	   filesystem = self._port.filesystem

port.filesystem is going away.	it woudl be better to have a self.host.

> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:94
> +    def run(self):
> +

Leading space?

>> Tools/Scripts/webkitpy/performance_tests/run_perf_tests.py:101
>> +		print >> sys.stderr, "Build not up to date for " +
self._port._path_to_driver()
> 
> Why not use _log ?

Or _printer?  Any time we're grabbing at sys.* directly we're making our code
less-mockable.