[RFC][icedtea-web] Fix regression in broken AppletTest (Was Re: Broken applet test)

[Danesh Dadachanji]

On 09/02/12 03:21 AM, Jiri Vanek wrote:
> On 02/09/2012 01:15 AM, Danesh Dadachanji wrote:
>>
>>
>> On 08/02/12 07:34 AM, Jiri Vanek wrote:
>>> On 02/07/2012 12:00 AM, Danesh Dadachanji wrote:
>>
>> [snip]
>>
>>>>
>>>> diff --git a/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>>>> b/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>>>> --- a/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>>>> +++ b/netx/net/sourceforge/jnlp/runtime/AppletEnvironment.java
>>>> @@ -207,6 +207,20 @@ public class AppletEnvironment implement
>>>> }
>>>>
>>>> /**
>>>> + * Set the applet of this environment; can only be called once.
>>>> + */
>>>> + public void setApplet(Applet applet) {
>>>> + if (this.applet != null) {
>>>> + if (JNLPRuntime.isDebug()) {
>>>> + Exception ex = new IllegalStateException("Applet can only be set
>>>> once.");
>>>> + ex.printStackTrace();
>>>
>>> Well this is more then interesting approach of debug message!
>>>
>>> Although personally I have nothing against this (and I was surprised it
>>> is working , and I started to like it:) ) it is not common stile in
>>> icedetea-web sources and just printed out and never thrown exception can
>>> be confusing....
>>>
>>
>> To be quite honest, I was following
>> netx.net.sourceforge.jnlp.runtime.JNLPClassLoader#setApplication (more
>> like mindlessly copying ;) ) so I had never really thought about the
>> exception specifically. Though I agree that one that isn't thrown is a
>> bad idea, I think it may be necessary. See below please.
>>
>>> So...
>>> 1)Shouldn't this exception really be thrown?
>> I think that there is no harm in trying to set the applet again. I do
>> not see why anyone would do this TBH (whereas I can see why
>> setApplication has it) but it if someone tries, icedtea-web treats it
>> as if nothing happened. Since it is ignored, I don't think it is
>> necessary to throw the exception and prevent the code from continuing.
>> It might be a useful way of checking you aren't doing the wrong thing
>> but I would like to assume that if you are trying to set an applet,
>> you know what you're doing!
>>
>>> 2)In case that not, then is this really something what need so screaming
>>> message? Isn't just "INFO - Somebody is trying setting applet second
>>> times
>> I think the original intent of the exception was mainly just to get
>> the stack trace, pointing out where setApplication() was being called
>> from. I think this is pretty useful when debugging but doing it the
>> other way (Thread.currentThread().getStackTrace) might be more
>> complicated to print. What do you think?
>>
>>> 3) how worthy is to have this only in debug mode (It looks like it can
>>> be serious considering "loudness" of printStackTrace).
>> Keeping it in debug mode is probably a good idea since setting the
>> applet again isn't really dangerous. We don't want to unnecessarily
>> print out messages so I would prefer to leave it as is.
>>
>>> a)If this is really harmless that somebody is trying to replace once
>>> setted applet, then I think (2) will be enough (and keep it in isDebug
>>> branch)
>>> b)If this is nearly harmless that somebody is trying to replace once
>>> setted applet but is important to know about it eg because some crash
>>> can be expected in users (not in debug mode) after this happens, then
>>> some info like (2) but without isDebug branching will be most correct.
>>> c)If it is really bad think which should never happens, then throwing
>>> this exception also out of debug mode is correct.
>>>
>>> From my current point of view a) is most correct, but feel free to
>>> disagree.
>>
>> My intention was just to do a safety check to make sure the var wasn't
>> being written over and to me, the exception is just a way of getting a
>> stack trace. While I think it's bad not to throw it, I think not
>> having the stack trace is less convenient. What do you think? I'm fine
>> with printing a normal message when in debug mode (without exception)
>> but I would ideally like to know where it's trying to be set again.
>> It's a sign that something shouldn't be happening and it's easier than
>> finding where you're calling setApplet from IMO.
>>
>> Thanks for the review!
>>
>
> Ok then - I agree throwing is wrong. And one is always angry when
> searching for "where this info-blbla message come from". Ok to commit as
> it was already written. Head and 1.2.
>

Awesome, thanks a lot for your comments!

Pushed to HEAD and 1.2:
http://icedtea.classpath.org/hg/icedtea-web/rev/eb09075636e5
http://icedtea.classpath.org/hg/release/icedtea-web-1.2/rev/2dd44d3ad58d