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

[Saad Mohammad]

Hi,

I have attached the patch with all the suggested changes.

A few notes below:

On 06/04/2012 02:49 PM, Omair Majid wrote:
> 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?
>
Yes, implemented this.
>> +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.
>
Yes, it now handles "" and "yes".
>> +   elif test -a "${withval}" ; then
>
> "-a" is AND. Perhaps you meant '-n "${withval}"' ? But even better would
> be "-e" to ensure that ${withval} exists.
>
Thank you for the suggestion. I am now using "-f" to check whether the 
file exists (and not a directory).
>> +     $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


Thanks for the review Omair.

-- 
Cheers,
Saad Mohammad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: configureBrowser06.patch
Type: text/x-patch
Size: 2670 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120605/fd57343a/configureBrowser06.patch