[icedtea-web] RFC: add support for packEnabled and versionEnabled

[Omair Majid]

On 02/09/2011 05:33 PM, Dr Andrew John Hughes wrote:
> On 13:04 Wed 09 Feb     , Omair Majid wrote:
>> 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.
>>
>
> I'd prefer System.err but I could live with System.out as long as we're consistent.
>
> Is there output on System.out that appears regardless of whether debugging is on or off?
> If so, the debug dependent stuff should be on System.err so it can be captured separately.
>

Netx is not supposed to output anything to console - other than 
exceptions that we want the user to see. If it does (outside of 
debug/verbose mode) it's a bug.

At the moment, it's a fairly mixed bag: some parts use system.out, 
others use system.err:

$ grep -rin 'err.print' netx | wc -l
43
$ grep -rin 'out.print' netx | wc -l
130

I suppose I can make everything use System.err, but that would be 
separate patch. For this patch, which one should I use?

>>>> @@ -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.
>
> You've snipped some of what I was commenting about.  I agree the additional
> conditional needs to be there but there's a lot of change in that area just
> to flip the equals comparison which hides the actual if extension.  The bit
> above that you snipped is JUST a change in the equals and could be done
> in a separate patch.
>

Yes, but it's necessary here. This was the overall code before:

if (contentEncoding != null) {
   if (contentEncoding.equals("gzip")) {
     // handle gzip
   } else if (contentEncoding.equals("pack200-gzip")) {
     // handle pack200-gzip
   }
}

I want to execute the same code as in the pack200-gzip branch but using 
explicit files. In this case, 
realLocation.getPath().endsWith(".pack.gz") is true but contentEncoding 
is null (since the server does not encode the content). So the code becomes:

   if (contentEncoding.equals("gzip")) {
     // handle gzip
   } else if (contentEncoding.equals("pack200-gzip") ||
       realLocation.getPath().endsWith(".pack.gz")) {
     // handle pack200-gzip
   }

But since contentEncoding is null, the first if statement will throw a 
NullPointerException. The smallest change I can think of to make this 
work is to flip the equals (since "constant string".equals(null) returns 
false). The following code handles (or should handle, barring any bugs) 
both implicit and explicit pack200 compression correctly.

   if ("gzip".equals(contentEncoding)) {
     // handle gzip
   } else if ("pack200-gzip".equals(contentEncoding) ||
       realLocation.getPath().endsWith(".pack.gz")) {
     // handle pack200-gzip
   }

Does that make more sense?

As a side note, this patch messes up indentation - I have removed the 
original "if (contentEncoding != null)" but left the indentation there.

Cheers,
Omair