Summary: | run-benchmark should close windows between iterations | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephanie Lewis <slewis> | ||||||
Component: | Tools / Tests | Assignee: | Stephanie Lewis <slewis> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, dewei_zhu, ews-watchlist, glenn, jbedard, lforschler, simon.fraser, slewis | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Stephanie Lewis
2019-03-06 20:26:53 PST
c is tricky because the solution involves AppleScript and it triggers the automation security feature that says automation can't send apple events without user approval. The prompt is a one time prompt so its not a big deal but this means bots can't run the apple-event code. So we also need to add an option to disable that codepath. Created attachment 363838 [details]
patch
Attachment 363838 [details] did not pass style-queue:
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:12: expected 2 blank lines, found 1 [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8: No name 'NSRunningApplication' in module 'AppKit' [pylint/E0611] [5]
Total errors found: 2 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Was there a different way to do this that no longer works? yes. Comment on attachment 363838 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=363838&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8 > +from AppKit import NSRunningApplication All the BrowserDriver classes will be loaded ahead. We should deferring this import still we use it otherwise, it may break on linux machines or Mac without AppKit module. > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:52 > + parser.add_argument('--no-apple-events', dest='allow_apple_events', action='store_false', default='store_true', help="Calling apple events requires user interation. Disable features which use apple events. Currently used for ensuring windows are closed in between test iterations.") The option name and dest are opposite which is confusing here. I think we should make it consistent here. Maybe call it --allow-apple-events and default to false? (In reply to dewei_zhu from comment #7) > Comment on attachment 363838 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363838&action=review > > > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:8 > > +from AppKit import NSRunningApplication > > All the BrowserDriver classes will be loaded ahead. We should deferring this > import still we use it otherwise, it may break on linux machines or Mac > without AppKit module. > > > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:52 > > + parser.add_argument('--no-apple-events', dest='allow_apple_events', action='store_false', default='store_true', help="Calling apple events requires user interation. Disable features which use apple events. Currently used for ensuring windows are closed in between test iterations.") > > The option name and dest are opposite which is confusing here. I think we > should make it consistent here. Maybe call it --allow-apple-events and > default to false? We want the default to be true and the use to have to explicitly disable the feature Alexey pointed out some prefs in run-safari to go test if (checkForArgumentAndRemoveFromARGV("--no-saved-state")) { push @args, ("-ApplePersistenceIgnoreStateQuietly", "YES"); # FIXME: Don't set ApplePersistenceIgnoreState once all supported OS versions respect ApplePersistenceIgnoreStateQuietly (rdar://15032886). push @args, ("-ApplePersistenceIgnoreState", "YES"); } so back to the drawing board Created attachment 363845 [details]
take2
Now with more preferences and less window closing
Attachment 363845 [details] did not pass style-queue:
ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:58: [OSXBrowserDriver._terminate_processes] No name 'NSRunningApplication' in module 'AppKit' [pylint/E0611] [5]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363845 [details] take2 View in context: https://bugs.webkit.org/attachment.cgi?id=363845&action=review > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_browser_driver.py:60 > + for app in NSRunningApplication.runningApplicationsWithBundleIdentifier_(bundle_id): How many apps are we expecting in this list? I think it's better to terminate them all first, then kill any which aren't terminated. It means that instead of waiting 2 * n seconds, you can just wait 2 seconds. > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:-25 > - self._safari_preferences = ["-HomePage", "about:blank", "-WarnAboutFraudulentWebsites", "0", "-ExtensionsEnabled", "0", "-ShowStatusBar", "0", "-NewWindowBehavior", "1", "-NewTabBehavior", "1"] This seems like a change outside the scope of this bug. Why are we moving it? The only reason I can think of is if the preferences were not respected if run after the OSXBrowserDriver constructor (which seems plausible) |