[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch

[Saad Mohammad]

On 07/28/2011 08:31 PM, Dr Andrew John Hughes wrote:
> On 10:47 Thu 28 Jul     , Saad Mohammad wrote:
>> On 07/25/2011 09:54 AM, Dr Andrew John Hughes wrote:
>>> On 09:15 Thu 21 Jul     , Jiri Vanek wrote:
>>>> On 07/20/2011 10:26 PM, Deepak Bhole wrote:
>>>>> * Saad Mohammad<smohammad at redhat.com>    [2011-07-19 11:04]:
>>>>>>>    I have attached the updated copy of Patch1.
>>>>>>>
>>>>>>>    On 07/19/2011 02:47 AM, Jiri Vanek wrote:
>>>>>>>>>    >>
>>>>>>>>>    >>I hope this is the 110% for Patch1! ;)
>>>>>>>>    >99% ;)
>>>>>>>    This is for sure the 110% ;)
>>>>>>>
>>>>>>>    --
>>>>>>>    Cheers,
>>>>>>>    Saad Mohammad
>>>>>>>
>>>>> Hi Saad,
>>>>>
>>>>> I have a few minor corrections for this one...
>>>>>
>>>>>>>    +
>>>>>>>    +            if (appTemplate == null&&    launchJNLP == null)
>>>>>>>    +                throw new NullPointerException(
>>>>>>>    +                        "Template JNLP file and Launching JNLP file are both null.");
>>>>>>>    +            else if (appTemplate == null)
>>>>>>>    +                throw new NullPointerException("Template JNLP file is null.");
>>>>>>>    +            else if (launchJNLP == null)
>>>>>>>    +                throw new NullPointerException("Launching JNLP file is null.");
>>>>>>>    +
>>>>> Throwing RuntimeExceptions is generally not a good idea. Consider making
>>>>> the above a checked exception.
>>>> I believe nullpointer exception is correct here. Or it will make mess later.
>>>> Although to repalce it with JNLMatch exception can do no harm.
>>>>
>>> I don't see the point of throwing an NPE, catching it and throwing it attached to something else.
>> The reason why I am doing this ^, is so the class that is creating an
>> instance of JNLPMatcher would not have to handle catching more than one
>> exceptions. Generally if any exception is thrown from this constructor,
>> it would results in the same outcome (JNLPMatcher failed to be
>> initialized). Attaching the exception to the new thrown exception would
>> help identify the cause.
>>
>> An alternative can be to remove the try/catch block completely and let
>> the compiler know that the contructor throws NullPointerException and
>> IOException(while parsing). In this case, the class that initializes
>> JNLPMatcher would have to catch all exceptions, or the two exceptions
>> individually.
>>
>> I think both methods of handing exceptions will work great, but please
>> let me know what you think?
> I'm not suggesting it should throw additional exceptions, but that
> these lines should just directly throw JNLPMatcherException rather
> than this throw-catch-throw dance.

ahh, I see. That does sound like a better solution.
Thanks :). I will make the changes on a new patch.

>>>>>>>    +            XMLElement appTemplateXML = new XMLElement();
>>>>>>>    +            XMLElement launchJNLPXML = new XMLElement();
>>>>>>>    +
>>>>>>>    +            // Remove the comments and CDATA from the JNLP file
>>>>>>>    +            final PipedInputStream pinTemplate = new PipedInputStream();
>>>>>>>    +            final PipedOutputStream poutTemplate = new PipedOutputStream(pinTemplate);
>>>>>>>    +            appTemplateXML.sanitizeInput(appTemplate, poutTemplate);
>>>>>>>    +
>>>>>>>    +            final PipedInputStream pinJNLPFile = new PipedInputStream();
>>>>>>>    +            final PipedOutputStream poutJNLPFile = new PipedOutputStream(pinJNLPFile);
>>>>>>>    +            launchJNLPXML.sanitizeInput(launchJNLP, poutJNLPFile);
>>>>>>>    +
>>>>> The above streams are never closed.
>>>> I' guess, ...
>>>>
>>>> ...see previous email..
>>>>
>>>> ...l) s.close();
>>>> }
>>>>
>>>>
>>>> around s.close() yo can possibly clsoe io exception into JNLPMatcherException.
>>>>
>>> You need a finally block.
>> Yes, I will add the finally block. Thanks :)
>>
>>>> Regards J.
>>>>
>>
>> -- 
>> Cheers,
>> Saad Mohammad
>>


-- 
Cheers,
Saad Mohammad