[webkit-reviews] review denied: [Bug 15223] webkitdir.pm::isQt() is not working properly in run-webbkit-test under Linux/Qt : [Attachment 16302] Modified patch using the pattern of webkitdir.pm

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has denied Julien Chaffraix
<julien.chaffraix at gmail.com>'s request for review:
Bug 15223: webkitdir.pm::isQt() is not working properly in run-webbkit-test
under Linux/Qt
http://bugs.webkit.org/show_bug.cgi?id=15223

Attachment 16302: Modified patch using the pattern of webkitdir.pm
http://bugs.webkit.org/attachment.cgi?id=16302&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
-    } 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.