<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 PerfAutoRun 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 #251460 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)"
href="https://bugs.webkit.org/show_bug.cgi?id=144038#c9">Comment # 9</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add PerfAutoRun 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=251460&action=diff" name="attach_251460" title="Patch">attachment 251460</a> <a href="attachment.cgi?id=251460&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251460&action=review">https://bugs.webkit.org/attachment.cgi?id=251460&action=review</a>
This patch needs a change log entry. Run Tools/Scripts/prepare-ChangeLogs --bug=144038 and fill up the description.
r- because of this.
Can we also rename to something more redescriptive like BenchmarkRunner?
Also, I think we should put this code somewhere under Tools/Scripts/webkitpy.
<span class="quote">> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:2
> +# coding : utf-8</span >
I don't think we want add these coding encoding since we don't do that elsewhere as Chris has already pointed out.
<span class="quote">> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:32
> + parser.add_argument(
> + '--platform',
> + dest='platform',
> + default='osx',
> + choices=[
> + 'osx',
> + 'ios',
> + 'windows'],
> + required=True)</span >
Why don't we just put this in single line?
All these line breaks are rather making the code harder to read.
See other code in webkitpy.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BaseBenchmarkHandle.py:2
> +# coding : utf-8</span >
Ditto about coding.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BaseBenchmarkHandle.py:7
> +class BaseBenchmarkHandle(object):</span >
Why do we need this class at all?
It seems like we just need GenericBenchmarkHandle.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandleFactory.py:7
> +from ..GenericHandleFactory import GenericHandleFactory</span >
Why don't we add a wrapper script in Tools/Scripts/, e.g. run-benchmark-in-browser
so that we don't have to prefix these imports with "..".
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandleFactory.py:17
> + handles = json.load(
> + open(
> + os.path.join(
> + os.path.dirname(__file__),
> + handleFileName),
> + 'r'))</span >
Again, we don't normally line break between each parenthesis like this.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandles.json:8
> + "GenericBenchmarkHandle": {
> + "moduleName": "GenericBenchmarkHandle",
> + "filePath": "BenchmarkHandle.GenericBenchmarkHandle"
> + },
> + "JetStreamBenchmarkHandle": {
> + "moduleName": "JetStreamBenchmarkHandle",
> + "filePath": "BenchmarkHandle.JetStreamBenchmarkHandle"</span >
Please do use 4-space indentation everywhere per our style guileline.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:18
> +class GenericBenchmarkHandle(BaseBenchmarkHandle):</span >
I don't think we need to suffix all these classes with "Handle".
Just like "Manager", "Controller", etc… these suffixes don't really convey extra information.
Here, GenericBenchmark tells us just as much as GenericBenchmarkHandle.
Also, handle usually refers to a pointer like object such as a file handle so I don't think we want to use it here.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:19
> + def prepareBenchmark(self, benchmarkPath, patch):</span >
Why don't we call this just "prepare" since the class name already says GenericBenchmarkHandle.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:23
> + def copyBenchmark(self, benchmarkPath):</span >
copyBenchmark sounds very generic. Why don't we say that we're copying to a temporary location.
e.g. _copyBenchmarkToTempDir?
Also, we normally prefix private/protected method names with underscore.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:24
> + self.webroot = tempfile.mkdtemp()</span >
we should be consistent in naming conventions.
either webRoot or web_root (the latter is PEP 8 style we use elsewhere for local variables and methods).
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:29
> + self.dest = os.path.join(
> + self.webroot,
> + os.path.split(
> + benchmarkPath)[1])</span >
Seriously, all these line breaks are driving me nuts.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:35
> + def applyPatch(self, patch):</span >
_applyPatch.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:37
> + oldCwd = os.getcwd()</span >
Please use a descriptive name such as oldWorkingDirectory. "cwd" stands for current working directory
so oldCwd doesn't make much semantic sense.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:39
> + retNum = subprocess.call(</span >
I'd call this exitCode instead.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:44
> + logger.info(
> + 'Cannot apply patch, will skip current benchmarkPath')</span >
It seems like this is an error, not an informative log, to me.
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:50
> + def clearBenchmark(self):
> + logger.info('Cleanning Benchmark')</span >
Why don't we call this clean?
<span class="quote">> Tools/Scripts/PerfAutoRun/Utilities/ResultWrapper/ResultWrapperFactory.py:29
> +if __name__ == '__main__':
> + merger = ResultWrapperFactory.create(['MergeResultWrapper'])
> + d1 = {</span >
Are these supposed to tests?
We usually put them in a separate tests directory.</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>