webkitdir.pm::isQt() parses the variables given by the user and in case it cannot find out the option "--qt" it uses the variable ${QTDIR}. In run-webkit-tests and without defining ${QTDIR}, this would only work for the first call to it because GetOptions removes the options it can resolve from ARGV. In addition, without defining ${QTDIR}, the calling of build-dumprendertree from run-webkit-tests will not be able to detect Qt platform through isQt() because it is called in a new context without the argument from the main script.
Created attachment 16301 [details] proposed patch With this patch, when calling isQt (same for isGdk), the comparison is done once. The result is stored in a global variable and is given in the following code. The global variables ($isQt and $isGdk) are now exported to be used in buid-dumprendertree. The saving / regeneration of $isQt was removed in run-webkit-tests as it is not necessary anymore (done directly in isQt()).
Comment on attachment 16301 [details] proposed patch We prefer not to export global variables from perl modules. I think you should follow the "x()/determineX()" pattern we use throughout webkitdirs.pm: my $isQt; sub determineIsQt() { return if defined($isQt); $isQt = <something>; } sub isQt() { determineIsQt(); return $isQt; }
Created attachment 16302 [details] Modified patch using the pattern of webkitdir.pm
Comment on attachment 16302 [details] Modified patch using the pattern of webkitdir.pm - } else { - $buildResult = system "WebKitTools/Scripts/build-dumprendertree", $configurationOption; + #On linux, we give the platform to build-dumprendertree as a parameter + } elsif (isQt()) { + $buildResult = system "WebKitTools/Scripts/build-dumprendertree", $configurationOption, "--qt"; + } elsif (isGdk()) { + $buildResult = system "WebKitTools/Scripts/build-dumprendertree", $configurationOption, "--gdk"; } A couple of comments here: 1) It looks like you've broken this script on OS X here because there's no case that OS X would fall into now (before it fell into the else). 2) I think it would be nice to store the final argument in a variable so that we don't need all these elsifs. This would also solve problem number (1). +sub determineIsQt() { We normally put the opening brace of a function on its own line. +sub determineIsGdk () { Ditto, and we don't put a space before the (). Overall, this looks great, but r- so that we can fix this on OS X and these small style issues.
Created attachment 16313 [details] Updated patch Added a variable to hold the platform that is provided just with for linux platforms (Qt or Gdk). The build on MacOSX should now work. Corrected the coding style errors.
Comment on attachment 16313 [details] Updated patch +sub determineIsGdk () Please remove the space before the parentheses. + if (defined($platformParameter)) { + $buildResult = system "WebKitTools/Scripts/build-dumprendertree", $configurationOption, $platformParameter; + } else { + $buildResult = system "WebKitTools/Scripts/build-dumprendertree", $configurationOption; + } You don't need to check defined($platformParameter) here. You can just pass it to system, since passing undef is the same as not passing anything at all. Thanks for all the fixes so far!
Created attachment 16327 [details] Updated patch Corrected the coding style (I hope for good). Now the arguments are passed in a array to build-dumprendertree as seen with aroben.
Comment on attachment 16327 [details] Updated patch r=me!
Committed revision 25670. In the future, please be sure to use spaces instead of tabs in the ChangeLog. Thanks!