[rfc][icedtea-web] fix (And tests) for PR1473
Jiri Vanek
jvanek at redhat.com
Mon Jun 24 03:27:32 PDT 2013
On 06/21/2013 05:13 PM, Adam Domurad wrote:
>
>>>
>>> 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!
I would like to backport this to 1.4 also.
>>
>
> 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
Not sneek! Those hunks would need some changes, and are useless so removal was thebest.
>
>>
>> }
>> 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.
all nits except the "fileExists" one fixed. Your eye was sharp as always.
Thanx!
More information about the distro-pkg-dev
mailing list