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

[Jiri Vanek]

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.


J.
> Cheers,
> Danesh