[RFC][icedtea-web]: PR658 - jnlp.{pack,version}Enabled
Omair Majid
omajid at redhat.com
Fri Mar 4 15:28:28 PST 2011
On 03/04/2011 06:16 PM, Denis Lila wrote:
>> > 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.
> Sure. I've attached the patches. packEnabledp1.patch is the first one,
> and packEnabledp2.patch is the second one.
>
Looks like i un-persuaded you to do 4 patches. I should have read the
second email first :)
>> > Makes sense. Good catch!
> Thanks :D
>
>> > 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.
> I was uncomfortable with this too. I think I found a good way to fix it
> using polymorphism.
>
>> > 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.
> The above allowed me to not make any changes to how we get the
> DownloadOptions for jnlp applications (that aren't applets).
> I just needed to move the body of the DL option getter to JNLPFile.java.
>
> Ok to push now?
>
Comments inline.
>
> exporting patch:
> # HG changeset patch
> # User Denis Lila<dlila at redhat.com>
> # Date 1299278183 18000
> # Node ID 669de8a29c6e27d81b465743ae8d4cd1afcd6f47
> # Parent 64b9d3a8239cc4521971c7a867f05ae92501bb70
> Fixed packed jar naming and pack.gz decompression problem.
>
> diff -r 64b9d3a8239c -r 669de8a29c6e ChangeLog
> --- a/ChangeLog Thu Mar 03 17:56:00 2011 -0500
> +++ b/ChangeLog Fri Mar 04 17:36:23 2011 -0500
> @@ -1,3 +1,12 @@
> +2011-03-04 Denis Lila<dlila at redhat.com>
> +
> + * 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 concatenating strings instead of replacing.
Cant parse this line.
> +
> +
> 2011-03-03 Deepak Bhole<dbhole at redhat.com>
>
> * plugin/icedteanp/IcedTeaNPPlugin.cc
> diff -r 64b9d3a8239c -r 669de8a29c6e 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 17:36:23 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());
Can you make a note (in a comment) about how order matters when checking
for content-type/compression?
> @@ -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 -r 669de8a29c6e 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 17:36:23 2011 -0500
> @@ -122,7 +122,7 @@
> filename = name + "__V" + resource.requestVersion + "." + extension;
> }
> if (usePack) {
> - filename = filename.replace(".jar", ".pack.gz");
> + filename = filename + ".pack.gz";
> }
>
> location = location.substring(0, lastSlash + 1) + filename;
>
>
Other then the two suggestions, this patch looks good.
> exporting patch:
> # HG changeset patch
> # User Denis Lila<dlila at redhat.com>
> # Date 1299280567 18000
> # Node ID f1a5201c6b04e0da78db3dd41b41b91e857f6312
> # Parent 8c953caa05bc335408c1cfc6c54cc6f2d79f9c6c
> Fix PR658
>
> diff -r 8c953caa05bc -r f1a5201c6b04 ChangeLog
> --- a/ChangeLog Fri Mar 04 17:45:57 2011 -0500
> +++ b/ChangeLog Fri Mar 04 18:16:07 2011 -0500
> @@ -1,3 +1,16 @@
> +2011-03-04 Denis Lila<dlila at redhat.com>
> +
> + * netx/net/sourceforge/jnlp/JNLPFile.java:
> + (getDownloadOptionsForJar): Moved here from JNLPClassLoader.java.
> + * netx/net/sourceforge/jnlp/PluginBridge.java
> + (usePack, useVersion): added.
> + (PluginBridge): initializing usePack and useVersion.
> + (getDownloadOptionsForJar): return the download options.
> + * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> + (getDownloadOptionsForJar): logic moved to JNLPFile.java and its
> + subclasses. Now just calling file.getDownloadOptionsForJar.
> + * NEWS: Updated with fix of PR658.
> +
> 2011-03-04 Denis Lila<dlila at redhat.com>
>
> * netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> diff -r 8c953caa05bc -r f1a5201c6b04 NEWS
> --- a/NEWS Fri Mar 04 17:45:57 2011 -0500
> +++ b/NEWS Fri Mar 04 18:16:07 2011 -0500
> @@ -20,6 +20,7 @@
> * NetX
> - Use Firefox's proxy settings if possible
> - RH669942: javaws fails to download version/packed files (missing support for jnlp.packEnabled and jnlp.versionEnabled)
> + - PR658: now jnlp.packEnabled works with applets.
> * Plugin
> - PR475, RH604061: Allow applets from the same page to use the same classloader
> - PR612: NetDania application ends on java.security.AccessControlException: access denied (java.util.PropertyPermission browser read)
> diff -r 8c953caa05bc -r f1a5201c6b04 netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java Fri Mar 04 17:45:57 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java Fri Mar 04 18:16:07 2011 -0500
> @@ -645,4 +645,32 @@
> return newVMArgs;
> }
>
> + /**
> + * XXX: this method does a "==" comparison between the input JARDesc and
> + * jars it finds through getResourcesDescs(). If ever the implementation
> + * of that function should change to return copies of JARDescs objects,
> + * then the "jar == aJar" comparison below should change accordingly.
You might want to add this comment to getResourcesDesc() too. That's
where a person making the change would look for such comments.
> + * @param jar: the jar whose download options to get.
> + * @return the download options.
> + */
> + public DownloadOptions getDownloadOptionsForJar(JARDesc jar) {
> + boolean usePack = false;
> + boolean useVersion = false;
> + ResourcesDesc[] descs = getResourcesDescs();
> + for (ResourcesDesc desc: descs) {
> + JARDesc[] jars = desc.getJARs();
> + for (JARDesc aJar: jars) {
> + if (jar == aJar/*jar.getLocation().equals(aJar.getLocation())*/) {
Please remove the comment.
> + if (Boolean.valueOf(desc.getPropertiesMap().get("jnlp.packEnabled"))) {
> + usePack = true;
> + }
> + if (Boolean.valueOf(desc.getPropertiesMap().get("jnlp.versionEnabled"))) {
> + useVersion = true;
> + }
> + }
> + }
> + }
> + return new DownloadOptions(usePack, useVersion);
> + }
> +
> }
> diff -r 8c953caa05bc -r f1a5201c6b04 netx/net/sourceforge/jnlp/PluginBridge.java
> --- a/netx/net/sourceforge/jnlp/PluginBridge.java Fri Mar 04 17:45:57 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java Fri Mar 04 18:16:07 2011 -0500
> @@ -40,6 +40,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)
> @@ -134,6 +136,27 @@
> // same page can communicate (there are applets known to require
> // such communication for proper functionality)
> this.uniqueKey = documentBase.toString();
> +
> + usePack = false;
> + useVersion = false;
> + String jargs = atts.get("java_arguments");
> + if (jargs != null) {
> + for (String s : jargs.split(" ")) {
> + String[] parts = s.trim().split("=");
> + if (parts.length == 2&& Boolean.valueOf(parts[1])) {
> + if ("-Djnlp.packEnabled".equals(parts[0])) {
> + usePack = true;
> + } else if ("-Djnlp.versionEnabled".equals(parts[0])) {
> + useVersion = true;
> + }
> + }
> + }
> + }
> + }
> +
> + @Override
> + public DownloadOptions getDownloadOptionsForJar(JARDesc jar) {
> + return new DownloadOptions(usePack, useVersion);
> }
>
> public String getTitle() {
> diff -r 8c953caa05bc -r f1a5201c6b04 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Mar 04 17:45:57 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Mar 04 18:16:07 2011 -0500
> @@ -1321,25 +1321,7 @@
> }
>
> private DownloadOptions getDownloadOptionsForJar(JARDesc jar) {
> - boolean usePack = false;
> - boolean useVersion = false;
> -
> - ResourcesDesc[] descs = file.getResourcesDescs();
> - 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;
> - }
> - }
> - }
> - }
> -
> - return new DownloadOptions(usePack, useVersion);
> + return file.getDownloadOptionsForJar(jar);
> }
Nice use of polymorphism.
Other than the minor changes, it looks fine.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list