[RFC][icedtea-web]: PR658 - jnlp.{pack,version}Enabled
Omair Majid
omajid at redhat.com
Fri Mar 4 13:03:01 PST 2011
On 03/04/2011 01:44 PM, Denis Lila wrote:
> Hi.
>
> This is supposed to fix the packEnabled issue.
>
Is it possible to split this into two patches? It would be nice to fix
the bugs in the current system in a separate patch than one which adds
plugin support for packEnabled.
> ChangeLog (with explanations of changes).
>
> + * netx/net/sourceforge/jnlp/PluginBridge.java
> + (usePack, useVersion, usePack(), useVersion()): added.
> + (PluginBridge): initializing usePack, useVersion.
>
> These methods are called from getDownloadOptionsForJar to determine
> whether pack/versionEnabled is on.
>
> + Also added import java.util.Arrays.
> + * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> + (downloadResource): changed the order in which pack200+gz compression
> + and gzip compression are checked.
>
> This fixes a problem where the http connection object told us that
> the stream was "gzip" encoded. A file could have been compressed
> with both pack200 and gzip, so a return value of "gzip" from
> con.getContentEncoding() is still correct, but in these cases
> we assumed incorrectly that it was_only_ gzip encoded, so we
> ended up trying to use a pack200 file as a jar file.
>
Makes sense. Good catch!
> + * netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
> + (getUrl): if usePack now calling concat instead of replace.
>
> Thishttp://download.oracle.com/javase/6/docs/technotes/guides/jweb/tools/pack200.html#pack200
> says that when using packEnabled X.jar should be named X.jar.pack.gz.
> We were computing X.pack.gz.
>
Doh. I wonder how I made that mistake. Thanks for fixing this!
> + * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> + (getDownloadOptionsForJar): added special case for PluginBridge, using
> + better jar comparison.
>
> Two changes here:
> 1. this explicitly tests whether the file is a
> PluginBridge. The call to desc.getPropertiesMap() would always
> return an empty Map when file is a PluginBridge. I tried changing
> the implementation of PluginBridge.getResources so that
> getResources(Class<T> launchType) would have a case for
> PropertyDesc.class, but that caused problems later, when we
> called System.setProperty on the returned properties.
>
Ugh. I really dont like the explicit checks for PluginBridge. I suppose
that's ok for now, but when we start fixing applets deployed through
JNLPs things will become much more interesting.
> 2. I replaced "jar==aJar" with a URL.equals() comparison. Without
> this, the condition would always evaluate to false when file is
> a PluginBridge, because of this line in PluginBridge.java:
> jarDescs.add(new JARDesc(new URL(codeBase, jars[i]),
> null, null, false, true, false, true));
>
> It creates new JARDesc's, so == fails.
> Should I use CacheUtil.urlEquals here, instead of URL.equals?
>
Using CacheUtil.urlEquals might be a better idea; URL.equals has some
interesting behaviour in terms of speed and connectivity - it performs
dns lookups [1].
> I tested this with two applets that use packEnabled:
> My toy test applet (it's working if a diagonal line is drawn):
> http://icedtea.classpath.org/~dlila/tmp/
>
> What started this all:
> http://www.jroller.com/dgilbert/entry/jfreechart_and_jxlayer
>
> They both load successfully, without any exceptions thrown.
> For me, the second applet is horribly slow with icedtea-web.
> This is probably because I was using a debug build of icedtea6,
> but I thought I should mention it. If someone could test it
> with a product build, that would be great.
>
> I would appreciate any feedback.
>
Some code comments are inline below.
> Thank you,
> Denis.
>
>
> packEnabled.patch
>
>
> diff -r 64b9d3a8239c ChangeLog
> --- a/ChangeLog Thu Mar 03 17:56:00 2011 -0500
> +++ b/ChangeLog Fri Mar 04 13:35:15 2011 -0500
> @@ -1,3 +1,18 @@
> +2011-03-04 Denis Lila<dlila at redhat.com>
> +
> + * netx/net/sourceforge/jnlp/PluginBridge.java
> + (usePack, useVersion, usePack(), useVersion()): added.
> + (PluginBridge): initializing usePack, useVersion.
> + Also added import java.util.Arrays.
> + * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> + (downloadResource): changed the order in which pack200+gz compression
> + and gzip compression are checked.
> + * netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
> + (getUrl): if usePack now calling concat instead of replace.
> + * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> + (getDownloadOptionsForJar): added special case for PluginBridge, using
> + better jar comparison.
> +
> 2011-03-03 Deepak Bhole<dbhole at redhat.com>
>
> * plugin/icedteanp/IcedTeaNPPlugin.cc
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/PluginBridge.java
> --- a/netx/net/sourceforge/jnlp/PluginBridge.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java Fri Mar 04 13:35:15 2011 -0500
> @@ -24,6 +24,7 @@
>
> import java.net.URL;
> import java.net.MalformedURLException;
> +import java.util.Arrays;
> import java.util.Calendar;
> import java.util.Hashtable;
> import java.util.Locale;
> @@ -40,6 +41,8 @@
> String[] cacheJars = new String[0];
> String[] cacheExJars = new String[0];
> Hashtable<String, String> atts;
> + private boolean usePack;
> + private boolean useVersion;
>
> public PluginBridge(URL codebase, URL documentBase, String jar, String main,
> int width, int height, Hashtable<String, String> atts)
> @@ -133,6 +136,27 @@
> this.uniqueKey = Calendar.getInstance().getTimeInMillis() + "-" +
> Math.abs(((new java.util.Random()).nextInt())) + "-" +
> documentBase;
> +
> + String jargs = atts.get("java_arguments");
> + if (jargs != null) {
> + List<String> args = Arrays.asList(jargs.split(" "));
> + for (int i = 0; i< args.size(); i++) {
> + args.set(i, args.get(i).trim());
> + }
> + usePack = args.contains("-Djnlp.packEnabled=true");
> + useVersion = args.contains("-Djnlp.versionEnabled=true");
Perhaps doing a boolean.valueOf on the value might be more appropriate:
if a user does -Djnlp.packEnabled=True we dont want this to fail.
> + } else {
> + usePack = false;
> + useVersion = false;
> + }
> + }
> +
> + public boolean usePack() {
> + return usePack;
> + }
> +
> + public boolean useVersion() {
> + return useVersion;
> }
>
> public String getTitle() {
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Fri Mar 04 13:35:15 2011 -0500
> @@ -653,11 +653,14 @@
>
> }
>
> - if ("gzip".equals(contentEncoding)) {
> + boolean packgz = "pack200-gzip".equals(contentEncoding) ||
> + realLocation.getPath().endsWith(".pack.gz");
> + boolean gzip = "gzip".equals(contentEncoding);
> +
> + if (packgz) {
> + downloadLocation = new URL(downloadLocation.toString() + ".pack.gz");
> + } else if (gzip) {
> downloadLocation = new URL(downloadLocation.toString() + ".gz");
> - } else if ("pack200-gzip".equals(contentEncoding) ||
> - realLocation.getPath().endsWith(".pack.gz")) {
> - downloadLocation = new URL(downloadLocation.toString() + ".pack.gz");
> }
>
> InputStream in = new BufferedInputStream(con.getInputStream());
> @@ -681,7 +684,21 @@
> * If the file was compressed, uncompress it.
> */
>
> - if ("gzip".equals(contentEncoding)) {
> + if (packgz) {
> + GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(
> + CacheUtil.getCacheFile(downloadLocation, resource.downloadVersion)));
> + InputStream inputStream = new BufferedInputStream(gzInputStream);
> +
> + JarOutputStream outputStream = new JarOutputStream(new FileOutputStream(
> + CacheUtil.getCacheFile(resource.location, resource.downloadVersion)));
> +
> + Unpacker unpacker = Pack200.newUnpacker();
> + unpacker.unpack(inputStream, outputStream);
> +
> + outputStream.close();
> + inputStream.close();
> + gzInputStream.close();
> + } else if (gzip) {
> GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(CacheUtil
> .getCacheFile(downloadLocation, resource.downloadVersion)));
> InputStream inputStream = new BufferedInputStream(gzInputStream);
> @@ -697,25 +714,8 @@
> outputStream.close();
> inputStream.close();
> gzInputStream.close();
> -
> - } else if ("pack200-gzip".equals(contentEncoding) ||
> - realLocation.getPath().endsWith(".pack.gz")) {
> - GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(
> - CacheUtil.getCacheFile(downloadLocation, resource.downloadVersion)));
> - InputStream inputStream = new BufferedInputStream(gzInputStream);
> -
> - JarOutputStream outputStream = new JarOutputStream(new FileOutputStream(
> - CacheUtil.getCacheFile(resource.location, resource.downloadVersion)));
> -
> - Unpacker unpacker = Pack200.newUnpacker();
> - unpacker.unpack(inputStream, outputStream);
> -
> - outputStream.close();
> - inputStream.close();
> - gzInputStream.close();
> }
>
> -
> resource.changeStatus(DOWNLOADING, DOWNLOADED);
> synchronized (lock) {
> lock.notifyAll(); // wake up wait's to check for completion
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java Fri Mar 04 13:35:15 2011 -0500
> @@ -122,7 +122,7 @@
> filename = name + "__V" + resource.requestVersion + "." + extension;
> }
> if (usePack) {
> - filename = filename.replace(".jar", ".pack.gz");
> + filename = filename.concat(".pack.gz");
Perhaps a simple '+' instead of a concat might make the intent clearer;
I had to look up what concat does...
> }
>
> location = location.substring(0, lastSlash + 1) + filename;
> diff -r 64b9d3a8239c netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Mar 03 17:56:00 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Mar 04 13:35:15 2011 -0500
> @@ -1269,12 +1269,18 @@
> for (ResourcesDesc desc: descs) {
> JARDesc[] jars = desc.getJARs();
> for (JARDesc aJar: jars) {
> - if (jar == aJar) {
> - if (Boolean.valueOf(desc.getPropertiesMap().get("jnlp.packEnabled"))) {
> - usePack = true;
> - }
> - if (Boolean.valueOf(desc.getPropertiesMap().get("jnlp.versionEnabled"))) {
> - useVersion = true;
> + if (jar.getLocation().equals(aJar.getLocation())) {
> + if (file instanceof PluginBridge) {
> + PluginBridge pb = (PluginBridge) file;
> + usePack = pb.usePack();
> + useVersion = pb.useVersion();
> + } else {
> + if (Boolean.valueOf(desc.getPropertiesMap().get("jnlp.packEnabled"))) {
> + usePack = true;
> + }
> + if (Boolean.valueOf(desc.getPropertiesMap().get("jnlp.versionEnabled"))) {
> + useVersion = true;
> + }
> }
You might be able to simplify this code. We only need this loop because
JNLPs have different resources sections and each section gets to specify
packEnabled and versionEnabled (at least, that's how I understand it);
this loop finds the right resources section. For the plugin, we dont
have resources sections, so we can do the check outside the loop. There
should be no need for the URL.equals() either then.
Cheers,
Omair
[1]
http://download.oracle.com/javase/6/docs/api/java/net/URL.html#equals%28java.lang.Object%29
More information about the distro-pkg-dev
mailing list