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

> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> 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.
>
>>> +                       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.