<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 - Add a script to automate browser based performance benchmarks (e.g. Speedometer and JetStream)"
href="https://bugs.webkit.org/show_bug.cgi?id=144038">bug 144038</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 #251599 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add a script to automate browser based performance benchmarks (e.g. Speedometer and JetStream)"
href="https://bugs.webkit.org/show_bug.cgi?id=144038#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add a script to automate browser based performance benchmarks (e.g. Speedometer and JetStream)"
href="https://bugs.webkit.org/show_bug.cgi?id=144038">bug 144038</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=251599&action=diff" name="attach_251599" title="Patch">attachment 251599</a> <a href="attachment.cgi?id=251599&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251599&action=review">https://bugs.webkit.org/attachment.cgi?id=251599&action=review</a>
<span class="quote">> Tools/ChangeLog:7
> +</span >
You should add a description as to what you're adding.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/README.md:87
> + * **result_wrapper**: the wrapper module you want to wrap the results. Current availble option is "MergeResultWrapper". To customize your own result wrapper, extends Utilities/ResultWrapper/BaseResultWrapper.py and register your module in Utilities/ResultWrapper/ResultWrapper.json</span >
I really don't like that this is an option. It's a code smell to have generalization we don't need right now.
It's always better to implement the bare minimum we need and generalize things as needed.
Otherwise, we'll end up extra layers of abstractions and configurations we don't need and cause a maintenance nightmare down the road.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:54
> + self.browserDriver.launchUrl(self.composeUrl(self.httpServerDriver.baseUrl(), benchmark['entry_point']), self.buildDir)</span >
Why don't we just use urlparse.urljoin?
<a href="https://docs.python.org/2/library/urlparse.html">https://docs.python.org/2/library/urlparse.html</a>
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> + self.browserDriver.closeBrowsers()
> + _log.error('No result. Something went wrong')
> + self.browserDriver.closeBrowsers()</span >
Why do we need to call closerBrowsers twice when result is None?
It seems like we just want to call it once.
Also, it seems like checking None-ness of result here seems strange.
Why don't we just put "_log.error('No result. Something went wrong')" in "except:" clause instead?
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:74
> + @classmethod
> + def composeUrl(cls, baseUrl, relativePath):
> + return '/'.join([baseUrl, relativePath])</span >
We don't this function if we used urlparse.urljoin above.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:81
> + fp.write(json.dumps(results))</span >
json.dump(results, fp) will be more efficient.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:84
> + _log.info('Cannot open output file: %s' % outputFile)
> + _log.info('Results are:\n %s', json.dumps(results))</span >
These should be errors.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:16
> + if not drivers:
> + raise Exception("No driver was found in %s" % (driverFileName))</span >
Why don't we just do "assert drivers" instead of repeating the exception message twice?
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_drivers.json:17
> + "ios": {
> + "safari": {
> + "moduleName": "iOSSafariDriver",
> + "filePath": "browser_driver.ios_safari_driver"
> + }
> + } </span >
Does this thing exist? It seems that shouldn't have this entry if this patch doesn't include iOS implementation.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:24
> + parser = argparse.ArgumentParser(
> + description='python TwistedHTTPServer.py webroot')</span >
Why don't we put this into a single line?
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:29
> + self.ip = [
> + ip for ip in socket.gethostbyname_ex(
> + socket.gethostname())[2] if not ip.startswith("127.")][0]</span >
This hsould all be in a single line.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:33
> + except:
> + _log.error('Cannot get the ip address of current machine')
> + raise</span >
Please pass the original exception object to raise as in:
except Exception:
…
raise Exception
so that we get to see the original error.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:49
> + self.serverPort = psutil.Process(
> + self.serverProcess.pid).connections()[0][3][1]</span >
Please put this in single line.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:70
> + except ValueError:
> + pass
> + except IndexError:
> + pass</span >
Perhaps we should add comments about why we're ignoring these errors.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:75
> + except:
> + raise</span >
Pleaes don't do this. This will prevent us from seeing the actual error.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/base_result_wrapper.py:9
> +from abc import abstractmethod
> +
> +
> +class BaseResultWrapper(object):
> + @abstractmethod
> + def wrap(self, results):
> + pass</span >
I really don't think we should have this abstract class.
I can't think of any use case in which another implementation of merging results is needed.
The fact the type of results "wrapper" needs to be specified in the plan is an extra annoynace that's unwarrented.
r- because of this.</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>