[webkit-reviews] review granted: [Bug 73079] Web Inspector: chromium: I'd like to add a script for running perf tests for WebInspector. : [Attachment 117174] [patch] next iteration.

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted 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 117174: [patch] next iteration.
https://bugs.webkit.org/attachment.cgi?id=117174&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117174&action=review


Sorry for the confusion earlier. It's not clear to me that getting rid of
port.tests() and just doing this generically in manager.py is actually the
right thing to do, but I definitely didn't want a port/test_files.py to keep
existing, which is why I wanted you to inline it for now.

> Tools/Scripts/run-inspector-perf-tests.py:42
> +    sys.exit(PerfTestsRunner(_perf_tests_dir).run())

Nit: I'd probably just hard-code 'inspector' for now and remove the variable if
it isn't needed elsewhere, but it's up to you.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:470
> +	   return find_files.find(self.filesystem, self.layout_tests_dir(),
paths, skipped_directories, _is_test_file)

Please rename this to be _find_test_files() to make it clear that this is a
protected function, not a public function (see the comment below in
rebaseline.py as well; if this has to stay as a public function, that's fine).


> Tools/Scripts/webkitpy/layout_tests/port/base.py:1020
> +    return _has_supported_extension(filesystem, filename) and not
is_reference_html_file(filename)

Can you make _supported_file_extensions be a local variable and move all of
these functions next to tests() since they're related?

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:121
> +	   for test_name in map(self._to_test_name,
self._port.find_test_files(args)):

Does this work if it's just self._port.tests(args)? It seems like this
shouldn't be calling find_test_files (or, previously, test_files.find())
directly.