From: Christian Couder <christian.couder@gmail.com>
Date: Mon, 29 Nov 2010 16:18:47 GMT
Subject: Re: [PATCH 5/6] web--browse: use (x-)www-browser if available
Message-ID: <AANLkTinGustedMv9rm+RgGMQckfxgbTPA+Yqpz7vqTfm@mail.gmail.com>
In-Reply-To: <1291042075-19983-6-git-send-email-giuseppe.bilotta@gmail.com>

Hi,

On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
>
> +# Debian and derivatives use x-www-browser or www-browser to set
> +# the default browser for the system
> +if test -z "$browser" ; then
> +       wwwbrowser="/usr/bin/www-browser"
> +       if test -n "$DISPLAY"; then
> +               wwwbrowser="/usr/bin/x-www-browser $wwwbrowser"
> +       fi
> +       for i in $wwwbrowser; do
> +               if test -x $i ; then
> +                       verstring="$($i --version 2> /dev/null | head -n 1)"
> +                       browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)"

Stupid questions:

Did you check that all the browser we support accept the --version option?
What if we add support for a new one that doesn't ?
Shouldn't we add something like :

test -z "$browser" && browser="$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"

And are you sure that when they support --version, the first word of
the output is
better than "$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"?
What if we add support for a new one?

> +                       case "$browser" in
> +                       mozilla)
> +                               browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
> +                               ;;
> +                       google)
> +                               browser="google-chrome"
> +                               ;;
> +                       esac
> +                       if valid_tool "$browser" ; then

valid_tool "$browser" is called once here...

> +                               browser_path="$i"
> +                               break
> +                       fi
> +               fi
> +       done
> +fi
> +
>  if test -z "$browser" ; then
>        if test -n "$DISPLAY"; then
>                browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo"
> @@ -129,7 +156,7 @@ if test -z "$browser" ; then
>                fi
>        done
>        test -z "$browser" && die "No known browser available."
> -else
> +else if test -z "$browser_path"; then
>        valid_tool "$browser" || die "Unknown browser '$browser'."

...valid_tool "$browser" is called again here if it failed above, and
here we die,
so isn't it clearer to die as soon as we call it and it fails?

Thanks,
Christian.