[webkit-reviews] review denied: [Bug 134339] Add option to specify output file location in sunspider. : [Attachment 233889] patch

[bugzilla-daemon at webkit.org]

Csaba Osztrogonác <ossy at webkit.org> has denied Gergő Balogh
<gbalogh.u-szeged at partner.samsung.com>'s request for review:
Bug 134339: Add option to specify output file location in sunspider.
https://bugs.webkit.org/show_bug.cgi?id=134339

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

------- Additional Comments from Csaba Osztrogonác <ossy at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233889&action=review


The base idea is good, but r- now due to some minor issue.

> PerformanceTests/SunSpider/sunspider:45
> +my $outputPath = "";

We don't need this extra variable, we can use this one:
my $resultsFile = "";

> PerformanceTests/SunSpider/sunspider:61
> +  --output-path     If given set the output file and its path.

I prefer --output.

> PerformanceTests/SunSpider/sunspider:75
> +	      'output-path=s' => \$outputPath,

'output=s' => \$resultsFile,

> PerformanceTests/SunSpider/sunspider:131
>  my $resultsFile = "$resultDirectory/sunspider-results-$timeString.js";
> +if ($outputPath) {
> +  $resultsFile = $outputPath;
> +}

You can do it in one line:
$resultsFile = "$resultDirectory/sunspider-results-$timeString.js" unless
$resultsFile;

> Tools/Scripts/run-sunspider:65
> +  --output-path     If given set the output file and its path.

Simple --output would be better name, and the comment like "Override the
default output path and filename"