[webkit-reviews] review denied: [Bug 33153] [multi-patch]Running multiple instances of run-webkit-tests is not possible on the same machine : [Attachment 46673] Updated changelog to correctly show added module path

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Andras Becsi
<abecsi at inf.u-szeged.hu>'s request for review:
Bug 33153: [multi-patch]Running multiple instances of run-webkit-tests is not
possible on the same machine
https://bugs.webkit.org/show_bug.cgi?id=33153

Attachment 46673: Updated changelog to correctly show added module path
https://bugs.webkit.org/attachment.cgi?id=46673&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 * Scripts/webkitperl/httpd.pm: Added.

I'm not sure what the rules for placing Perl modules are. Some are in
WebKitTools/Scripts, others in WebKitTools/Scripts/webkitperl.

+my $iExploderLogDir = "/tmp/iExploderLogs";

We don't normally use abbreviations if we can avoid them -
iExploderLogDirectory would be better.

+configAndOpenHTTPDIfNeeded();

Ditto.

+    system "WebKitTools/Scripts/run-safari", "-NSOpen",
"$iExploderLogDir/redirect.html";

If it's named "log directory", then it's not OK to keep other things there.

-my $testDirectory = getcwd() . "/LayoutTests";
+my $testDirectory = catfile(getcwd(), "LayoutTests");

Perl documentation says that File::Spec subroutines should not be called
directly, but rather as class methods: File::Spec->catfile('a','b');. I'm not
sure what to make out of this.

+my $testResultsDirectory = catfile($tmpDir, "layout-test-results");

Ditto. This is repeated several more times in this patch.

+    my @httpdArgs = (
+    "-f", "$httpdConfig",
+    "-C", "DocumentRoot \"$documentRoot\"",

Seems like these should be indented.

+sub setHTTPDStandalone

It's not really clear what this function does, it needs a better name. Maybe
something related to "waitUntilQuit"?

+$isHttpdOpen = closeHTTPD();

closeHTTPD() doesn't explicitly return any value. Does this code do what it's
supposed to do in Perl?

+    # Setup a link to where the js test templates are stored, use -c so that
mod_alias will already be laoded.

This is just moved code, but: "loaded".

+my @defaultArgs = getDefaultConfigForTestDir($testDirectory);
+ at args = (@defaultArgs, @args);

This code looks fragile. Can there be some check for conflicting arguments?

This generally looks same, and is a great step in the right direction.