[Webkit-unassigned] [Bug 147800] Use python 'with' statement to make cleanup more robust and minor code cleaning
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 7 16:52:22 PDT 2015
https://bugs.webkit.org/show_bug.cgi?id=147800
Ryosuke Niwa <rniwa at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #258543|review?, commit-queue? |review-, commit-queue-
Flags| |
--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 258543
--> https://bugs.webkit.org/attachment.cgi?id=258543
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=258543&action=review
> Tools/ChangeLog:7
> +
Need some change description here.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:25
> +class built_benchmark(object):
Class names should be CamelCase.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:36
> +
> + def __enter__(self):
> + self._runner._web_root = self._runner._benchmark_builder.prepare(self._runner._plan_name, self._runner._plan)
> +
> + def __exit__(self, exc_type, exc_value, traceback):
> + if self._runner._benchmark_builder:
> + self._runner._benchmark_builder.clean()
I don't think it makes much sense to have a separate class from BenchmarkBuilder just to implement __enter__ and __exit__.
Why can't we just add them to BenchmarkBuilder?
It's also strange for these two classes to rely on runner object.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:39
> +class test_environment(object):
Ditto. This should be named TestEnvironment.
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:48
> + self._runner._http_server_driver.serve(self._runner._web_root)
> + self._runner._browser_driver.prepare_env(self._runner._device_id)
> + url = urlparse.urljoin(self._runner._http_server_driver.base_url(), self._runner._plan_name + '/' + self._runner._plan['entry_point'])
> + self._runner._browser_driver.launch_url(url, self._runner._device_id)
I don't think it's a good design to modify runner's property directly like this
> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
> + with test_environment(self), timeout(self._plan['timeout']):
> result = self._http_server_driver.fetch_result()
It looks like _browser_driver is not accessed anywhere else so I don't think it needs to be be on _runner at all.
Ideally, I'd like see the code like this here:
with TestEnvironment(benchmark) as env, timeout(self._plan['timeout'])
result = env.fetch_results()
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150807/db5cc8d4/attachment.html>
More information about the webkit-unassigned
mailing list