Move cursor to corner and fix safari window size before running benchmark
Created attachment 254681 [details] Patch
Attachment 254681 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:9: No name 'NSScreen' in module 'AppKit' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8: No name 'CGEventCreateMouseEvent' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:9: No name 'CGEventPost' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:10: No name 'kCGEventMouseMoved' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:11: No name 'kCGHIDEventTap' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:12: No name 'kCGMouseButtonLeft' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] Total errors found: 6 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254681&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:29 > + try: > + resolution = re.split('\s+', subprocess.check_output(['/usr/bin/defaults', 'read', 'com.apple.Safari', 'NSWindow Frame BrowserWindowFrame']).strip())[4:] > + subprocess.check_call(['/usr/bin/defaults', 'write', 'com.apple.Safari', 'NSWindow Frame BrowserWindowFrame', ' '.join(resolution * 2)]) > + except Exception as error: > + _log.error('Reset safari window size failed - Error: {}'.format(error)) What size are we setting the window to be? I think the window size needs to be configurable.
Comment on attachment 254681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254681&action=review >> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:29 >> + _log.error('Reset safari window size failed - Error: {}'.format(error)) > > What size are we setting the window to be? > I think the window size needs to be configurable. The return of "defaults read com.apple.Safari ...' is a list contains 8 numbers. And the last 4 numbers indicate the largest screen size, while the first 4 number indicate current configuration. If you overwrite first 4 numbers with the largest value, the safari will launch to maximum window.
Comment on attachment 254681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254681&action=review > Tools/ChangeLog:6 > + Reviewed by Ryosuke Niwa Was this really reviewed already? r? flag is set. Also, you are missing the period at the end of the sentence. > Tools/ChangeLog:7 > + Maybe a changelog to explain why this is needed and what you mean by fixing the Safari Window size? > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:53 > + def moveCursorToCorner(cls): Which corner? I could use something like "moveCursorToTopLeftCorner". Or a more generic "moveCursor(x, y)" that takes coordinates in parameters. > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:25 > + try: We may want to move this code to a function with a clear that so that it is clearer what it does. > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:26 > + resolution = re.split('\s+', subprocess.check_output(['/usr/bin/defaults', 'read', 'com.apple.Safari', 'NSWindow Frame BrowserWindowFrame']).strip())[4:] Do we really need to call subprocesses for this. I believe there are pythonic ways of doing this (NSUserDefaults.standardUserDefaults()). Maybe this is useful: http://www.macdevcenter.com/pub/a/mac/2003/01/31/pyobjc_one.html
Comment on attachment 254681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254681&action=review >> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:25 >> + try: > > We may want to move this code to a function with a clear that so that it is clearer what it does. ...with a clear *name*...
Comment on attachment 254681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254681&action=review >> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:53 >> + def moveCursorToCorner(cls): > > Which corner? I could use something like "moveCursorToTopLeftCorner". Or a more generic "moveCursor(x, y)" that takes coordinates in parameters. moveCursorTo(x,y) is a better name for my second proposal, sorry.
Created attachment 254760 [details] Patch
Comment on attachment 254760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254760&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:60 > + def getScreenSize(cls): In WebKit, we use "get" prefix only when we have an out argument. See http://www.webkit.org/coding/coding-style.html#names-out-argument So this function should be "screenSize" instead. > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:18 > + self.launchProcess(buildDir=browserBuildPath, appName='Google Chrome.app', url=url, args=['--args', '--homepage', url, '--window-size={0},{1}'.format(self.getScreenSize()[0], self.getScreenSize()[1])]) Please use named format e.g. '{width}{height}'.format(width=~, height=~).
Created attachment 254764 [details] Patch
Attachment 254764 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:7: No name 'NSScreen' in module 'AppKit' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8: No name 'CGEventCreateMouseEvent' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:9: No name 'CGEventPost' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:10: No name 'kCGEventMouseMoved' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:11: No name 'kCGHIDEventTap' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:12: No name 'kCGMouseButtonLeft' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] Total errors found: 6 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254764&action=review > Tools/ChangeLog:6 > + Reviewed by Ryosuke Niwa You shouldn't fill this up until you get r+ because it implies I've already given you r+, which I haven't. > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:25 > - self.launchProcess(buildDir=browserBuildPath, appName='Google Chrome Canary.app', url=url, args=['--args', '--homepage', url]) > + self.launchProcess(buildDir=browserBuildPath, appName='Google Chrome Canary.app', url=url, args=['--args', '--homepage', url, '--window-size={width},{height}'.format(width=self.screenSize()[0], height=self.screenSize()[1])]) Can we instead make self.screenSize() return a dictionary with width/height as keys? > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:24 > + self.moveCursor(0, 0) Instead of duplicating calls to closeBrowsers and moveCursor here, why don't we just call super(OSXSafariDriver, self).prepareEnv() ? > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:58 > + # resolution = re.split('\s+', NSUserDefaults.standardUserDefaults().persistentDomainForName_('com.apple.Safari')['NSWindow Frame BrowserWindowFrame'].strip())[4:] > + resolution = re.split('\s+', subprocess.check_output(['/usr/bin/defaults', 'read', 'com.apple.Safari', 'NSWindow Frame BrowserWindowFrame']).strip())[4:] Can't we just use self.screenSize here instead of user defaults craziness?
Created attachment 254771 [details] Patch
Attachment 254771 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:7: No name 'NSScreen' in module 'AppKit' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8: No name 'CGEventCreateMouseEvent' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:9: No name 'CGEventPost' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:10: No name 'kCGEventMouseMoved' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:11: No name 'kCGHIDEventTap' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:12: No name 'kCGMouseButtonLeft' in module 'Quartz.CoreGraphics' [pylint/E0611] [5] Total errors found: 6 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254771 [details] Patch Clearing flags on attachment: 254771 Committed r185483: <http://trac.webkit.org/changeset/185483>
All reviewed patches have been landed. Closing bug.