Re: [PATCH 5/6] web--browse: use (x-)www-browser if available

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Nov 29, 2010, 19:05 UTC
Message-ID: <AANLkTimec0prJ29MR3N7GtHJ1FCkk+7=1UJtqiaLZqhK@mail.gmail.com>
In-Reply-To: <AANLkTinGustedMv9rm+RgGMQckfxgbTPA+Yqpz7vqTfm@mail.gmail.com>

On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 20 quoted lines

> 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?

I don't have all of them readily available, so I checked with Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm missing konqueror and dillo.

> What if we add support for a new one that doesn't ?

I think that the worse issue would be (x-)www-browser linking to something that doesn't support --version regardless of wether we support it or not.

> Shouldn't we add something like :
>
> test -z "$browser" && browser="$(readlink $i)"

My first idea was to go for something like browser="$(basename $(readlink $i))" (not sure why you would need test -z before). Since it would need special-casing anyway (e.g. chromium-browser -> chromium), I opted out for the --version way so that (1) we could catch one of our friendly cases even if the binary was called something else and (2) some invocation paths try to get the version anyway, so we could do it once and for all.

> And are you sure that when they support --version, the first word of
> the output is
> better than "$(readlink $i)"?

Probably depends on the metric used for 'better' ;-)

> What if we add support for a new one?

Then when adding it we should also look at its --version output and see if it needs special treatment.

Show 19 quoted lines

>> +                       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...
>
>> -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?

We allow valid_tool to be false in the x-www-browser case, in which case we test www-browser, and if it's still not valid we go on and use the previous paths. So we cannot die in case of an invalid (x-)www-browser. But there's a bug in the www-brower testing, it needs an else that resets browser to the empty string.

-- 
Giuseppe "Oblomov" Bilotta