Bug 15223 - webkitdir.pm::isQt() is not working properly in run-webkit-tests under Linux/Qt
Summary: webkitdir.pm::isQt() is not working properly in run-webkit-tests under Linux/Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-09-16 14:11 PDT by Julien Chaffraix
Modified: 2007-09-20 04:14 PDT (History)
0 users

See Also:


Attachments
proposed patch (3.71 KB, patch)
2007-09-16 14:47 PDT, Julien Chaffraix
aroben: review-
Details | Formatted Diff | Diff
Modified patch using the pattern of webkitdir.pm (3.04 KB, patch)
2007-09-16 17:26 PDT, Julien Chaffraix
aroben: review-
Details | Formatted Diff | Diff
Updated patch (2.41 KB, patch)
2007-09-18 11:32 PDT, Julien Chaffraix
aroben: review-
Details | Formatted Diff | Diff
Updated patch (3.29 KB, patch)
2007-09-19 12:50 PDT, Julien Chaffraix
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2007-09-16 14:11:42 PDT
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.
Comment 1 Julien Chaffraix 2007-09-16 14:47:51 PDT
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 2 Adam Roben (:aroben) 2007-09-16 15:28:05 PDT
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;
}
Comment 3 Julien Chaffraix 2007-09-16 17:26:23 PDT
Created attachment 16302 [details]
Modified patch using the pattern of webkitdir.pm
Comment 4 Adam Roben (:aroben) 2007-09-17 22:22:16 PDT
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.
Comment 5 Julien Chaffraix 2007-09-18 11:32:09 PDT
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 6 Adam Roben (:aroben) 2007-09-18 13:28:44 PDT
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!
Comment 7 Julien Chaffraix 2007-09-19 12:50:53 PDT
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 8 Adam Roben (:aroben) 2007-09-19 13:43:15 PDT
Comment on attachment 16327 [details]
Updated patch

r=me!
Comment 9 David Kilzer (:ddkilzer) 2007-09-20 04:14:25 PDT
Committed revision 25670.

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