[webkit-reviews] review canceled: [Bug 74836] Replace webkitperl/run-leaks_unittest/RunLeaks.pm with webkitperl/LoadAsModule.pm : [Attachment 119811] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has canceled Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 74836: Replace webkitperl/run-leaks_unittest/RunLeaks.pm with
webkitperl/LoadAsModule.pm
https://bugs.webkit.org/show_bug.cgi?id=74836

Attachment 119811: Patch
https://bugs.webkit.org/attachment.cgi?id=119811&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119811&action=review


>> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:36
>> +LoadAsModule::load("run-leaks");
> 
> I think this is definitely a step in the right direction (sharing code to
load Perl scripts as modules), but this implementation has a couple of
drawbacks:
> 
> 1. You need a separate LoadAsModule::load() call for each script.
> 2. Scripts to test are in the LoadAsModule package, not their own package.
> 3. If two scripts had to be loaded, any methods with the same name would
collide.
> 
> I think LoadAsModule should work like this where the first argument is the
package name, and the second is the script name:
> 
> use lib "..";
> use LoadAsModule qw(RunLeaks run-leaks);
> 
> There is a special "import" method that you may define that gets called when
there are arguments to the 'use' statement (see lib.pm for an example).
> 
> Maybe we can put the whole sub-package in a big eval statement?
> 
> package LoadAsModule;
> 
> [...]
> 
> sub import
> {
>     my ($self, $package, $script) = @_;
> 
>     eval q{
>	  package } . $package . q{;
> 
>	  use [...];
> 
>	  sub readFile($);
> 
>	  my $runLeaksPath = File::Spec->catfile(sourceDir(), "Tools",
"Scripts", "} . $script . q{");
>	  eval "sub {" . readFile($runLeaksPath) . "}";
> 
>	  sub readFile($) {
>	  [...]
>     };
> }
> 
> I'm not sure yet if this would work or not, though.
> 
> Maybe we can just do something more straight-forward without using a giant
eval statement.  I'm going to play with this a bit.

See the attached patch for how to do this.