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

[Omair Majid]

On 02/10/2011 03:27 AM, Dr Andrew John Hughes wrote:
> On 08:26 Thu 10 Feb     , Dr Andrew John Hughes wrote:
>> On 18:13 Wed 09 Feb     , Omair Majid wrote:
>>> 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?
>>>
>>
>> Ok so use System.out for the exceptions the user should always see,
>> and System.err for debug output.  Then 2>  can capture the debug output
>> separate from the other output.
>>
>> I agree it should be a separate cleanup patch.
>>
>> I think they are both if (debug) cases in this patch so use System.err.

Done and pushed. Thanks for the reviews.

Cheers,
Omair