Bug 175126 – REGRESSION(r219850): run-benchmark script broken on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

The run-benchmark script dynamically generates the list of supported browsers and platforms (currently linux and osx) by loading all python files from Tools/Scripts/webkitpy/benchmark_runner/browser_driver and getting the browser_name and platform variables from the classes defined there.

This means, that this classes should never raise an exception when loaded on other platforms or otherwise they will broke the whole script. Its fine if they raise an exception when executing on any of the methods they implement, but not when just loading/importing the class.

So, yo avoid raising exceptions on another platforms we load the python modules that are platform specific on the functions rather than on the main file or the main class body. Example: https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py?rev=216213#L110

On r219850 <https://trac.webkit.org/r219850> a method for _screen_size() was added also for the OSX platform, but this method is called from the main file so its evaluated when the load of the classes is done to get the lists of platforms an browsers. Therefore it aborts on Linux like this:

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Perf%29/builds/850/steps/benchmark-test/logs/stdio

[Carlos Alberto Lopez Perez]

Created attachment 317111 [details]
Patch

[Matthew Stewart]

Comment on attachment 317111 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:17
> +        window_size_arg = '--window-size={width},{height}'.format(width=int(OSXBrowserDriver._screen_size().width), height=int(OSXBrowserDriver._screen_size().height))

Add this line inside create_chrome_options() as well

[Stephanie Lewis]

Comment on attachment 317111 [details]
Patch

Sorry for the breakage.  Rather than copying the code everywhere can we just create get_screen_size_method in browser_driver than OSX implements.  We should have done that in the first place

[Stephanie Lewis]

I didn't think that one through all the way.  I still think thats a long line to duplicate but that's irrelevant to the issue at hand.  If one of you upload a patch with the change Matthew requested I'll r+ it

[Carlos Alberto Lopez Perez]

(In reply to Stephanie Lewis from comment #3)
> Comment on attachment 317111 [details]
> Patch
> 
> Sorry for the breakage.  Rather than copying the code everywhere can we just
> create get_screen_size_method in browser_driver than OSX implements.  We
> should have done that in the first place

There is already a _screen_size() method in osx_browser_driver.

I will try to use a create_args() function on this firefox and chrome files to avoid duplicating code.

Patch incoming soon

[Carlos Alberto Lopez Perez]

Created attachment 317182 [details]
Patch

[Matthew Stewart]

Looks good to me!

[WebKit Commit Bot]

Comment on attachment 317182 [details]
Patch

Clearing flags on attachment: 317182

Committed r220246: <http://trac.webkit.org/changeset/220246>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33714151>