[webkit-reviews] review denied: [Bug 79673] Parametrize run-with-jhbuild and update-webkitgtk-libs with platform --gtk/--efl : [Attachment 131848] Proposal for patching jhbuild related scripts, v4

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Dominik Röttsches
<dominik.rottsches at linux.intel.com>'s request for review:
Bug 79673: Parametrize run-with-jhbuild and update-webkitgtk-libs with platform
--gtk/--efl
https://bugs.webkit.org/show_bug.cgi?id=79673

Attachment 131848: Proposal for patching jhbuild related scripts, v4
https://bugs.webkit.org/attachment.cgi?id=131848&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=131848&action=review


Thanks! This looks good overall but needs another iteration I think. See below:


> Tools/Scripts/update-webkit-libs-jhbuild:41
> +    if ($platformEfl) {
> +	   $platform = "efl";
> +	   $prettyPlatform = "EFL";
> +    }

I'm no Perl monkey (at all), but isn't there a nicer way to handle the option
name with the prettyPlatform? Some kind of mapping/dictionary maybe?

> Tools/jhbuild/jhbuild-wrapper:118
> +    sys.exit("No platform specified for jhbuild-wrapper.")

Please raise a ValueError  here (or an Exception like it's done in the other
functions above).

> Tools/jhbuild/jhbuild-wrapper:133
> +platform = determine_platform()

Wrap this in a try/except and sys.exit() if you catch a ValueError.