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
Created attachment 317111 [details] Patch
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
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
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
(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
Created attachment 317182 [details] Patch
Looks good to me!
Comment on attachment 317182 [details] Patch Clearing flags on attachment: 317182 Committed r220246: <http://trac.webkit.org/changeset/220246>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33714151>