[icedtea-web] RFC: allow alternate means of finding browsers
Dr Andrew John Hughes
ahughes at redhat.com
Wed Mar 16 13:28:12 PDT 2011
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"));
What about other platforms? I didn't even realise we were supporting Windows.
We are more likely to find users initially on *BSD systems given there is a BSD port.
> public static void setInitialArgments(List<String> args) {
> checkInitialized();
> SecurityManager securityManager = System.getSecurityManager();
> diff -r 7ddab63cf8fe netx/net/sourceforge/jnlp/services/XBasicService.java
> --- a/netx/net/sourceforge/jnlp/services/XBasicService.java Tue Dec 21 16:48:12 2010 -0500
> +++ b/netx/net/sourceforge/jnlp/services/XBasicService.java Tue Jan 04 12:51:10 2011 -0500
> @@ -31,7 +31,6 @@
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.ApplicationInstance;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> -import net.sourceforge.jnlp.util.PropertiesFile;
>
> /**
> * The BasicService JNLP service.
> @@ -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?
Probably also need some output if the user inputted command fails the if test.
> } 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?
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list