Bug 144038 – Add a script to run Speedometer and JetStream on a browser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)

Created attachment 251306 [details]
Patch

[WebKit Commit Bot]

Attachment 251306 [details] did not pass style-queue:


ERROR: Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/TwistedHTTPServer.py:4:  No name 'EncodingResourceWrapper' in module 'twisted.web.resource'  [pylint/E0611] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:9:  No name 'HTTPHandleFactory' in module 'HTTPHandle'  [pylint/E0611] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXSafariHandle.py:8:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXChromeHandle.py:8:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 251306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251306&action=review

Ryosuke is the right person to review this, just making a few initial comments.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:12
> ++        for (var i = 0; i < benchmarks.length; ++ i) {

no space between ++ and i

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:21
> ++        

Not needed.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:28
> ++        //submit result to server

Missing space after //. Missing capital letter for Submit and missing '.' at the end of the sentence.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:30
> ++        var http = new XMLHttpRequest();

We usually use xhr variable name for these rather than http I believe.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:31
> ++        var url = '/report';

No need for this variable.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:32
> ++        http.open("POST", url, true);

true can be omitted as async is the default.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:39
> ++        if(http.readyState == 4 && http.status == 200) {

xhr.readyState == XMLHttpRequest.DONE

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:40
> ++                closeRequest = new XMLHttpRequest();

missing var

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:41
> ++                closeRequest.open("GET", '/shutdown', true);

true can be omitted as async is the default.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:56
> ++ Copyright (C) 2014 Apple Inc. All rights reserved.

2015?

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:95
> ++        else

else should be on previous line.

> Tools/Scripts/PerfAutoRun/Patches/JetStream.patch:109
> ++    <p class="summary">JetStream is a JavaScript benchmark suite focused on the most advanced web applications. For more information, read the <a href="in-depth.html">in-depth analysis</a>. Bigger scores are better.</p>

s/Bigger/Higher ?

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:59
> +                            'Http Server is serving at port: %d',

HTTP

> Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXSafariHandle.py:60
> +        '/Users/Leelour/Code/Webkit/OpenSource/WebKitBuild/Release/',

?

[Chris Dumez]

Comment on attachment 251306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251306&action=review

> Tools/Scripts/PerfAutoRun/Patches/Speedometer.patch:74
> ++            console.log(dict);

debug? Do we want to keep this?

> Tools/Scripts/PerfAutoRun/Patches/Speedometer.patch:85
> ++            var http = new XMLHttpRequest();

xhr is a better name.

> Tools/Scripts/PerfAutoRun/Patches/Speedometer.patch:86
> ++            var url = '/report';

unnecessary variable

> Tools/Scripts/PerfAutoRun/Patches/Speedometer.patch:87
> ++            http.open("POST", url, true);

true can be omitted.

> Tools/Scripts/PerfAutoRun/Patches/Speedometer.patch:94
> ++                if(http.readyState == 4 && http.status == 200) {

XMLHttpRequest.DONE

> Tools/Scripts/PerfAutoRun/Patches/Speedometer.patch:95
> ++                    closeRequest = new XMLHttpRequest();

missing var

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:2
> +# coding : utf-8

I don't think we usually have this in our scripts.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:3
> +import argparse

A blank line before the imports would be nicer IMHO.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:29
> +            'OSX',

For command line arguments, I think it would be nice to avoid capital letters.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:33
> +    # should we add chrome as an option? Well, chrome uses webkit in iOS.

# FIXME: Should we...

(extra FIXME: and capital letter)

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:39
> +            'Safari',

For command line arguments, I think it would be nice to avoid capital letters.

[Chris Dumez]

Comment on attachment 251306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251306&action=review

> Tools/Scripts/PerfAutoRun/README.md:54
> +* **path/to/browser/directory**: should be the folder contains the executable binary(e.g. /Application/ in OSX which contains Safari.app)

s/contains/containing/
missing space before parenthesis.
s/in OSX/on OSX/

> Tools/Scripts/PerfAutoRun/README.md:58
> +To create a plan, you may need to refer to Plans/jetstream.plan.

s/may need to refer/may refer/

> Tools/Scripts/PerfAutoRun/README.md:80
> +    * **count**: the number of times you want to execute benchmark

s/to execute benchmark/to run the benchmark./

> Tools/Scripts/PerfAutoRun/README.md:81
> +    * **original_benchmark**: a relative path of benchmark to the root of this project ('PerfAutoRun' directory)

Path of the benchmark, relative to the root of this project.

> Tools/Scripts/PerfAutoRun/README.md:82
> +    * **benchmark_path**: (**OPTIONAL**) a relative path of patch to the root of this project ('PerfAutoRun' directory)

Path of patch to apply, relative to the root of this project.

> Tools/Scripts/PerfAutoRun/README.md:83
> +    * **entry_point**: the relative url you want browser to launch(a relative path to the webroot)

Missing space before parenthesis.

> Tools/Scripts/PerfAutoRun/README.md:84
> +    * **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

typo: availble

> Tools/Scripts/PerfAutoRun/README.md:85
> +    * **output_file**: specifiy the output file

Typo: specifiy

> Tools/Scripts/PerfAutoRun/README.md:89
> +    * when you launch the page('entry_point' in benchmark), the test will start automatically

Missing space before parenthesis.

> Tools/Scripts/PerfAutoRun/README.md:93
> +      var http = new XMLHttpRequest();

same comments as before for this XHR code sample.

> Tools/Scripts/PerfAutoRun/README.md:108
> +* create a patch file against original file

Missing capital letter.

> Tools/Scripts/PerfAutoRun/README.md:109
> +    * Go to the directory contains the benchmark directory(e.g. for JetStream, you should go to PerformaceTest folder)

Missing space before parenthesis.

[Chris Dumez]

Comment on attachment 251306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251306&action=review

> Tools/Scripts/PerfAutoRun/README.md:110
> +    * use ```shell git diff --relative HEAD >> your.patch``` to create your patch

what is "shell" ?

Shouldn't it be > your.patch ? so that it actually creates the file?

> Tools/Scripts/PerfAutoRun/README.md:112
> +* Create a plan for the benchmark(refer to **"How to create a plan"** for more details)

Missing space before parenthesis.

> Tools/Scripts/PerfAutoRun/README.md:113
> +* Do following instruction **ONLY IF NEEDED**, in most case, you do not have to.

Follow these instructions...
in most cases,

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:6
> +import json

Please sort the imports alphabetically.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:16
> +from Utils import timeout

Can be merged with the previous from Utils import.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:77
> +                # wait for http server to launch and platform handler to clean

Missing capital letter.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:78
> +                # the environment

Missing '.' at the end.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:94
> +                    logger.error('No result. Something goes wrong')

"went wrong" ?

> Tools/Scripts/PerfAutoRun/Utilities/GenericHandleFactory.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/GenericHandleFactory.py:6
> +import logging

alphabetical sorting.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPHandle.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPHandle.py:9
> +    def serve(self, webRoot, serveCnt):

What does "serveCnt" stand for? Please avoid abbreviations.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPHandleFactory.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/NaiveHTTPServer.py:2
> +# coding:utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/NaiveHTTPServer.py:7
> +import posixpath

alphabetical sorting.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/NaiveHTTPServer.py:69
> +        self.running = True

isRunning, we like using prefixes for boolean variables.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/TwistedHTTPServer.py:2
> +# coding: utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/TwistedHTTPServer.py:28
> +    backdoor = BackdoorPage()

Not sure backdoor is a great name. It sounds scary :)

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:24
> +        # FIXME: this may not be reliable

Missing '.' at the end.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:25
> +        logger.info('finding the ip address of current machine')

"Finding", "IP"

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:32
> +            logger.error('cannot get the ip address of current machine')

Missing capital letter.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:50
> +        logger.info('start to fetching the port number of the http server')

Missing capital letter.

> Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/SimpleHTTPHandle.py:88
> +                                'Http Server is serving at port: %d',

HTTP

> Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXChromeHandle.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXSafariHandle.py:35
> +        args = [browserBuildPath + '/Safari.app/Contents/MacOS/Safari']

os.path.join()

> Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXSafariHandle.py:50
> +        safaris = NSRunningApplication.runningApplicationsWithBundleIdentifier_(

"safariInstances"? or simply "instances"?

> Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/PlatformHandle.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/PlatformHandleFactory.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/ResultWrapper/ResultWrapperFactory.py:2
> +# coding : utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/Utils.py:2
> +# coding: utf-8

Same comment as before.

> Tools/Scripts/PerfAutoRun/Utilities/Utils.py:25
> +def getPathFromProjRoot(relativePathToProjRoot):

Avoid abbreviations, Proj -> Project

Created attachment 251460 [details]
Patch

[WebKit Commit Bot]

Attachment 251460 [details] did not pass style-queue:


ERROR: Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/JetStreamBenchmarkHandle.py:15:  [JetStreamBenchmarkHandle.prepareBenchmark] Use of super on an old style class  [pylint/E1002] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/JetStreamBenchmarkHandle.py:15:  [JetStreamBenchmarkHandle.prepareBenchmark] Use of super on an old style class  [pylint/E1002] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/HTTPHandle/HTTPServer/TwistedHTTPServer.py:4:  No name 'EncodingResourceWrapper' in module 'twisted.web.resource'  [pylint/E0611] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/BenchmarkRunner.py:14:  No name 'HTTPHandleFactory' in module 'HTTPHandle'  [pylint/E0611] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXSafariHandle.py:10:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/PerfAutoRun/Utilities/PlatformHandle/OSXChromeHandle.py:10:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
Total errors found: 6 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 251460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251460&action=review

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.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:2
> +# coding : utf-8

I don't think we want add these coding encoding since we don't do that elsewhere as Chris has already pointed out.

> Tools/Scripts/PerfAutoRun/PerfAutoRun.py:32
> +    parser.add_argument(
> +        '--platform',
> +        dest='platform',
> +        default='osx',
> +        choices=[
> +            'osx',
> +            'ios',
> +            'windows'],
> +        required=True)

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.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BaseBenchmarkHandle.py:2
> +# coding : utf-8

Ditto about coding.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BaseBenchmarkHandle.py:7
> +class BaseBenchmarkHandle(object):

Why do we need this class at all?
It seems like we just need GenericBenchmarkHandle.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandleFactory.py:7
> +from ..GenericHandleFactory import GenericHandleFactory

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 "..".

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandleFactory.py:17
> +    handles = json.load(
> +        open(
> +            os.path.join(
> +                os.path.dirname(__file__),
> +                handleFileName),
> +            'r'))

Again, we don't normally line break between each parenthesis like this.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/BenchmarkHandles.json:8
> +  "GenericBenchmarkHandle": {
> +    "moduleName": "GenericBenchmarkHandle", 
> +    "filePath": "BenchmarkHandle.GenericBenchmarkHandle"
> +  }, 
> +  "JetStreamBenchmarkHandle": {
> +    "moduleName": "JetStreamBenchmarkHandle", 
> +    "filePath": "BenchmarkHandle.JetStreamBenchmarkHandle"

Please do use 4-space indentation everywhere per our style guileline.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:18
> +class GenericBenchmarkHandle(BaseBenchmarkHandle):

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.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:19
> +    def prepareBenchmark(self, benchmarkPath, patch):

Why don't we call this just "prepare" since the class name already says GenericBenchmarkHandle.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:23
> +    def copyBenchmark(self, benchmarkPath):

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.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:24
> +        self.webroot = tempfile.mkdtemp()

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).

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:29
> +        self.dest = os.path.join(
> +            self.webroot,
> +            os.path.split(
> +                benchmarkPath)[1])

Seriously, all these line breaks are driving me nuts.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:35
> +    def applyPatch(self, patch):

_applyPatch.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:37
> +            oldCwd = os.getcwd()

Please use a descriptive name such as oldWorkingDirectory. "cwd" stands for current working directory
so oldCwd doesn't make much semantic sense.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:39
> +            retNum = subprocess.call(

I'd call this exitCode instead.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:44
> +                logger.info(
> +                    'Cannot apply patch, will skip current benchmarkPath')

It seems like this is an error, not an informative log, to me.

> Tools/Scripts/PerfAutoRun/Utilities/BenchmarkHandle/GenericBenchmarkHandle.py:50
> +    def clearBenchmark(self):
> +        logger.info('Cleanning Benchmark')

Why don't we call this clean?

> Tools/Scripts/PerfAutoRun/Utilities/ResultWrapper/ResultWrapperFactory.py:29
> +if __name__ == '__main__':
> +    merger = ResultWrapperFactory.create(['MergeResultWrapper'])
> +    d1 = {

Are these supposed to tests?
We usually put them in a separate tests directory.

Created attachment 251527 [details]
Patch

[WebKit Commit Bot]

Attachment 251527 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:17:  whitespace before '}'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:13:  No name 'http_server_driver_factory' in module 'http_server_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:15:  No name 'browser_driver_factory' in module 'browser_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:9:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
Total errors found: 10 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 251564 [details]
Patch

[WebKit Commit Bot]

Attachment 251564 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:13:  No name 'http_server_driver_factory' in module 'http_server_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:15:  No name 'browser_driver_factory' in module 'browser_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:9:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
Total errors found: 9 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 251564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251564&action=review

> Tools/Scripts/run-benchmark:9
> +logger.setLevel(logging.DEBUG)

We probably don't want to debug logging by default.
You probably want to add an option to control logging level instead, leaving default.

> Tools/Scripts/run-benchmark:25
> +    args = parser.parse_args()

Please add a blink line after this so that we can separate the list of arguments from the code below.
Probably another blank line is needed before BenchmarkRunner is instantiated too.

> Tools/Scripts/run-benchmark:26
> +    logger.info('Initializing program with following parameters')

I'd make these debug log since the user had just specified these options.

> Tools/Scripts/webkitpy/benchmark_runner/README.md:1
> +# PerfAutoRun

Please rename this.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:10
> +logger = logging.getLogger('PerfAutorun')

Do _log = logging.getLogger(__name__)

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:12
> +logger = logging.getLogger('PerfAutoRun')

Do _log = logging.getLogger(__name__)

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:27
> +        if patch:

We prefer early return over nested if's. Just do:
if patch:
  return self.webRoot
instead.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:32
> +            if errorCode != 0:

In WebKit coding styling, we don't use == or != against 0.
Just use "if errorCode:" instead.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/jetstream_benchmark_builder.py:10
> +logger = logging.getLogger('PerfAutoRun')

Do _log = logging.getLogger(__name__)

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/jetstream_benchmark_builder.py:25
> +        if errorCode != 0:

Ditto about not checking against 0.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:20
> +logger = logging.getLogger('PerfAutoRun')

Do _log = logging.getLogger(__name__)

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:55
> +                # Wait for http server to launch and platform handler to clean
> +                # the environment.

I don't think this comment adds any value.  Please remove it.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:56
> +                time.sleep(2)

Why are we sleeping for 2s?  Are we waiting for httpd to launch?
If so, we should instead check for pid file to be generated.
Waiting for 2s is neither sufficient nor necesary condition for httpd to have launched.
See how apache_http_server.py (somewhere in webkitpy) does this.
In fact, you might wanna just use that class with a new httpd.conf.

Also, we should probably be doing it in httpServerDriver.serve.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64
> +                if len(result) == 0:

No need to call len.  Just do "if not result:".

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:65
> +                    self.platformDriver.closeBrowsers()

Why don't we put this into "finally:" statment intead so that we don't have to repeat it three times.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:73
> +            time.sleep(1)

Why are we waiting for 1s?

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:10
> +logger = logging.getLogger('PerfAutoRun')

Do _log = logging.getLogger(__name__)

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:21
> +    logger.warn('Loading %s failed, start using pre-defined drivers' % (driverFileName))
> +    drivers = {
> +        'osx': {
> +            'safari': {'moduleName': 'OSXSafariDriver', 'filePath': 'browser_driver.osx_safari_driver'},
> +            'chrome': {'moduleName': 'OSXChromeDriver', 'filePath': 'browser_driver.osx_chrome_driver'},
> +        },
> +    }

I don't think we need a fallback code here.  It's just duplicating the code.
In fact, it's probably a good idea to do a full stop if the configuration file specified by the user doesn't exist.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:10
> +# We assume that this handle can only be used when the platform is OSX
> +from AppKit import NSRunningApplication
> +from browser_driver import BrowserDriver

Perhaps put this inside try & catch and spit out an error message instead?

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:13
> +logger = logging.getLogger('PerfAutoRun')

Do _log = logging.getLogger(__name__)

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:24
> +        # may need to be modified for develop build, such as setting up
> +        # libraries

It seems like this will fit into a single line.  Also use "FIXME:" prefix as done elsewhere.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:32
> +        time.sleep(2)

Why 2s?

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:10
> +# We assume that this handle can only be used when the platform is OSX.
> +from AppKit import NSRunningApplication
> +from browser_driver import BrowserDriver

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:13
> +logger = logging.getLogger('PerfAutoRun')

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:29
> +        # Stop for initialization of the safari process, otherwise, open
> +        # command may use the system safari.
> +        time.sleep(3)

Really?  It's unfortuante tht we don't have any other mechanism to ensure this :(

> Tools/Scripts/webkitpy/benchmark_runner/data/plans/jetstream.plan:15
> +{
> +    "http_server_driver": "SimpleHTTPServerDriver", 
> +    "benchmarks": [
> +        {
> +            "timeout" : 600,
> +            "count": 5,
> +            "benchmark_builder": "JetStreamBenchmarkBuilder",
> +            "original_benchmark": "../../../../PerformanceTests/JetStream",
> +            "benchmark_patch": "data/patches/JetStream.patch",
> +            "entry_point": "JetStream/JetStream-1.0.1/index.html",
> +            "result_wrapper": "MergeResultWrapper",
> +            "output_file": "jetstream.result"
> +        }
> +    ]
> +}

Now that we're making Builder subclasses, why don't we put this information as static class variables instead?

> Tools/Scripts/webkitpy/benchmark_runner/generic_factory.py:9
> +logger = logging.getLogger('PerfAutoRun')

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:81
> +                        p = subprocess.Popen(' '.join(['/usr/sbin/lsof',
> +                                                       '-a',
> +                                                       '-iTCP',
> +                                                       '-sTCP:LISTEN',
> +                                                       '-p',
> +                                                       str(self.serverProcess.pid)]),
> +                                             shell=True,
> +                                             stdout=subprocess.PIPE,
> +                                             stderr=subprocess.PIPE)

Wrong indentation. Each subsequent indentation is done by exactly 4 spaces.

> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:7
> +class DictionaryMerger(BaseResultWrapper):

I think this is unwarrent generization since this is the only class that inherits from BaseResultWrapper.
Just get rid of BaseResultWrapper and use this class directly.

In fact, this could just be a methods in benchmark_runner instead.

> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:9
> +        if len(dicts) == 0:

Ditto. Use if not dicts.

> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:10
> +logger = logging.getLogger('PerfAutoRun')

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/utils.py:8
> +logger = logging.getLogger('PerfAutoRun')

Ditto.

Created attachment 251599 [details]
Patch

Comment on attachment 251564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251564&action=review

>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:56
>> +                time.sleep(2)
> 
> Why are we sleeping for 2s?  Are we waiting for httpd to launch?
> If so, we should instead check for pid file to be generated.
> Waiting for 2s is neither sufficient nor necesary condition for httpd to have launched.
> See how apache_http_server.py (somewhere in webkitpy) does this.
> In fact, you might wanna just use that class with a new httpd.conf.
> 
> Also, we should probably be doing it in httpServerDriver.serve.

It have been proved that we do not have to sleep 2 seconds since the httpServerDriver will try to fetch the port number it is serving. Once it get the port number, this also indicates that the server is running.
I didn't find an easy way to use apache_http_server class as we want to have a '/shutdown' page which will terminate http server once it get hit.

>> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:10
>> +from browser_driver import BrowserDriver
> 
> Perhaps put this inside try & catch and spit out an error message instead?

Doing try-catch for this import makes no difference. We do throw exception in BrowserDriverFactory not BrowserDriver.

>> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:29
>> +        time.sleep(3)
> 
> Really?  It's unfortuante tht we don't have any other mechanism to ensure this :(

It's kind of a race condition. It would be great that we can find a way to merge two subprocess.Popen (line 26 and 30) in to one command.

>> Tools/Scripts/webkitpy/benchmark_runner/data/plans/jetstream.plan:15
>> +}
> 
> Now that we're making Builder subclasses, why don't we put this information as static class variables instead?

I think it would be better to follow the pattern of generic benchmark(e.g. speedometer). If we remove keys like 'original_benchmark' we need to add extra if conditions to check whether it is provided in '.plan' file.

>> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:81
>> +                                             stderr=subprocess.PIPE)
> 
> Wrong indentation. Each subsequent indentation is done by exactly 4 spaces.

My bad. I should put them in one line and this should have been fix since you've pointed it out last review.

>> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/merge_result_wrapper.py:7
>> +class DictionaryMerger(BaseResultWrapper):
> 
> I think this is unwarrent generization since this is the only class that inherits from BaseResultWrapper.
> Just get rid of BaseResultWrapper and use this class directly.
> 
> In fact, this could just be a methods in benchmark_runner instead.

I think I should call it ResultWrapperInterface rather than BaseResultWrapper. And since we provide the ability for others to import benchmarks, I think it would be better to allow them to define other wrappers.

[WebKit Commit Bot]

Attachment 251599 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:13:  No name 'browser_driver_factory' in module 'browser_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:14:  No name 'http_server_driver_factory' in module 'http_server_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:9:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
Total errors found: 9 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 251599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251599&action=review

> Tools/ChangeLog:7
> +

You should add a description as to what you're adding.

> 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

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.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:54
> +                self.browserDriver.launchUrl(self.composeUrl(self.httpServerDriver.baseUrl(), benchmark['entry_point']), self.buildDir)

Why don't we just use urlparse.urljoin?
https://docs.python.org/2/library/urlparse.html

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> +                        self.browserDriver.closeBrowsers()
> +                        _log.error('No result. Something went wrong')
> +                    self.browserDriver.closeBrowsers()

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?

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:74
> +    @classmethod
> +    def composeUrl(cls, baseUrl, relativePath):
> +        return '/'.join([baseUrl, relativePath])

We don't this function if we used urlparse.urljoin above.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:81
> +                fp.write(json.dumps(results))

json.dump(results, fp) will be more efficient.

> 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))

These should be errors.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:16
> +    if not drivers:
> +        raise Exception("No driver was found in %s" % (driverFileName))

Why don't we just do "assert drivers" instead of repeating the exception message twice?

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_drivers.json:17
> +    "ios": {
> +         "safari": {
> +            "moduleName": "iOSSafariDriver", 
> +            "filePath": "browser_driver.ios_safari_driver"
> +        }
> +    } 

Does this thing exist? It seems that shouldn't have this entry if this patch doesn't include iOS implementation.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server/twisted_http_server.py:24
> +    parser = argparse.ArgumentParser(
> +        description='python TwistedHTTPServer.py webroot')

Why don't we put this into a single line?

> 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]

This hsould all be in a single line.

> 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

Please pass the original exception object to raise as in:
except Exception:
…
    raise Exception
so that we get to see the original error.

> 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]

Please put this in single line.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:70
> +                    except ValueError:
> +                        pass
> +                    except IndexError:
> +                        pass

Perhaps we should add comments about why we're ignoring these errors.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:75
> +            except:
> +                raise

Pleaes don't do this.  This will prevent us from seeing the actual error.

> 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

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.

[Ryosuke Niwa]

(In reply to comment #18)
> Comment on attachment 251599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251599&action=review
>
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:74
> > +    @classmethod
> > +    def composeUrl(cls, baseUrl, relativePath):
> > +        return '/'.join([baseUrl, relativePath])
> 
> We don't this function if we used urlparse.urljoin above.

We don't *need*.

[Ryosuke Niwa]

Comment on attachment 251599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251599&action=review

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:16
> +    if not builders:
> +        raise Exception("No driver in %s was found" % (builderFileName))

Ditto about "assert builders" to avoid duplicating the error message.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:40
> +        except:
> +            raise

Oh I guess I was wrong about this supressing the exception since it will pass on the original exception object
but it doesn't seem like we don't do it elsewhere in our codebase so it's probably a good idea.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:65
> +                        _log.error('No result. Something went wrong')

It seems like when we're encountered this error, we need to exit early instead of contiuning to execute.
Otherwise, we'll end up still reproducing results with a fewer number of samples which is undesirable.

> Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:16
> +    if not drivers:
> +        raise Exception("No driver in %s was found" % (driverFileName))

Ditto about doing "assert drivers" to avoid duplicating the error message.

> Tools/Scripts/webkitpy/benchmark_runner/result_wrapper/result_wrapper_factory.py:17
> +    if not wrappers:
> +        raise Exception("No driver in %s was found" % (wrapperFileName))

Ditto about asserting to avoid code duplicate.

[Ryosuke Niwa]

Comment on attachment 251599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251599&action=review

> Tools/ChangeLog:3
> +        Add PerfAutoRun to automate browser based performance benchmarks(e.g. Speedometer and JetStream)

I've just updated the bug title since it's no longer called PerfAutoRun.
Plesae update this line in the change log before uploading a new patch.

Created attachment 251614 [details]
Patch

[WebKit Commit Bot]

Attachment 251614 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:15:  No name 'browser_driver_factory' in module 'browser_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:16:  No name 'http_server_driver_factory' in module 'http_server_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:8:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:9:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:8:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:8:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
Total errors found: 11 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 251614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251614&action=review

> Tools/ChangeLog:8
> +        Wrapper script to run benchmark

You need a period at the end of this sentence as well as a blank line afterwards.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66
> +                    else:
> +                        _log.error('No result. Something went wrong. Will skip current benchmark.')
> +                        break
> +                    self.browserDriver.closeBrowsers()

Why don't we just do all of this inside except: instead?
It's probably okay to repeat self.browserDriver.closeBrowsers() twice
if that meant we can get rid of this ugly None-ness check.

Created attachment 251619 [details]
Patch

[WebKit Commit Bot]

Attachment 251619 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:15:  No name 'browser_driver_factory' in module 'browser_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:16:  No name 'http_server_driver_factory' in module 'http_server_driver'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver_factory.py:8:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/generic_benchmark_builder.py:9:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:9:  No name 'NSRunningApplication' in module 'AppKit'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/benchmark_builder/benchmark_builder_factory.py:8:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:7:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/http_server_driver_factory.py:8:  No name 'benchmark_runner' in module 'webkitpy'  [pylint/E0611] [5]
Total errors found: 11 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 251619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251619&action=review

> Tools/ChangeLog:5
> +        Reviewed by Ryosuke Niwa

Btw, in the future, please this as NOBODY (OOPS!) until I set r+ on your patch.
Commit queue will automatically replace it with my name before committing it.
And it's actually wrong to say it has been reviewed by me since it means I've r+ed this patch, which I haven't done it until now.

[WebKit Commit Bot]

Comment on attachment 251619 [details]
Patch

Clearing flags on attachment: 251619

Committed r183309: <http://trac.webkit.org/changeset/183309>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.