[rfc][icedtea-web] fix (And tests) for PR1473

Adam Domurad adomurad at redhat.com
Fri Jun 21 08:13:42 PDT 2013


>>
>> OK we discussed this more on IRC and I think we're agreed:
>> - Don't redownload if no href (already the case)
>> - Don't redownload if not local file (needs to be added)
>>
>> The case where you save the JNLP and immediately launch it
>> unfortunately does redownload, we can
>> however do some timestamp check if you really do not like this.
>> Other than that ideally the caching system would be allow for a simple
>> HEAD request to determine if
>> the JNLP needs to be updated, instead of a full redownload.
>
>
> ook, try this:)

Thanks for the update!

>>
>> Cheers,
>> -Adam
>

Patch:

> diff -r a236aa5f729b netx/net/sourceforge/jnlp/Launcher.java
> --- a/netx/net/sourceforge/jnlp/Launcher.java	Fri Jun 21 12:15:03 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/Launcher.java	Fri Jun 21 16:13:37 2013 +0200
> @@ -260,30 +260,18 @@
>          return tg.getApplication();
>      }
>
> -    /**
> -     * Launches a JNLP file by calling the launch method for the
> -     * appropriate file type.
> -     *
> -     * @param location the URL of the JNLP file to launch
> -     * @throws LaunchException if there was an exception
> -     * @return the application instance
> -     */
> -    public ApplicationInstance launch(URL location) throws LaunchException {
> -        return launch(toFile(location));
> -    }
>
>      /**
>       * Launches a JNLP file by calling the launch method for the
>       * appropriate file type.
>       *
>       * @param location the URL of the JNLP file to launch
> -     * @param fromSource if true, the JNLP file will be re-read from the source
>       * location to get the pristine version
>       * @throws LaunchException if there was an exception
>       * @return the application instance
>       */
> -    public ApplicationInstance launch(URL location, boolean fromSource) throws LaunchException {
> -        return launch(fromUrl(location, fromSource));
> +    public ApplicationInstance launch(URL location) throws LaunchException {
> +        return launch(fromUrl(location));
>      }
>
>      /**
> @@ -372,28 +360,7 @@
>          }
>      }
>
> -    /**
> -     * Launches a JNLP file by calling the launch method for the
> -     * appropriate file type in a different thread.
> -     *
> -     * @param file the JNLP file to launch
> -     */
> -    public void launchBackground(JNLPFile file) {
> -        BgRunner runner = new BgRunner(file, null);
> -        new Thread(runner).start();
> -    }
> -
> -    /**
> -     * Launches the JNLP file at the specified location in the
> -     * background by calling the launch method for its file type.
> -     *
> -     * @param location the location of the JNLP file
> -     */
> -    public void launchBackground(URL location) {
> -        BgRunner runner = new BgRunner(null, location);
> -        new Thread(runner).start();
> -    }
> -
> +
>      /**
>       * Launches the JNLP file in a new JVM instance.  The launched
>       * application's output is sent to the system out and it's
> @@ -473,62 +440,36 @@
>      /**
>       * Returns the JNLPFile for the URL, with error handling.
>       */
> -    private JNLPFile fromUrl(URL location, boolean fromSource) throws LaunchException {
> +    private JNLPFile fromUrl(URL location) throws LaunchException {
>          try {
>              JNLPFile file = null;
>
>              file = new JNLPFile(location, parserSettings);
> +
> +            boolean isLocal = false;
> +            boolean haveHref = false;
> +            if ("file".equalsIgnoreCase(location.getProtocol()) && new File(location.getFile()).exists()) {

Hm, why does it have to exist to be local (current logic == only 
redownload if it exists) ? Anyway, its probably harmless either way

> +                isLocal = true;
> +            }
> +            if (file.getSourceLocation() != null) {
> +                haveHref = true;
> +            }
>
> -            if (fromSource) {
> -                // Launches the jnlp file where this file originated.
> -                if (file.getSourceLocation() != null) {
> -                    file = new JNLPFile(file.getSourceLocation(), parserSettings);
> -                }
> +            if (isLocal && haveHref) {
> +                file = new JNLPFile(file.getSourceLocation(), parserSettings);
>              }
>              return file;
>          } catch (Exception ex) {
> -            if (ex instanceof LaunchException)
> +            if (ex instanceof LaunchException) {
>                  throw (LaunchException) ex; // already sent to handler when first thrown
> -            else
> +            } else {
>                  // IO and Parse
>                  throw launchError(new LaunchException(null, ex, R("LSFatal"), R("LCReadError"), R("LCantRead"), R("LCantReadInfo")));
> +            }
>          }
>      }
>
> -    /**
> -     * Returns the JNLPFile for the URL, with error handling.
> -     */
> -    @Deprecated
> -    private JNLPFile toFile(URL location) throws LaunchException {
> -        try {
> -            JNLPFile file = null;
> -
> -            try {
> -                ParserSettings settings = new ParserSettings(true, true, false);
> -                file = new JNLPFile(location, (Version) null, settings, updatePolicy); // strict
> -            } catch (ParseException ex) {
> -                ParserSettings settings = new ParserSettings(false, true, true);
> -                file = new JNLPFile(location, (Version) null, settings, updatePolicy);
> -
> -                // only here if strict failed but lax did not fail
> -                LaunchException lex =
> -                        launchWarning(new LaunchException(file, ex, R("LSMinor"), R("LCFileFormat"), R("LNotToSpec"), R("LNotToSpecInfo")));
> -
> -                if (lex != null)
> -                    throw lex;
> -            }
> -
> -            return file;
> -        } catch (Exception ex) {
> -            if (ex instanceof LaunchException)
> -                throw (LaunchException) ex; // already sent to handler when first thrown
> -            else
> -                // IO and Parse
> -                throw launchError(new LaunchException(null, ex, R("LSFatal"), R("LCReadError"), R("LCantRead"), R("LCantReadInfo")));
> -        }
> -    }
> -
> -    /**
> +   /**
>       * Launches a JNLP application.  This method should be called
>       * from a thread in the application's thread group.
>       */
> @@ -973,31 +914,6 @@
>
>      };
>
> -    /**
> -     * This runnable is used by the <code>launchBackground</code>
> -     * methods to launch a JNLP file from a separate thread.
> -     */
> -    private class BgRunner implements Runnable {
> -        private JNLPFile file;
> -        private URL location;
> -
> -        BgRunner(JNLPFile file, URL location) {
> -            this.file = file;
> -            this.location = location;
> -        }
> -
> -        public void run() {
> -            try {
> -                if (file != null)
> -                    launch(file);
> -                if (location != null)
> -                    launch(location);
> -            } catch (LaunchException ex) {
> -                // launch method communicates error conditions to the
> -                // handler if it exists, otherwise we don't care because
> -                // there's nothing that can be done about the exception.
> -            }
> -        }
> -    };
> +

I approve this sneak-in

>
>  }
> diff -r a236aa5f729b netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java	Fri Jun 21 12:15:03 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java	Fri Jun 21 16:13:37 2013 +0200
> @@ -215,7 +215,7 @@
>              Launcher launcher = new Launcher(false);
>              launcher.setParserSettings(settings);
>              launcher.setInformationToMerge(extra);
> -            launcher.launch(getFileLocation(), true);
> +            launcher.launch(getFileLocation());
>          } catch (LaunchException ex) {
>              // default handler prints this
>          } catch (Exception ex) {
> diff -r a236aa5f729b tests/reproducers/simple/GeneratedId/resources/GeneratedId.jnlp
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/reproducers/simple/GeneratedId/resources/GeneratedId.jnlp	Fri Jun 21 16:13:37 2013 +0200
> @@ -0,0 +1,51 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +
> +<!--
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2, or (at your option)
> +any later version.
> +
> +IcedTea is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING.  If not, write to the
> +Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library.  Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module.  An independent module is a module which is not derived from
> +or based on this library.  If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so.  If you do not wish to do so, delete this
> +exception statement from your version.
> + -->
> +
> +<jnlp spec="1.0" codebase="." href="GeneratedId.jnlp">
> +   <information>
> +      <title>Test Generated Id</title>
> +      <vendor>IcedTea</vendor>
> +   </information>
> +   <resources>
> +      <jar href="GeneratedId.jar" main="true"/>
> +   </resources>
> +   <application-desc main-class="GeneratedId">
> +      <argument>SomeId</argument>
> +   </application-desc>
> +</jnlp>
> diff -r a236aa5f729b tests/reproducers/simple/GeneratedId/srcs/GeneratedId.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/reproducers/simple/GeneratedId/srcs/GeneratedId.java	Fri Jun 21 16:13:37 2013 +0200
> @@ -0,0 +1,44 @@
> +/* Copyright (C) 2012 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING.  If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library.  Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module.  An independent module is a module which is not derived from
> +or based on this library.  If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so.  If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +
> +
> +public class GeneratedId {
> +    static public void main(String[] args) {
> +        for(int x = 0; x<args.length; x++){
> +            System.out.println(x+" - id: "+args[x]);
> +        }
> +    }
> +}
> diff -r a236aa5f729b tests/reproducers/simple/GeneratedId/testcases/GeneratedIdTest.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/reproducers/simple/GeneratedId/testcases/GeneratedIdTest.java	Fri Jun 21 16:13:37 2013 +0200
> @@ -0,0 +1,183 @@
> +/* Copyright (C) 2012 Red Hat, Inc.
> +
> + This file is part of IcedTea.
> +
> + IcedTea is free software; you can redistribute it and/or
> + modify it under the terms of the GNU General Public License as published by
> + the Free Software Foundation, version 2.
> +
> + IcedTea is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with IcedTea; see the file COPYING.  If not, write to
> + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> + 02110-1301 USA.
> +
> + Linking this library statically or dynamically with other modules is
> + making a combined work based on this library.  Thus, the terms and
> + conditions of the GNU General Public License cover the whole
> + combination.
> +
> + As a special exception, the copyright holders of this library give you
> + permission to link this library with independent modules to produce an
> + executable, regardless of the license terms of these independent
> + modules, and to copy and distribute the resulting executable under
> + terms of your choice, provided that you also meet, for each linked
> + independent module, the terms and conditions of the license of that
> + module.  An independent module is a module which is not derived from
> + or based on this library.  If you modify this library, you may extend
> + this exception to your version of the library, but you are not
> + obligated to do so.  If you do not wish to do so, delete this
> + exception statement from your version.
> + */
> +
> +import java.io.File;
> +import java.io.FileInputStream;
> +import java.io.IOException;
> +import java.util.ArrayList;
> +import java.util.List;
> +import junit.framework.Assert;
> +import net.sourceforge.jnlp.ProcessResult;
> +import net.sourceforge.jnlp.ServerAccess;
> +import org.junit.Test;
> +
> +public class GeneratedIdTest {
> +
> +    private static final ServerAccess server = new ServerAccess();
> +    private static final String okBase = "0 - id: ";
> +    private static final String someId1 = "SomeId";
> +    private static final String someId2 = "AnotherId";
> +    private static final String okBase1 = okBase + someId1;
> +    private static final String okBase2 = okBase + someId2;
> +    private static final String baseName1 = "GeneratedId.jnlp";
> +    private static final String baseName1_noHref = "GeneratedIdNoHref.jnlp";
> +    private static final String baseName2 = "GeneratedId_1_tmp.jnlp";
> +    private static final String baseName2_noHref = "GeneratedIdNoHref_1_tmp.jnlp";
> +
> +    public static File prepareChangedFileWithHref() throws IOException {
> +        File src = new File(server.getDir(), baseName1);
> +        File dest = new File(server.getDir(), baseName2);
> +        String srcJnlp = ServerAccess.getContentOfStream(new FileInputStream(src));
> +        ServerAccess.saveFile(srcJnlp.replace(someId1, someId2), dest);
> +        return dest;
> +    }
> +
> +    public static File prepareChangedFileNoHref() throws IOException {
> +        File src = new File(server.getDir(), baseName1);
> +        File dest = new File(server.getDir(), baseName2_noHref);
> +        String srcJnlp = ServerAccess.getContentOfStream(new FileInputStream(src));
> +        ServerAccess.saveFile(srcJnlp.replace(someId1, someId2).replace("href=\"GeneratedId.jnlp\"", ""), dest);
> +        return dest;
> +    }
> +
> +    public static File prepareCopiedFileNoHref() throws IOException {
> +        File src = new File(server.getDir(), baseName1);
> +        File dest = new File(server.getDir(), baseName1_noHref);
> +        String srcJnlp = ServerAccess.getContentOfStream(new FileInputStream(src));
> +        ServerAccess.saveFile(srcJnlp.replace("href=\"GeneratedId.jnlp\"", ""), dest);
> +        return dest;
> +    }
> +
> +    @Test
> +    //have href
> +    //is local
> +    //should be redownlaoded

s/redownlaoded/redownloaded/ (for each comment) :-)

> +    //href points to different file
> +    public void launchLocalChangedFileWithHref() throws Exception {
> +        File dest = prepareChangedFileWithHref();
> +        List<String> l = new ArrayList<String>(3);
> +        l.add(server.getJavawsLocation());
> +        l.add(ServerAccess.HEADLES_OPTION);
> +        l.add(dest.getAbsolutePath());
> +        ProcessResult pr = ServerAccess.executeProcess(l);
> +        Assert.assertTrue("Stdout should contain '" + okBase1 + "', but did not.", pr.stdout.contains(okBase1));
> +    }
> +
> +    @Test
> +    //do not have href
> +    //is local
> +    //should NOT be redownlaoded
> +    public void launchLocalChangedFileWithNoHref() throws Exception {
> +        File dest = prepareChangedFileNoHref();
> +        List<String> l = new ArrayList<String>(3);
> +        l.add(server.getJavawsLocation());
> +        l.add(ServerAccess.HEADLES_OPTION);
> +        l.add(dest.getAbsolutePath());
> +        ProcessResult pr = ServerAccess.executeProcess(l);
> +        Assert.assertTrue("Stdout should contain '" + okBase2 + "', but did not.", pr.stdout.contains(okBase2));
> +    }
> +
> +    @Test
> +    //do  have href

[nit] space

> +    //is local
> +    //should be redownlaoded (how to verify!?!)
> +    public void launchLocalFileWithHref() throws Exception {
> +        File dest = new File(server.getDir(), baseName1);
> +        List<String> l = new ArrayList<String>(3);
> +        l.add(server.getJavawsLocation());
> +        l.add(ServerAccess.HEADLES_OPTION);
> +        l.add(dest.getAbsolutePath());
> +        ProcessResult pr = ServerAccess.executeProcess(l);
> +        Assert.assertTrue("Stdout should contain '" + okBase1 + "', but did not.", pr.stdout.contains(okBase1));
> +    }
> +
> +    @Test
> +    //do  have href

Do NOT here. :-)

> +    //is local
> +    //should NOT be redownlaoded (how to verify!?!)

[nit] IMO unnecessary comment because there is no need to verify this. 
No href+local file -> no place to possibly redownload from.

> +    public void launchLocalFileNoHref() throws Exception {
> +        File dest = prepareCopiedFileNoHref();
> +        List<String> l = new ArrayList<String>(3);
> +        l.add(server.getJavawsLocation());
> +        l.add(ServerAccess.HEADLES_OPTION);
> +        l.add(dest.getAbsolutePath());
> +        ProcessResult pr = ServerAccess.executeProcess(l);
> +        Assert.assertTrue("Stdout should contain '" + okBase1 + "', but did not.", pr.stdout.contains(okBase1));
> +    }
> +
> +    @Test
> +    //remote
> +    //have href
> +    //should not be redownlaoded (how to verify!?!)

Although this one is trickier :-)

> +    //href is same file
> +    public void launchRemoteFileWithHref() throws Exception {
> +        ProcessResult pr = server.executeJavawsHeadless("/" + baseName1);
> +        Assert.assertTrue("Stdout should contain '" + okBase1 + "', but did not.", pr.stdout.contains(okBase1));
> +    }
> +
> +    //remote
> +    //have href
> +    //should NOT be redownlaoded
> +    //href is different file
> +    @Test
> +    public void launchRemoteChangedFileWithHref() throws Exception {
> +        File f = prepareChangedFileWithHref();
> +        ProcessResult pr = server.executeJavawsHeadless("/" + f.getName());
> +        Assert.assertTrue("Stdout should contain '" + okBase2 + "', but did not.", pr.stdout.contains(okBase2));
> +    }
> +
> +    @Test
> +    //remote
> +    //have not href
> +    //should not be redownlaoded (how to verify!?!)
> +    //href is same file

It has no href but 'href is same file' ??

> +    public void launchRemoteFileWithNoHref() throws Exception {
> +        File f = prepareCopiedFileNoHref();
> +        ProcessResult pr = server.executeJavawsHeadless("/" + f.getName());
> +        Assert.assertTrue("Stdout should contain '" + okBase1 + "', but did not.", pr.stdout.contains(okBase1));
> +    }
> +
> +    //remote
> +    //have not href
> +    //should NOT be redownlaoded
> +    //href is different file

It has no href but 'href is different file' ??

> +    @Test
> +    public void launchRemoteChangedFileWithNoHref() throws Exception {
> +        File f = prepareChangedFileNoHref();
> +        ProcessResult pr = server.executeJavawsHeadless("/" + f.getName());
> +        Assert.assertTrue("Stdout should contain '" + okBase2 + "', but did not.", pr.stdout.contains(okBase2));
> +    }
> +}


Thank you, mostly nits (beyond the typo). Address nits however you 
choose and push.

Happy hacking,
-Adam



More information about the distro-pkg-dev mailing list