Bug 192905

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 BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: dewei_zhu, ews, glenn, rniwa, simon.fraser, slewis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch dewei_zhu: review+

Description Simon Fraser (smfr) 2018-12-19 17:14:31 PST
Add support to run-benchmark to use non-default copies of the browser apps
Comment 1 Simon Fraser (smfr) 2018-12-19 17:16:04 PST
Created attachment 357763 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-12-19 17:16:06 PST
<rdar://problem/46845840>
Comment 3 dewei_zhu 2018-12-19 17:32:50 PST
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
Comment 4 Stephanie Lewis 2018-12-19 17:46:25 PST
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 5 Ryosuke Niwa 2018-12-19 20:03:40 PST
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?
Comment 6 Simon Fraser (smfr) 2018-12-20 11:47:35 PST
--build-directory doesn't let me point at a custom Safari.app (e.g. an STP).
Comment 7 dewei_zhu 2018-12-20 13:59:45 PST
If you build the binary locally, --build-directory is expecting a folder than contains Safari.app. Also this folder should contain all related frameworks.
Comment 8 Simon Fraser (smfr) 2018-12-20 14:16:12 PST
I want to run against pre-built app bundles (spades). Yes, I'm aware of the perf implications.
Comment 9 Simon Fraser (smfr) 2018-12-20 15:35:06 PST
Created attachment 357879 [details]
Patch
Comment 10 Simon Fraser (smfr) 2018-12-20 15:47:40 PST
Created attachment 357883 [details]
Patch
Comment 11 dewei_zhu 2018-12-20 17:11:08 PST
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.
Comment 12 Simon Fraser (smfr) 2018-12-21 15:53:43 PST
https://trac.webkit.org/changeset/239522/webkit