[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