change --no-launch-safari to --no-show-results in scripts
Created attachment 178078 [details] Patch
Comment on attachment 178078 [details] Patch Looks reasonable.
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");
(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");
Created attachment 178643 [details] rework to just pass --no-show-results to ORWT
new patch, much simpler ...
Comment on attachment 178643 [details] rework to just pass --no-show-results to ORWT OK.
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>
All reviewed patches have been landed. Closing bug.