Provide option to disable Mac OS 10.7 application resume when using {debug, run}-{safari, minibrowser, test-runner, test-webkit-api}, and run-webkit-app

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

Since Mac OS 10.7, Safari resumes its last saved state on launch. I propose that by default run-safari/debug-safari disables resuming for Safari with the assumption that a person that explicitly launches such a second instance of Safari most likely intends to have a different working set of web pages.

For example, I frequently run both shipping Safari and Safari with a development build of WebKit (via run-safari --debug) where the former instance is used for daily browsing/reading a W3C spec. and the latter is used for debugging an issue.

While it's sufficient to pass -ApplePersistenceIgnoreState YES to run-safari/debug-safari to disable the resume feature, I propose making this the default behavior. That is, when the command line argument -ApplePersistenceIgnoreState isn't specified we default to -ApplePersistenceIgnoreState YES (disable application resume).

[Daniel Bates]

Created attachment 105086 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 105086 [details]
Patch

I don’t think it’s a great idea to require that someone remember the user default name in order to reenable this functionality.  An explicit command line argument would be much more friendly.

[Alexey Proskuryakov]

I don't feel about this strongly, but I would be slightly unhappy if run-safari had a different behavior than launching Safari from Xcode in this regard (or even if I had to add this argument in Xcode project settings on each of my machines for consistency).

That said, having all windows reopen when launching Safari for debugging is generally quite unfortunate.

[Mark Rowe (bdash)]

Xcode already has a preference on the executable that controls this setting.

[Daniel Bates]

(In reply to comment #4)
> Xcode already has a preference on the executable that controls this setting.

Notice that we don't have such an option when using our command line tools to launch Safari.

[Daniel Bates]

Created attachment 105383 [details]
Patch

[Daniel Bates]

Comment on attachment 105383 [details]
Patch

Based on Mark Rowe's suggestion, I've added an optional command line argument, called restore-state (disabled by default) that can be passed to {run, debug}-safari.

[Mark Rowe (bdash)]

Comment on attachment 105383 [details]
Patch

This appears to require that one memorize the user default name if you want to use gdb-safari rather than debug-safari.  I think it would make sense to push the option handling down to gdb-safari.

[Daniel Bates]

Created attachment 105566 [details]
Patch and unit test

Updated patch based on Mark Rowe's suggestion. Also added a unit test.

I am open to suggestions on the name of the function checkForApplePersistenceIgnoreStateArgumentOrInsertIntoArrayRef().

[Daniel Bates]

Created attachment 105567 [details]
Patch and unit test

Updated patch; Sorted imports in script gdb-safari.

[Daniel Bates]

Created attachment 105568 [details]
Patch and unit test

Fix if-statement condition in script gdb-safari. By default, we don't want Safari to restore state.

[Daniel Bates]

Comment on attachment 105568 [details]
Patch and unit test

Clearing review flag as this patch no longer applies. Will rebase patch.

[Daniel Bates]

Created attachment 115993 [details]
Patch

I chose to make no-saved-state an optional command line argument for now instead of making this behavior the default. If we discover that people regularly pass no-saved-state to these tools then we can change the default behavior to always disable application resume and make state restoration the exception.

[David Kilzer (:ddkilzer)]

Comment on attachment 115993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115993&action=review

r=me

> Tools/ChangeLog:4
> +        Provide option to disable Mac OS 10.7 application resume when using
> +        {debug, run}-{safari, minibrowser, test-runner, test-webkit-api}, and run-webkit-app

Nit:  Should mention gdb-safari here as well.

> Tools/ChangeLog:18
> +        * Scripts/debug-minibrowser: Call printHelpAndExitForRunAndDebugWebKitAppIfNeeded()
> +        to print a help message and exit(3) if the command line argument --help was given.

Nit: Don't you mean 'exit(1)' not 'exit(3)' here?

> Tools/Scripts/webkitdirs.pm:59
>         &productDir
> +       &printHelpAndExitForRunAndDebugWebKitAppIfNeeded

The new method should appear before &prodcutDir to keep the list alphabetized.

> Tools/Scripts/webkitdirs.pm:2062
> +    push @args, ("-ApplePersistenceIgnoreState", "YES") if isLion() && checkForArgumentAndRemoveFromArrayRef("--no-saved-state", \@args);

Nit: This means that "--saved-state" is not a valid option, but that's probably okay for now.

It's rather unfortunate that we're doing our own argument parsing in these Perl scripts instead of using GetOpt::Long or GetOpt::Std, but it's hard to distribute the argument checking the way scripts (and library) methods are currently written.  It's also hard to get a list of valid options for a given script because of this same issue.  In the future, we may want to think about how to unify this argument parsing and help printing.  (I'm sure this problem has been solved before as well; maybe we can find a solution from another open source Perl project.)  Just something to think about--it shouldn't block this patch.

> Tools/Scripts/webkitdirs.pm:2094
> -    exec { $gdbPath } $gdbPath, @architectureFlags, $appPath or die;
> +    exec { $gdbPath } $gdbPath, @architectureFlags, $appPath, argumentsForRunAndDebugMacWebKitApp() or die;

Nit: This changes the list of arguments passed to gdb to include a post-processed @ARGV.  That's probably okay, but could break some one relying on the old behavior of ignoring "unknown" arguments.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (From update of attachment 115993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115993&action=review
> 
> > Tools/Scripts/webkitdirs.pm:2094
> > -    exec { $gdbPath } $gdbPath, @architectureFlags, $appPath or die;
> > +    exec { $gdbPath } $gdbPath, @architectureFlags, $appPath, argumentsForRunAndDebugMacWebKitApp() or die;
> 
> Nit: This changes the list of arguments passed to gdb to include a post-processed @ARGV.  That's probably okay, but could break some one relying on the old behavior of ignoring "unknown" arguments.

This is Bug 72829.

[Eric Seidel (no email)]

Attachment 115993 [details] was posted by a committer and has review+, assigning to Daniel Bates for commit.

[Daniel Bates]

(In reply to comment #14)
> (From update of attachment 115993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115993&action=review
> [...]
> > Tools/ChangeLog:4
> > +        Provide option to disable Mac OS 10.7 application resume when using
> > +        {debug, run}-{safari, minibrowser, test-runner, test-webkit-api}, and run-webkit-app
> 
> Nit:  Should mention gdb-safari here as well.

No, it should not mention gdb-safari because gdb-safari was removed in <http://trac.webkit.org/changeset/97562>. 

> 
> > Tools/ChangeLog:18
> > +        * Scripts/debug-minibrowser: Call printHelpAndExitForRunAndDebugWebKitAppIfNeeded()
> > +        to print a help message and exit(3) if the command line argument --help was given.
> 
> Nit: Don't you mean 'exit(1)' not 'exit(3)' here?

No, I didn't mean to refer to the actual code in the patch here, "exit(1)". Instead, I wanted to refer to the exit() function call. I'll change "exit(1)" to "exit()" here.

> 
> > Tools/Scripts/webkitdirs.pm:59
> >         &productDir
> > +       &printHelpAndExitForRunAndDebugWebKitAppIfNeeded
> 
> The new method should appear before &prodcutDir to keep the list alphabetized.

Will fix before landing.

> 
> > Tools/Scripts/webkitdirs.pm:2062
> > +    push @args, ("-ApplePersistenceIgnoreState", "YES") if isLion() && checkForArgumentAndRemoveFromArrayRef("--no-saved-state", \@args);
> 
> Nit: This means that "--saved-state" is not a valid option, but that's probably okay for now.
> 
> It's rather unfortunate that we're doing our own argument parsing in these Perl scripts instead of using GetOpt::Long or GetOpt::Std, but it's hard to distribute the argument checking the way scripts (and library) methods are currently written.  It's also hard to get a list of valid options for a given script because of this same issue.  In the future, we may want to think about how to unify this argument parsing and help printing.  (I'm sure this problem has been solved before as well; maybe we can find a solution from another open source Perl project.)  Just something to think about--it shouldn't block this patch.
> 

Yes, it would be beneficial to standardize on a common argument parsing solution that we can use throughout our scripts.

> > Tools/Scripts/webkitdirs.pm:2094
> > -    exec { $gdbPath } $gdbPath, @architectureFlags, $appPath or die;
> > +    exec { $gdbPath } $gdbPath, @architectureFlags, $appPath, argumentsForRunAndDebugMacWebKitApp() or die;
> 
> Nit: This changes the list of arguments passed to gdb to include a post-processed @ARGV.  That's probably okay, but could break some one relying on the old behavior of ignoring "unknown" arguments.

Yes, having debug-safari pass any additional command line arguments through to gdb is a change of behavior. Lets see if how people react to this new behavior. If people find this behavior unfavorable then we can remove it.

For completeness, this comment is more applicable to bug #72829, which added this behavior to debug-safari. This patch modifies the functionality added by the patch for bug #72829 by processing @ARGV as part of the machinery to support the optional command line argument "--no-saved-state".

[Daniel Bates]

Committed r103640: <http://trac.webkit.org/changeset/103640>