<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Use python 'with' statement to make cleanup more robust and minor code cleaning"
href="https://bugs.webkit.org/show_bug.cgi?id=147800">bug 147800</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #258543 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Use python 'with' statement to make cleanup more robust and minor code cleaning"
href="https://bugs.webkit.org/show_bug.cgi?id=147800#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Use python 'with' statement to make cleanup more robust and minor code cleaning"
href="https://bugs.webkit.org/show_bug.cgi?id=147800">bug 147800</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=258543&action=diff" name="attach_258543" title="Patch">attachment 258543</a> <a href="attachment.cgi?id=258543&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=258543&action=review">https://bugs.webkit.org/attachment.cgi?id=258543&action=review</a>
<span class="quote">> Tools/ChangeLog:7
> +</span >
Need some change description here.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:25
> +class built_benchmark(object):</span >
Class names should be CamelCase.
<span class="quote">> 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()</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:39
> +class test_environment(object):</span >
Ditto. This should be named TestEnvironment.
<span class="quote">> 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)</span >
I don't think it's a good design to modify runner's property directly like this
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:108
> + with test_environment(self), timeout(self._plan['timeout']):
> result = self._http_server_driver.fetch_result()</span >
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()</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>