Bug 66902 - 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
Summary: Provide option to disable Mac OS 10.7 application resume when using {debug, r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-24 15:27 PDT by Daniel Bates
Modified: 2011-12-23 15:48 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.08 KB, patch)
2011-08-24 15:41 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (5.32 KB, patch)
2011-08-26 11:17 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and unit test (12.31 KB, patch)
2011-08-29 20:24 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and unit test (12.31 KB, patch)
2011-08-29 20:26 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and unit test (12.31 KB, patch)
2011-08-29 20:33 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (7.31 KB, patch)
2011-11-20 12:11 PST, Daniel Bates
ddkilzer: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2011-08-24 15:27:51 PDT
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).
Comment 1 Daniel Bates 2011-08-24 15:41:25 PDT
Created attachment 105086 [details]
Patch
Comment 2 Mark Rowe (bdash) 2011-08-24 15:44:57 PDT
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.
Comment 3 Alexey Proskuryakov 2011-08-24 15:56:44 PDT
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.
Comment 4 Mark Rowe (bdash) 2011-08-24 15:59:48 PDT
Xcode already has a preference on the executable that controls this setting.
Comment 5 Daniel Bates 2011-08-26 11:16:57 PDT
(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.
Comment 6 Daniel Bates 2011-08-26 11:17:24 PDT
Created attachment 105383 [details]
Patch
Comment 7 Daniel Bates 2011-08-26 14:25:50 PDT
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.
Comment 8 Mark Rowe (bdash) 2011-08-29 17:03:36 PDT
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.
Comment 9 Daniel Bates 2011-08-29 20:24:29 PDT
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().
Comment 10 Daniel Bates 2011-08-29 20:26:54 PDT
Created attachment 105567 [details]
Patch and unit test

Updated patch; Sorted imports in script gdb-safari.
Comment 11 Daniel Bates 2011-08-29 20:33:02 PDT
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.
Comment 12 Daniel Bates 2011-10-05 01:07:12 PDT
Comment on attachment 105568 [details]
Patch and unit test

Clearing review flag as this patch no longer applies. Will rebase patch.
Comment 13 Daniel Bates 2011-11-20 12:11:43 PST
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.
Comment 14 David Kilzer (:ddkilzer) 2011-11-28 12:32:14 PST
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.
Comment 15 David Kilzer (:ddkilzer) 2011-11-28 13:25:34 PST
(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.
Comment 16 Eric Seidel (no email) 2011-12-21 14:30:09 PST
Attachment 115993 [details] was posted by a committer and has review+, assigning to Daniel Bates for commit.
Comment 17 Daniel Bates 2011-12-23 15:37:24 PST
(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".
Comment 18 Daniel Bates 2011-12-23 15:48:05 PST
Committed r103640: <http://trac.webkit.org/changeset/103640>