[webkit-reviews] review denied: [Bug 51790] [WinCairo] Patch to download the WinCairo dependencies as part of build-webkit. : [Attachment 89265] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 51790: [WinCairo] Patch to download the WinCairo dependencies as part of
build-webkit.
https://bugs.webkit.org/show_bug.cgi?id=51790

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

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=89265&action=review

> Tools/Scripts/update-webkit-auxiliary-libs:38
> +system("perl", "Tools/Scripts/update-webkit-dependency", $auxiliaryLibsURL,
"win") == 0 or die;

Are you sure that the working directory is always the same when this script is
invoked? Using $FindBin::Bin to locate the other script might be safer.

> Tools/Scripts/update-webkit-dependency:150
> +sub getLibraryName

Please give this function a prototype (like lastModifiedToUnixTime has).

> Tools/Scripts/update-webkit-dependency:157
> +  my $path = shift;
> +  my $filename;
> +  my $i = rindex($path, "/");
> +  $filename = substr($path, $i + 1);
> +  $filename = substr($filename, 0, rindex($filename, ".zip"));
> +  return $filename;

Please use 4-space indents.

I think a regular expression might be clearer:

$path =~ m#/([^/])\.zip$#;
return $1;

> Tools/Scripts/update-webkit-wincairo-libs:38
> +system("perl", "Tools/Scripts/update-webkit-dependency", $winCairoLibsURL,
".") == 0 or die;

Same comments here about working directory.