[webkit-reviews] review denied: [Bug 25517] build-webkit script should print build time at end : [Attachment 38883] Patch for bug 25517 : build-webkit script should print build time at end

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Laurent Cerveau
<lcerveau at me.com>'s request for review:
Bug 25517: build-webkit script should print build time at end
https://bugs.webkit.org/show_bug.cgi?id=25517

Attachment 38883: Patch for bug 25517 : build-webkit script should print build
time at end
https://bugs.webkit.org/attachment.cgi?id=38883&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
This is a great first patch!  Some comments below:

> +2009-09-01  Laurent Cerveau	<lcerveau at me.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	    <http://webkit.org/b/25517> build-webkit script should print build
time at end
> +
> +	   * Scripts/build-webkit:
> +		Added startTime and endTime variable so that the build time is
computed and printed as
> +		part of the build message. Proper display formatting (hours,
min, secs).

Lines should be indented by exactly 8 spaces.  There appears to be 9 spaces or
2 tabs used above.

> +my $endTime = time;
> +my $buildTime = $endTime - $startTime;
> +
> +my $buildHours = ($buildTime - $buildTime % 3600)/3600;
> +my $buildMins = (($buildTime - $buildHours*3600) - ($buildTime -
$buildHours*3600) %60)/60;
> +my $buildSecs = $buildTime - $buildHours*3600 - $buildMins*60;

This logic would be best in it's own function that takes ($endTime -
$startTime) as input and returns a formatted string.  Maybe call it
formatBuildTime().

I would also leave off hour calculations and only calculate minutes and
seconds.  Most modern hardware builds WebCore in much less than an hour.

Please add spaces before and after all operators (*, %, /, +, -).

> +
> +

There should only be one blank line here.

>  print "\n";
>  print "===========================================================\n";
> -print " WebKit is now built. To run $launcherName with this newly-built\n";
> -print " code, use the \"$launcherPath\" script.\n";
> +print " WebKit is now built ($buildHours:$buildMins:$buildSecs). \n";
> +print " To run $launcherName with this newly-built code, use the\n";
> +print " \"$launcherPath\" script.\n";
>  print "===========================================================\n";

This is fine, but a single $buildTime string from formatBuildTime() would
replace $buildHours:$buildMins:$buildSecs.

r- to fix the above issues.  Thanks!