Bug 192905 – Add support to run-benchmark to use non-default copies of the browser apps

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

[Simon Fraser (smfr)]

Add support to run-benchmark to use non-default copies of the browser apps

[Simon Fraser (smfr)]

Created attachment 357763 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/46845840>

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

[Stephanie Lewis]

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.

[Ryosuke Niwa]

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?

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

I want to run against pre-built app bundles (spades). Yes, I'm aware of the perf implications.

[Simon Fraser (smfr)]

Created attachment 357879 [details]
Patch

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/239522/webkit