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

[Danesh Dadachanji]

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!

Cheers,
Danesh