Bug 145858

Summary: Move cursor to corner and fix safari window size before running benchmark
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dewei_zhu, glenn, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description dewei_zhu 2015-06-10 15:03:51 PDT
Move cursor to corner and fix safari window size before running benchmark
Comment 1 dewei_zhu 2015-06-10 15:06:57 PDT
Created attachment 254681 [details]
Patch
Comment 2 WebKit Commit Bot 2015-06-10 15:09:44 PDT
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 3 Ryosuke Niwa 2015-06-10 19:09:01 PDT
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 4 dewei_zhu 2015-06-10 20:04:27 PDT
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 5 Chris Dumez 2015-06-10 20:36:00 PDT
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 6 Chris Dumez 2015-06-10 20:36:52 PDT
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 7 Chris Dumez 2015-06-10 20:38:34 PDT
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.
Comment 8 dewei_zhu 2015-06-11 14:50:40 PDT
Created attachment 254760 [details]
Patch
Comment 9 Ryosuke Niwa 2015-06-11 15:53:05 PDT
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=~).
Comment 10 dewei_zhu 2015-06-11 16:19:32 PDT
Created attachment 254764 [details]
Patch
Comment 11 WebKit Commit Bot 2015-06-11 16:20:35 PDT
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 12 Ryosuke Niwa 2015-06-11 16:30:47 PDT
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?
Comment 13 dewei_zhu 2015-06-11 17:33:44 PDT
Created attachment 254771 [details]
Patch
Comment 14 WebKit Commit Bot 2015-06-11 17:35:59 PDT
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 15 WebKit Commit Bot 2015-06-11 19:17:03 PDT
Comment on attachment 254771 [details]
Patch

Clearing flags on attachment: 254771

Committed r185483: <http://trac.webkit.org/changeset/185483>
Comment 16 WebKit Commit Bot 2015-06-11 19:17:07 PDT
All reviewed patches have been landed.  Closing bug.