webkitdir.pm::isQt() is not working properly in run-webkit-tests under Linux/Qt

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Julien Chaffraix]

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.

[Julien Chaffraix]

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()).

[Adam Roben (:aroben)]

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;
}

[Julien Chaffraix]

Created attachment 16302 [details]
Modified patch using the pattern of webkitdir.pm

[Adam Roben (:aroben)]

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.

[Julien Chaffraix]

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.

[Adam Roben (:aroben)]

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!

[Julien Chaffraix]

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.

[Adam Roben (:aroben)]

Comment on attachment 16327 [details]
Updated patch

r=me!

[David Kilzer (:ddkilzer)]

Committed revision 25670.

In the future, please be sure to use spaces instead of tabs in the ChangeLog.  Thanks!