[webkit-reviews] review granted: [Bug 228276] [GTK][WPE] Organize list of package dependencies into separated files : [Attachment 434263] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has granted Diego Pino <dpino at igalia.com>'s
request for review:
Bug 228276: [GTK][WPE] Organize list of package dependencies into separated
files
https://bugs.webkit.org/show_bug.cgi?id=228276

Attachment 434263: Patch

https://bugs.webkit.org/attachment.cgi?id=434263&action=review




--- Comment #8 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 434263
  --> https://bugs.webkit.org/attachment.cgi?id=434263
Patch

Much nicer, thanks for updating the patch! I have left a couple of comments
more
in case you feel like applying them before landing =)


View in context: https://bugs.webkit.org/attachment.cgi?id=434263&action=review

> Tools/wpe/install-dependencies:43
> +    source $(dirname "$0")/dependencies/$packageManager

Missing quoting around the path passed to source:

  source "$(dirname "$0")/dependencies/$packageManager"

> Tools/wpe/install-dependencies:45
> +    case "$packageManager" in

...and then this “case” statement won't be needed, you can replace it with:

   "installDependenciesWith${packageManager^}"

which will expand to installDependenciesWithApt, installDependenciesWithDnf,
and so on. The caret in the ${foo^} expansion converts the first letter of
the value to uppercase =)

> Tools/wpe/install-dependencies:59
> +    apt-get install -y --no-install-recommends ${PACKAGES[@]}

Typically I would point out that there is quoting around the variable
expansion:

  apt-get install -y --no-install-recommends "${PACKAGES[@]}"

but in this case it does not matter much because we know that package names
never
contain spaces or other characters that would be interpreted in some special
way
by the shell. Feel free to add the quoting before landing, but as said, it's
not
strictly needed here :)