| Summary: | Add support to run-benchmark to use non-default copies of the browser apps | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dewei_zhu, ews-watchlist, glenn, rniwa, simon.fraser, slewis, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 199496 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Simon Fraser (smfr)
2018-12-19 17:14:31 PST
Created attachment 357763 [details]
Patch
Comment on attachment 357763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357763&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:40 > + elif self._browser_path: It's strange that launch_url takes browser_build_path as an argument but keep '_browser_path' as an instance variable. It seems cleaner to me to change the signature of launch_url to also take browser_path as an extra argument instead of changing the contractor of browser drivers. What do you think? > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:47 > + parser.add_argument('--browser-path', help='Specify the path to a non-default copy of the target browser as a path to the .app.') This may need to be mutual exclusive with '--build-directory'. Refer: https://docs.python.org/2.7/library/argparse.html#argparse.ArgumentParser.add_mutually_exclusive_group I agree with Dewei's comment about being consistent with browser_path and build_dir. I still think it's a bad idea to add this as an option. Using a bundle for performance testing can bite you in weird ways. Comment on attachment 357763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357763&action=review > Tools/ChangeLog:10 > + Add support for a --browser-path argument to run-benchmark, which allows you to use > + a custom app bundle for a given browser (only implemented for Safari at present). We already have --build-directory? How is this option different from that? --build-directory doesn't let me point at a custom Safari.app (e.g. an STP). If you build the binary locally, --build-directory is expecting a folder than contains Safari.app. Also this folder should contain all related frameworks. I want to run against pre-built app bundles (spades). Yes, I'm aware of the perf implications. Created attachment 357879 [details]
Patch
Created attachment 357883 [details]
Patch
Comment on attachment 357883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357883&action=review > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:49 > + > + group = parser.add_mutually_exclusive_group() > + group.add_argument('--browser-path', help='Specify the path to a non-default copy of the target browser as a path to the .app.') > + group.add_argument('--build-directory', dest='build_dir', help='Path to the browser executable (e.g. WebKitBuild/Release/).') > + Nit: It seems cleaner if we move this section below the last time we invoke parser.add_argument. |