Bug 174445 – Add WebDriver support in browser driver part of BenchmarkRunner

• Component: New Bugs
• Status: RESOLVED FIXED

[Matthew Stewart]

Add WebDriver support in browser driver part of BenchmarkRunner

[Matthew Stewart]

Created attachment 315317 [details]
Patch

[Stephanie Lewis]

Comment on attachment 315317 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:29
> +        args.insert(2, url)

This feels clunky.  Given this code is duplicated below I think it worth having helper function just to give it a name like insert_url_into_arguments.  Also rather than changing the args array itself I'd make a new array so we don't have to worry about the constants being in a bad state.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_firefox_driver.py:25
> +        args.insert(0, url)

ditto.  Your insert function should probably take a pos and return the new args array and can be used in both places.

[Build Bot]

Comment on attachment 315317 [details]
Patch

Attachment 315317 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4110917

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315328 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Matthew Stewart]

Created attachment 315719 [details]
Patch

[Stephanie Lewis]

Comment on attachment 315719 [details]
Patch

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

Otherwise patch looks awesome!

> Tools/ChangeLog:8
> +        * Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver.py:

This changelog is long enough that you should have a paragraph under the reviewed bit describing in more detail what it does.  Also some minimal annotation on the various functions to describe their purpose can be useful.

[Matthew Stewart]

Created attachment 315726 [details]
Patch

Comment on attachment 315726 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver.py:22
> +    def launch_driver(self, url, driver):
> +        pass

This function name is ambiguous with 'launch_driver(self, url, options, browser_build_path)'.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_minibrowsergtk_driver.py:43
> +        raise ValueError("Browser \"%s\" is not available with webdriver" % self.browser_name)

str.format is preferred.

[Matthew Stewart]

Created attachment 315955 [details]
Patch

Comment on attachment 315955 [details]
Patch

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

> Tools/ChangeLog:5
> +

r=me

[WebKit Commit Bot]

Comment on attachment 315955 [details]
Patch

Clearing flags on attachment: 315955

Committed r219850: <http://trac.webkit.org/changeset/219850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.