[Webkit-unassigned] [Bug 174331] Add setup script for Web Driver part of Benchmark Runner script

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174331

--- Comment #11 from Dean Johnson <dean_johnson at apple.com> ---
Comment on attachment 315508
  --> https://bugs.webkit.org/attachment.cgi?id=315508
Patch

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

Great work so far!

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:22
> +driver_executable = os.path.join(os.path.dirname(os.path.realpath(driver_init_file)), 'chromedriver')

Can you adopt this for geckodriver and chromedriver above, too? This seems very concise!

> Tools/Scripts/webkitpy/thirdparty/__init__.py:173
> +        version = subprocess.check_output(['curl', '--silent', CHROME_DRIVER_URL + "LATEST_RELEASE"]).strip()

We should use urllib2 here instead of subprocess. urllib2.urlopen(url).read() I believe should be all that's needed.

It may also be useful to have a function called install_binary (without a prepended _ to avoid getting captured in autoinstall_everything()), which you pass a url and it will download the binary and initialize it as a mock python object.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:174
> +        filename_postfix = self.get_driver_filename()[0]

Nit, instead of using indices, consider using _ as a placeholder like so:
filename_postfix, _ = self.get_driver_filename()

> Tools/Scripts/webkitpy/thirdparty/__init__.py:186
> +        filename_postfix = self.get_driver_filename()[1]

Ditto to _ comment.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:192
> +            open(os.path.join(directory, '__init__.py'), 'w+')

I think utilizing install_binary will reduce much of the duplication here.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:200
> +        import json

Nit: These imports can be done at the top of the file.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:202
> +        response = urlopen(json_url)

Nit: Usually only urllib2 functions all called from urllib2 rather than being imported directly. Example: urllib2.urlopen.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:208
> +    def get_driver_filename(self):

We should put this outside of this class since it's not dependent on self (except for get_os_info).

I would recommend rewriting this function for conciseness / flatness as follows:
def get_os_info():
  (os_name, os_type) = get_os_info()
  chrome_os, filefox_os = 'unsupported', 'unsupported'
  if os_name == 'Linux' and os_type == '64':
    chrome_os, firefox_os = 'linux64', 'linux64'
  elif os_name == 'Linux':
    chrome_os, firefox_os = 'linux32', 'linux32'
  elif os_name == 'Darwin':
    chrome_os, firefox_os = 'mac64', 'macos'
  return (chrome_os, firefox_os)

> Tools/Scripts/webkitpy/thirdparty/__init__.py:224
> +    def get_os_info(self):

Ditto to get_driver_filename. We should put this outside of this class.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170718/b572ffd6/attachment.html>