[webkit-reviews] review denied: [Bug 25101] run-webkit-tests should launch DumpRenderTree and the image diff tool in a way compatible with threads. : [Attachment 29346] Proposed fix.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 25101: run-webkit-tests should launch DumpRenderTree and the image diff
tool in a way compatible with threads.
https://bugs.webkit.org/show_bug.cgi?id=25101

Attachment 29346: Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=29346&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Overall this patch looks great, but I have two concerns:

1. The serialization of %ENV using regular expressions is very clever, but I
think it would be more straight-forward if Data::Dumper were used to do the
serializing, and "eval" to do the deserializing.  The Data::Dumper module is
present in all "modern" installations of Perl and it would simplify the code in
run-webkit-tests and the utility script (ExecAppWithEnv.pm).  Try this:

$ perl -e 'use Data::Dumper; my $d = Data::Dumper->new([\%ENV], [qw(ENV)]);
$d->Indent(0); $d->Purity(1); print $d->Dump();'

Also, instead of using "escape" and "unescape", I think it's clearer to use
"serialize" and "deserialize" when describing the process of moving %ENV to the
subprocess.

2. Please rename "ExecAppWithEnv.pm" to something that resembles a script name
like "execAppWithEnv".	Naming it "ExecAppWithEnv.pm" made me think it was a
Perl module, but it's not.  It's a Perl script that is invoked by
run-webkit-tests.

r- to resolve the above issues.  Thanks!