[RFC][icedtea-web]: Configure browser paths

[Omair Majid]

On 06/04/2012 01:40 PM, Saad Mohammad wrote:
> The patch attached allows the user to configure browser paths and/or
> disable browsers from being used in IcedTea-Web.

I like this idea. Some comments on the implementaiton details in-line below.

> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2012-06-04  Saad Mohammad  <smohammad at redhat.com>
> +
> +	Allows the user to configure browser paths and/or disable browsers.
> +	* acinclude.m4 (IT_FIND_BROWSER): Checks if the browser is set be disabled, or
> +	if the path provided is valid. Otherwise, it locates the default path to the browser
> +	if found on the system.
> +	* configure.ac: Uses IT_FIND_BROWSER to find/configure browsers
> +
>  2012-06-04  Adam Domurad  <adomurad at redhat.com>
>  
>  	Added self to AUTHORS.
> diff --git a/acinclude.m4 b/acinclude.m4
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -880,3 +880,29 @@
>    AC_MSG_RESULT([${FULL_VERSION}])
>    AC_SUBST([FULL_VERSION])
>  ])
> +
> +dnl REQUIRED Parameters: 
> +dnl [browser name, variable to store path, default terminal command to run browser (if installed)]

Would it be possible to make the last argument optional? So if the
caller leaves out the terminal command, default to the browser name?

> +AC_DEFUN([IT_FIND_BROWSER],
> +[
> + AC_MSG_CHECKING([for $1])
> +  AC_ARG_WITH([$1],
> +              [AS_HELP_STRING(--with-$1,specify the location of $1 or use 'no' as value to disable browser)],

I think just "specify the location of $1" should be enough. "", "no" and
"yes" are common values that we should accept anyway.

> +  [
> +   if  test "${withval}" = "no" ; then 
> +    $2=""

Please extends to handle "" (--with-firefox) and "yes"
(--with-firefox=yes) too.

> +   elif test -a "${withval}" ; then 

"-a" is AND. Perhaps you meant '-n "${withval}"' ? But even better would
be "-e" to ensure that ${withval} exists.

> +     $2=${withval}

Please be careful with quoting. If the value of withval includes a
space, this will break.

> +   else 
> +    AC_MSG_FAILURE([invalid location specified to $1: ${withval}])
> +   fi
> +  ],

Does this message make sense in the context it's printed?

"checking for firefox... " (output from AC_MSG_FAILURE)

Maybe just a "not found"?

> +  [
> +    $2=$(which $3 2> /dev/null)

This is probably not the best thing to do. "which" is not a posix
standard command, so some machines/shells may not have it at all. It
also breaks with aliases:

$ alias firefox='firefox --sync'
$ FIREFOX="$(which firefox 2>/dev/null)"
$ echo $FIREFOX
alias firefox='firefox --sync' /usr/bin/firefox

> +  ])
> +   if test -a "${$2}"; then

Another "-a" here.

Omair