Summary: | run-webkit-tests should launch DumpRenderTree and the image diff tool in a way compatible with threads. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Levin <levin> | ||||||
Component: | Tools / Tests | Assignee: | David Levin <levin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aroben, ddkilzer, eric, mrowe | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 10906 | ||||||||
Attachments: |
|
Description
David Levin
2009-04-08 14:22:53 PDT
Created attachment 29346 [details]
Proposed fix.
Yeah, this is something ddkilzer or bdash or darin adler would want to review (as our local perl gurus). Comment on attachment 29346 [details]
Proposed fix.
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!
Created attachment 29358 [details]
Proposed fix.
Addressed feedback.
Comment on attachment 29358 [details]
Proposed fix.
r=me Thanks!
Comment on attachment 29358 [details] Proposed fix. >+ * Scripts/execAppWithEnv.pm: Added. Oops! This should just be "Scripts/execAppWithEnv". :) Committed <http://trac.webkit.org/changeset/42359> |