Bug 145858 – Move cursor to corner and fix safari window size before running benchmark

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Move cursor to corner and fix safari window size before running benchmark

Created attachment 254681 [details]
Patch

[WebKit Commit Bot]

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.

[Ryosuke Niwa]

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.

[Chris Dumez]

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

[Chris Dumez]

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*...

[Chris Dumez]

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

[Ryosuke Niwa]

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

[WebKit Commit Bot]

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.

[Ryosuke Niwa]

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

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 254771 [details]
Patch

Clearing flags on attachment: 254771

Committed r185483: <http://trac.webkit.org/changeset/185483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.