[icedtea-web] RFC: allow alternate means of finding browsers
Omair Majid
omajid at redhat.com
Thu Mar 17 09:14:11 PDT 2011
On 03/16/2011 04:28 PM, Dr Andrew John Hughes wrote:
> On 12:54 Tue 04 Jan , Omair Majid wrote:
>> On 12/22/2010 06:29 PM, Dr Andrew John Hughes wrote:
>>> On 17:15 Wed 22 Dec , Omair Majid wrote:
>>>> On 12/22/2010 03:42 PM, Dr Andrew John Hughes wrote:
>>>>> On 15:32 Wed 22 Dec , Omair Majid wrote:
>>>>>> Hi,
>>>>>>
>>>>>> The attached patch allows netx to use a browser based on the BROWSER env
>>>>>> variable or using xdg-open, as opposed to relying on user-supplied
>>>>>> browser command.
>>>>>>
>>>>>> A new configuration option deployment.browser.source is used to
>>>>>> determine where to find the browser. If it is set to "PATH" (the
>>>>>> default), the configuration option deployment.browser.path is used. If
>>>>>> deployment.browser.source is set to "ENV" then the $BROWSER environment
>>>>>> variable is used. If deployment.browser.source is set to "XDG" then the
>>>>>> program xdg-open is used to launch the browser.
>>>>>>
>>>>>
>>>>> I would make XDG the default so it works like other applications on the system
>>>>> (which I presume use either this or $BROWSER) if deployment.browser.path is not set.
>>>>> Then NetX automatically picks up the system browser rather than prompting.
>>>>>
>>>>
>>>> Ah, great idea! Should I keep support for using $BROWSER or remove that
>>>> too in favour of xdg-open?
>>>>
>>>
>>> Please keep it. I don't see how it does any harm.
>>>
>>> You might also want to consider the scenario that xdg-open is not available
>>> and fall back to $BROWSER then prompting if that is also empty.
>>>
>>
>> Done.
>>
>>>>> Shouldn't we do some basic sanity checks on the contents of getEnv? (And path too
>>>>> if we don't already)
>>>>>
>>>>
>>>> We should, and we dont do it for path either. Actually netx refers to
>>>> path as 'command' and that's how it deals with it too - it simply execs
>>>> "command url" to open the url. This allows a user to use a program name
>>>> (like firefox) instead of the entire path to it. So checking for
>>>> file/path existence makes this common case fail:
>>>> BROWSER=chromium-browser javaws foo.jnlp
>>>> (and similar for path). I dont see any simple way to check if the
>>>> command is valid - short of actually running the command. Do you have
>>>> any other sanity checks in mind?
>>>
>>> On *ix systems, you could run 'which command' and then execute the result of that
>>> instead. How well that works depends if we want to allow options to be specified
>>> to the binary.
>>>
>>
>> I am using the 'type' builtin to check if command is valid rather than
>> 'which' since the behaviour of 'type' is a POSIX standard, while
>> 'which' may or may not return an error code on failure.
>>
>>> I would at least check for things like \r and \n which would suggest something fishy to me.
>>> It doesn't immediately seem a huge issue as the netx binary is not privileged.
>>>
>>
>> Also done.
>>
>> Cheers,
>> Omair
>
>> diff -r 7ddab63cf8fe netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Tue Dec 21 16:48:12 2010 -0500
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Tue Jan 04 12:51:10 2011 -0500
>> @@ -608,6 +608,24 @@
>> }
>> }
>>
>> + public static boolean isWindows() {
>> + String os = System.getProperty("os.name");
>> + if (os != null&& os.startsWith("Windows")) {
>> + return true;
>> + } else {
>> + return false;
>> + }
>> + }
>> +
>> + public static boolean isLinux() {
>> + String os = System.getProperty("os.name");
>> + if (os != null&& os.startsWith("Linux")) {
>> + return true;
>> + } else {
>> + return false;
>> + }
>> + }
>> +
>
> These could be simply:
>
> return (os != null&& os.startsWith("Linux"));
>
Fixed.
> What about other platforms? I didn't even realise we were supporting Windows.
We dont - I am quite sure a number of assumptions made by icedtea-web
become invalid when run under Windows. This piece of code already
existed in XBasicService; I just moved it to a more prominent/sensible
location.
> We are more likely to find users initially on *BSD systems given there is a BSD port.
>
Good point. I have renamed the isLinux method to isUnix. It looks at the
separator character to determine if it is running on a Unix or Unix-like
system. Everything done in the Linux case should work for other
Unix/Posix systems as well.
>> @@ -191,9 +190,47 @@
>> if (initialized)
>> return;
>> initialized = true;
>> + initializeBrowserCommand();
>> + if (JNLPRuntime.isDebug()) {
>> + System.out.println("browser is " + command);
>> + }
>> + }
>>
>> - if (isWindows()) {
>> + /**
>> + * Initializes {@link #command} to launch a browser
>> + */
>> + private void initializeBrowserCommand() {
>> + if (JNLPRuntime.isWindows()) {
>> command = "rundll32 url.dll,FileProtocolHandler ";
>
> Wow, that's hideous.
>
>> + } else if (JNLPRuntime.isLinux()) {
>> + DeploymentConfiguration config = JNLPRuntime.getConfiguration();
>> + command = config.getProperty(DeploymentConfiguration.KEY_BROWSER_PATH);
>> + if (command != null) {
>> + return;
>> + }
>> +
>> + if (posixCommandExists("xdg-open")) {
>> + command = "xdg-open";
>> + return;
>> + }
>> +
>> + if (posixCommandExists(System.getenv("BROWSER"))) {
>> + command = System.getenv("BROWSER");
>> + return;
>> + }
>> +
>> + while (true) {
>> + command = promptForCommand(null);
>> + if (command != null&& posixCommandExists(command)) {
>> + config.setProperty(DeploymentConfiguration.KEY_BROWSER_PATH, command);
>> + try {
>> + config.save();
>> + } catch (IOException e) {
>> + e.printStackTrace();
>> + }
>> + break;
>> + }
>> + }
>
> Maybe it's worth having some debug output in this bit to say what was picked?
Already done. Since initializeBrowserCommand has a number of return
points, I added the debug output in the caller. See the statement
System.out.println("browser is " + command) above.
> Probably also need some output if the user inputted command fails the if test.
>
Good point. Fixed
>> } else {
>> DeploymentConfiguration config = JNLPRuntime.getConfiguration();
>> command = config.getProperty(DeploymentConfiguration.KEY_BROWSER_PATH);
>> @@ -213,12 +250,31 @@
>> }
>> }
>>
>> - private boolean isWindows() {
>> - String os = System.getProperty("os.name");
>> - if (os != null&& os.startsWith("Windows"))
>> - return true;
>> - else
>> + /**
>> + * Check that a command exists on a posix-like system
>> + * @param command the command to check
>> + * @return true if the command exists
>> + */
>> + private boolean posixCommandExists(String command) {
>> + if (command == null || command.trim().length() == 0) {
>> return false;
>> + }
>> +
>> + command = command.trim();
>> + if (command.contains("\n") || command.contains("\r")) {
>> + return false;
>> + }
>> +
>> + try {
>> + Process p = Runtime.getRuntime().exec(new String[] { "bash", "-c", "type " + command });
>> + p.waitFor();
>> + return (p.exitValue() == 0);
>> + } catch (IOException e) {
>> + e.printStackTrace();
>> + return false;
>> + } catch (InterruptedException e) {
>> + return false;
>> + }
>> }
>>
>> private String promptForCommand(String cmd) {
>
> Why don't we print the stack trace if interrupted?
>
Done. Please see the attached patch.
Okay to commit?
Cheers,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: configure-browser-04.patch
Type: text/x-patch
Size: 5944 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110317/fc9387f0/configure-browser-04.patch
More information about the distro-pkg-dev
mailing list