[icedtea-web] RFC: add support for packEnabled and versionEnabled
Omair Majid
omajid at redhat.com
Wed Feb 9 10:04:56 PST 2011
On 02/09/2011 11:42 AM, Dr Andrew John Hughes wrote:
> On 15:42 Wed 26 Jan , Omair Majid wrote:
>> Hi,
>>
>> The attached patch adds support for packEnabled and versionEnabled
>> properties in JNLP files. This allows JNLP applications to use pack200
>> compression and versioned jars without requiring special server
>> configuration/software. It also fixes bug RH669942.
>>
>> The patch adds a new class DownloadOptions which specify download
>> options for jars (currently just version and pack200 support). Another
>> new class ResourceUrlCreator is used to get a list of valid urls for a
>> resource. ResourceTracker now uses ResourceUrlCreator to get the list of
>> valid urls for downloading a jar and then tries to find the best one.
>> initializeResource() and downloadResource() both use this url.
>> downloadResource() has been modified to look at the file extension as
>> well as the content-encoding when deciding whether a file is compressed
>> or not.
>>
>> The patch is much bigger than I had expected it to be. It also affects
>> core functionality - downloading jars. I would appreciate any comments
>> and suggestions for improvements.
>>
>> Thanks,
>> Omair
>>
>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=669942
>
> Comments inline.
>
Thanks for looking it over.
>> diff -r 64da2a80df88 netx/net/sourceforge/jnlp/cache/ResourceTracker.java
>> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Tue Jan 25 10:19:20 2011 -0500
>> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Wed Jan 26 15:31:51 2011 -0500
>> @@ -636,16 +648,16 @@
>> String contentEncoding = con.getContentEncoding();
>>
>> if (JNLPRuntime.isDebug()) {
>> - System.err.println("Content encoding for " + resource.location + ": "
>> - + contentEncoding);
>> + System.out.println("Downloading" + resource.location + " using " +
>> + realLocation + " (encoding : " + contentEncoding + ")");
>> +
>
> Shouldn't this still be System.err?
>
I like to send most debug output to System.out, but I dont think it's a
big deal. I can change it if you like.
>> @@ -686,7 +698,8 @@
>> inputStream.close();
>> gzInputStream.close();
>>
>> - } else if (contentEncoding.equals("pack200-gzip")) {
>> + } 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);
>
> I'm not sure these changes are necessary. Although you eliminate the null check, it does create
> a lot of additional noise for no apparent change. Maybe something for a seperate patch?
>
No, it's actually required here. It's the simplest way I can think of to
make the existing code that dealt with content-encoding also work for
pack.gz files. Before, we would ask the server for foo.jar and the
server could send it with content-encoding pack200-gzip (by actually
sending foo.pack.gz). With this patch, we ask the server for foo.pack.gz
directly. The server sends foo.pack.gz with null content encoding. We
must deal with this in the exact same way as foo.jar with content
encoding pack200-gzip.
Notice these lines in the patch:
- } else if (contentEncoding.equals("pack200-gzip")) {
+ } else if ("pack200-gzip".equals(contentEncoding) ||
+ realLocation.getPath().endsWith(".pack.gz")) {
If realLocation.getPath().endsWith(".pack.gz") is true, then
contentEncoding is null. Perhaps I should have made it more explicit.
>> @@ -777,27 +792,45 @@
>> }
>>
>> /**
>> - * Returns the versioned url for a resource
>> - * @param resource the resource to get the url for
>> + * Returns the best URL to use for downloading the resource
>> + *
>> + * @param resource the resource
>> + * @return a URL or null
>> */
>> - private URL getVersionedResourceURL(Resource resource) {
>> - String actualLocation = resource.location.getProtocol() + "://"
>> - + resource.location.getHost();
>> - if (resource.location.getPort() != -1) {
>> - actualLocation += ":" + resource.location.getPort();
>> + private URL findBestUrl(Resource resource) {
>> + DownloadOptions options = downloadOptions.get(resource);
>> + if (options == null) {
>> + options = new DownloadOptions(false, false);
>> }
>> - actualLocation += resource.location.getPath();
>> - if (resource.requestVersion != null
>> -&& resource.requestVersion.isVersionId()) {
>> - actualLocation += "?version-id=" + resource.requestVersion;
>> +
>> + List<URL> urls = new ResourceUrlCreator(resource, options).getUrls();
>> + if (JNLPRuntime.isDebug()) {
>> + System.out.println(urls);
>
> System.err? Possibly with some kind of description of what is being printed.
>
I have still left it as System.out, but added a better description.
>> }
>> - URL versionedURL;
>> - try {
>> - versionedURL = new URL(actualLocation);
>> - } catch (MalformedURLException e) {
>> - return resource.location;
>> + URL bestUrl = null;
>> + for (int i = 0; i< urls.size(); i++) {
>> + URL url = urls.get(i);
>> + try {
>> + URLConnection connection = url.openConnection();
>> + connection.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip");
>> + if (connection instanceof HttpURLConnection) {
>> + HttpURLConnection con = (HttpURLConnection)connection;
>> + int responseCode = con.getResponseCode();
>> + if (responseCode == -1 || responseCode< 200 || responseCode>= 300) {
>> + continue;
>> + }
>> + }
>> + if (JNLPRuntime.isDebug()) {
>> + System.out.println("best url for " + resource.toString() + " is " + url.toString());
>> + }
>> + bestUrl = url;
>> + break;
>> + } catch (IOException e) {
>> + // continue
>> + }
>> }
>> - return versionedURL;
>> +
>> + return bestUrl;
>> }
>>
>> /**
>> diff -r 64da2a80df88 netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java Wed Jan 26 15:31:51 2011 -0500
>> @@ -0,0 +1,163 @@
>> +/* ResourceUrlCreator.java
>> + Copyright (C) 2011 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; 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. */
>> +
>> +package net.sourceforge.jnlp.cache;
>> +
>> +import java.net.MalformedURLException;
>> +import java.net.URL;
>> +import java.util.LinkedList;
>> +import java.util.List;
>> +
>> +import net.sourceforge.jnlp.DownloadOptions;
>> +
>> +public class ResourceUrlCreator {
>> +
>> + protected final Resource resource;
>> + protected final DownloadOptions downloadOptions;
>> +
>> + public ResourceUrlCreator(Resource resource, DownloadOptions downloadOptions) {
>> + this.resource = resource;
>> + this.downloadOptions = downloadOptions;
>> + }
>> +
>> + /**
>> + * Returns a list of URLs that the resources might be downloadable from.
>> + * The Resources may not be downloadable from any of them. The returned order is the order
>> + * the urls should be attempted in.
>> + * @return
>> + */
>> + public List<URL> getUrls() {
>> + List<URL> urls = new LinkedList<URL>();
>> + URL url = null;
>> +
>> + if (downloadOptions.useExplicitPack()&& downloadOptions.useExplicitVersion()) {
>> + url = getUrl(resource, true, true);
>> + if (url != null) {
>> + urls.add(url);
>> + }
>> + url = getUrl(resource, false, true);
>> + if (url != null) {
>> + urls.add(url);
>> + }
>> + url = getUrl(resource, true, false);
>> + if (url != null) {
>> + urls.add(url);
>> + }
>> + } else if (downloadOptions.useExplicitPack()) {
>> + url = getUrl(resource, true, false);
>> + if (url != null) {
>> + urls.add(url);
>> + }
>> + } else if (downloadOptions.useExplicitVersion()) {
>> + url = getUrl(resource, false, true);
>> + if (url != null) {
>> + urls.add(url);
>> + }
>> + }
>> +
>> + url = getVersionedUrlUsingQuery(resource);
>> + urls.add(url);
>> +
>> + urls.add(resource.getLocation());
>> +
>> + return urls;
>> + }
>> +
>> + /**
>> + * Returns a url for the resource.
>> + * @param resource the resource
>> + * @param usePack whether the URL should point to the pack200 file
>> + * @param useVersion whether the URL should be modified to include the version
>> + * @return a URL for the resource or null if an appropraite URL can not be found
>> + */
>> + protected URL getUrl(Resource resource, boolean usePack, boolean useVersion) {
>
> getURL?
>
I have been naming variables/methods as *Url* for a while now, including
multiple methods in this patch itself (look at the method above this
one). The advice mentioned on
http://www.ibm.com/developerworks/library/ws-tip-namingconv.html#h7
makes sense to me. What do you think?
>> + if (!(usePack || useVersion)) {
>> + throw new IllegalArgumentException("either pack200 or version required");
>> + }
>> +
>> + String location = resource.getLocation().toString();
>> + int lastSlash = resource.getLocation().toString().lastIndexOf('/');
>> + if (lastSlash == -1) {
>> + return resource.getLocation();
>> + }
>> + String filename = location.substring(lastSlash + 1);
>> + if (useVersion&& resource.requestVersion != null) {
>> + String parts[] = filename.split("\\.", 2);
>> + String name = parts[0];
>> + String extension = parts[1];
>> + filename = name + "__V" + resource.requestVersion + "." + extension;
>> + }
>> + if (usePack) {
>> + filename = filename.replace(".jar", ".pack.gz");
>> + }
>> +
>> + location = location.substring(0, lastSlash + 1) + filename;
>> + try {
>> + URL newUrl = new URL(location);
>> + return newUrl;
>> + } catch (MalformedURLException e) {
>> + return null;
>> + }
>> + }
>> +
>> + /**
>> + * Returns the URL for a resource, relying on HTTP query for getting the
>> + * right version
>> + *
>> + * @param resource the resource to get the url for
>> + */
>> + protected URL getVersionedUrlUsingQuery(Resource resource) {
>> + String actualLocation = resource.getLocation().getProtocol() + "://"
>> + + resource.getLocation().getHost();
>> + if (resource.getLocation().getPort() != -1) {
>> + actualLocation += ":" + resource.getLocation().getPort();
>> + }
>> + actualLocation += resource.getLocation().getPath();
>> + if (resource.requestVersion != null
>> +&& resource.requestVersion.isVersionId()) {
>> + actualLocation += "?version-id=" + resource.requestVersion;
>> + }
>> + URL versionedURL;
>> + try {
>> + versionedURL = new URL(actualLocation);
>> + } catch (MalformedURLException e) {
>> + return resource.getLocation();
>> + }
>> + return versionedURL;
>> + }
>> +
>> +}
>> \ No newline at end of file
>> diff -r 64da2a80df88 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Jan 25 10:19:20 2011 -0500
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Wed Jan 26 15:31:51 2011 -0500
>> @@ -1243,4 +1246,27 @@
>> jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key));
>> }
>> }
>> +
>> + private DownloadOptions getDownloadOptionsForJar(JARDesc jar) {
>> + boolean usePack = false;
>> + boolean useVersion = false;
>> +
>> + ResourcesDesc[] descs = file.getResourceDescs();
>> + 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;
>> + }
>
> Is Boolean.valueOf necessary? Won't autoboxing handle this?
>
propertiesMap is a Map<String,String>. It contains key,value pairs for
system properties (eg "swing.useSystemFontSettings"). So autoboxing cant
handle it.
Any other thoughts or comments?
Cheers,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 669942-add-pack-and-version-03.patch
Type: text/x-patch
Size: 25507 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110209/f7320382/669942-add-pack-and-version-03.patch
More information about the distro-pkg-dev
mailing list