[webkit-reviews] review granted: [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 : [Attachment 115993] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
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
https://bugs.webkit.org/show_bug.cgi?id=66902

Attachment 115993: Patch
https://bugs.webkit.org/attachment.cgi?id=115993&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.