Bug 174390

Created attachment 315164 [details] Patch

Comment on attachment 315164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315164&action=review good job. Just fix up webdriver and websocket and I think we'll be good. > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:62 > + pass Probably should dump an error message if this version of the function gets called. Something about instantiating an abstract class > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:100 > + pass ditto > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:40 > + parser.add_argument('--driver', dest='driver', default='web-socket', choices=['web-driver', 'web-socket']) webdriver I think should be one word same for websocket

Created attachment 315286 [details] Patch

Comment on attachment 315286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315286&action=review > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:62 > + raise NotImplementedError('BenchmarkRunner is an abstract class and shouldn\'t be instantiated.') unnecessary 'pass'? Or would it make more sense to make it an abstractmethod? > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:102 > + pass It maybe cleaner to make execute function to be with BenchmarkBuilder(self._plan_name, self._plan, self.name) as web_root: self._run_benchmark(int(self._plan['count']), web_root) and add name = 'webdriver' on WebDriverBenchmarkRunner and 'websocket' on WebSocketBenchmarkRunner. > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:40 > + parser.add_argument('--driver', dest='driver', default='websocket', choices=['webdriver', 'websocket']) Maybe we could try something like benchmark_runner_map = { WebDriverBenchmarkRunner.name: WebDriverBenchmarkRunner, WebSocketBenchmarkRunner.name: WebSocketBenchmarkRunner, }; choices=benchmark_runner_map.keys() > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:96 > + if args.driver == 'webdriver': > + runner = WebDriverBenchmarkRunner(args.plan, args.localCopy, args.countOverride, args.buildDir, args.output, args.platform, args.browser, args.scale_unit, args.device_id) > + else: > + runner = WebSocketBenchmarkRunner(args.plan, args.localCopy, args.countOverride, args.buildDir, args.output, args.platform, args.browser, args.scale_unit, args.device_id) If we adopt 'benchmark_runner_map', then, there hat we can do is: benchmark_runner_class = benchmark_runner_map[args.driver] runner = benchmark_runner_class(args.plan, args.localCopy, args.countOverride, args.buildDir, args.output, args.platform, args.browser, args.scale_unit, args.device_id) > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:100 > + Remove this line > Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:8 > +from benchmark_runner import BenchmarkRunner > +from benchmark_builder import BenchmarkBuilder > +from selenium.webdriver.support.ui import WebDriverWait nit, import should be alphabetical Once execute is removed, BenchmarkBuilder is not needed in this file. > Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:15 > + name = 'webdriver' > Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:23 > + url = 'file://%s/%s/%s' % (web_root, self._plan_name, test_file) I would recommend to use str.format. > Tools/Scripts/webkitpy/benchmark_runner/webdriver_benchmark_runner.py:34 > + def execute(self): > + with BenchmarkBuilder(self._plan_name, self._plan, 'webdriver') as web_root: > + self._run_benchmark(int(self._plan['count']), web_root) Not necessary anymore. > Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:8 > +from benchmark_builder import BenchmarkBuilder Once execute is removed, this should be cleaned up as well. > Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:17 > + add name = 'websocket' > Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:21 > + BenchmarkRunner.__init__(self, plan_file, local_copy, count_override, build_dir, output_file, platform, browser, scale_unit=True, device_id=None) It would be better to use 'super(WebSocketBenchmarkRunner, self)' instead. > Tools/Scripts/webkitpy/benchmark_runner/websocket_benchmark_runner.py:44 > + def execute(self): > + with BenchmarkBuilder(self._plan_name, self._plan, 'websocket') as web_root: > + self._run_benchmark(int(self._plan['count']), web_root) Not needed anymore.

Created attachment 315873 [details] Patch

Created attachment 315879 [details] Patch

Comment on attachment 315879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315879&action=review > Tools/ChangeLog:6 > + Reviewed by Dewei Zhu. r=me.

Created attachment 315958 [details] Subclass Benchmark Runner script for WebDriver support

Comment on attachment 315958 [details] Subclass Benchmark Runner script for WebDriver support Rejecting attachment 315958 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 315958, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Dewei Zhu found in /Volumes/Data/EWS/WebKit/Tools/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.webkit.org/results/4181285

Created attachment 316334 [details] patch

Comment on attachment 316334 [details] patch Rejecting attachment 316334 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 316334, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: .webkit.org/git/WebKit e1d65cb..c668947 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 219851 = e1d65cbd37dbe81d09d4d06fec21583903fbd13e r219852 = c66894799395d770494683b1519ab41f7e4b007b Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.webkit.org/results/4181637

Created attachment 316344 [details] Patch

Comment on attachment 316344 [details] Patch Clearing flags on attachment: 316344 Committed r219857: <http://trac.webkit.org/changeset/219857>

All reviewed patches have been landed. Closing bug.