Bug 104298 – change --no-launch-safari to --no-show-results in scripts

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

change --no-launch-safari to --no-show-results in scripts

[Dirk Pranke]

Created attachment 178078 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 178078 [details]
Patch

Looks reasonable.

[Daniel Bates]

Comment on attachment 178078 [details]
Patch

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

> Tools/ChangeLog:13
> +        for ORWT (I didn't modify ORWT itself because making the arg parsing
> +        work turned out to be pretty awkward).

(*) Can you elaborate on the awkwardness?

> Tools/Scripts/run-webkit-tests:102
> +    if (grep(/--no-show-results/, @ARGV)) {
> +        push(@ARGV, "--no-launch-safari");
> +        @ARGV = grep { $_ ne "--no-show-results" } @ARGV;
> +    }

Assuming we take the approach of substituting --[no-]launch-safari for --[no-]show-results (see my question (*) above), I suggest that we take advantage of the convenience function webkitdirs::checkForArgumentAndRemoveFromARGV() instead of implementing similar functionality. As its name implies, webkitdirs::checkForArgumentAndRemoveFromARGV() checks for the presence of the specified command line argument and removes it if present. The function returns true if the argument was in the list of command line arguments; otherwise, it returns false. Simplifying this code using webkitdirs::checkForArgumentAndRemoveFromARGV(), we have:

push(@ARGV, "--no-launch-safari") if checkForArgumentAndRemoveFromARGV("--no-show-results");

> Tools/Scripts/run-webkit-tests:106
> +    if (grep(/--show-results/, @ARGV)) {
> +        push(@ARGV, "--launch-safari");
> +        @ARGV = grep { $_ ne "--show-results" } @ARGV;
> +    }

Assuming we take the approach of substituting --[no-]-launch-safari for --[no-]show-results (see my question (*) above), we can similarly write this using webkitdirs::checkForArgumentAndRemoveFromARGV() as:

push(@ARGV, "--launch-safari") if checkForArgumentAndRemoveFromARGV("--show-results");

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 178078 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178078&action=review
> 
> > Tools/ChangeLog:13
> > +        for ORWT (I didn't modify ORWT itself because making the arg parsing
> > +        work turned out to be pretty awkward).
> 
> (*) Can you elaborate on the awkwardness?
> 

I wasn't sure if you meant here or in the ChangeLog, but when I initially looked at the perl arg-parsing code I didn't see a way to easily make one option an alias for another, and since we displayed the default values for --launch-safari as well it looked like it was going to take a bunch of searching-and-replacing in multiple locations to make things work. However, I just re-checked the Getopt::Long perldoc and it looks like you can define a custom subroutine to make the args aliases, so I'll try that and if it works push all of this down into old-run-webkit-tests so we don't need to do the substitution any more.

Failing all that, your suggestion below sounds fine.

> > Tools/Scripts/run-webkit-tests:102
> > +    if (grep(/--no-show-results/, @ARGV)) {
> > +        push(@ARGV, "--no-launch-safari");
> > +        @ARGV = grep { $_ ne "--no-show-results" } @ARGV;
> > +    }
> 
> Assuming we take the approach of substituting --[no-]launch-safari for --[no-]show-results (see my question (*) above), I suggest that we take advantage of the convenience function webkitdirs::checkForArgumentAndRemoveFromARGV() instead of implementing similar functionality. As its name implies, webkitdirs::checkForArgumentAndRemoveFromARGV() checks for the presence of the specified command line argument and removes it if present. The function returns true if the argument was in the list of command line arguments; otherwise, it returns false. Simplifying this code using webkitdirs::checkForArgumentAndRemoveFromARGV(), we have:
> 
> push(@ARGV, "--no-launch-safari") if checkForArgumentAndRemoveFromARGV("--no-show-results");
> 
> > Tools/Scripts/run-webkit-tests:106
> > +    if (grep(/--show-results/, @ARGV)) {
> > +        push(@ARGV, "--launch-safari");
> > +        @ARGV = grep { $_ ne "--show-results" } @ARGV;
> > +    }
> 
> Assuming we take the approach of substituting --[no-]-launch-safari for --[no-]show-results (see my question (*) above), we can similarly write this using webkitdirs::checkForArgumentAndRemoveFromARGV() as:
> 
> push(@ARGV, "--launch-safari") if checkForArgumentAndRemoveFromARGV("--show-results");

[Dirk Pranke]

Created attachment 178643 [details]
rework to just pass --no-show-results to ORWT

[Dirk Pranke]

new patch, much simpler ...

[Eric Seidel (no email)]

Comment on attachment 178643 [details]
rework to just pass --no-show-results to ORWT

OK.

[WebKit Review Bot]

Comment on attachment 178643 [details]
rework to just pass --no-show-results to ORWT

Clearing flags on attachment: 178643

Committed r137237: <http://trac.webkit.org/changeset/137237>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.