[webkit-reviews] review denied: [Bug 49503] update-webkit-support-libs should fall back to existing WebKitSupportLibrary version if there is no internet connectivity : [Attachment 73851] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 49503: update-webkit-support-libs should fall back to existing
WebKitSupportLibrary version if there is no internet connectivity
https://bugs.webkit.org/show_bug.cgi?id=49503

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

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

It's great to make this script not rely on the network. I think it might be
clearer if the "were we able to check the expected version" check happened at a
higher level somehow. Maybe just combining your sequential ifs into if/else
would make it clearer, i.e.:

if ($wasSupportLibsURLReachable) {
    ...do one thing...
} else {
    ...do another...
}

> WebKitTools/Scripts/update-webkit-support-libs:51
> +my $supportLibsURL =
"http://developer.apple.com/opensource/internet/$versionFile";

This name seems confusing. It's not the URL of the support libs; it's the URL
of the version file.

> WebKitTools/Scripts/update-webkit-support-libs:60
> +my $wasSupportLibsURLReachable = WEXITSTATUS($?) == 0;

This variable has the same naming issue.

> WebKitTools/Scripts/update-webkit-support-libs:68
> +if ($wasSupportLibsURLReachable && $extractedVersion eq $expectedVersion) {
> +    # Check whether the extracted library is up-to-date. If it is, we don't
have anything to do.

I think this comment should be above the if, not below it.

> WebKitTools/Scripts/update-webkit-support-libs:81
> +if ($wasSupportLibsURLReachable && $zipFileVersion ne $expectedVersion) {
> +    # Check whether the downloaded library is up-to-date. If it isn't, the
user needs to download it.

I think this comment should be above the if, not below it.