Icedtea-web splashscreen implementation

[Jiri Vanek]

On 11/01/2012 05:30 PM, Omair Majid wrote:
> On 11/01/2012 11:32 AM, Jiri Vanek wrote:
>> On 10/29/2012 07:42 PM, Omair Majid wrote:
>>> Hi Jiri,
>>>
>>> Comments in-line below.
>>
>> Thank you :)
>>
>> But you are giving me hard times. It needs a lot of garbage-searching in
>> deep corners of my nearly-forgotten memories to recall why I have done
>> something.
>
> Err... sorry.

NP:)
>
>>>>    }
>>>> diff -r c52fcd37dbd8 netx/net/sourceforge/jnlp/Launcher.java
>>>> --- a/netx/net/sourceforge/jnlp/Launcher.java    Tue Oct 23 09:56:26
>>>> 2012 +0200
>>>> +++ b/netx/net/sourceforge/jnlp/Launcher.java    Wed Oct 24 11:46:08
>>>> 2012 +0200
>>>
>>>>        protected ApplicationInstance launchApplet(JNLPFile file,
>>>> boolean enableCodeBase, Container cont) throws LaunchException {
>>>
>>>> +        if (JNLPRuntime.getForksAllowed()&&   file.needsNewVM()) {
>>>> +            if (!JNLPRuntime.isHeadless()) {
>>>> +                SplashScreen sp = SplashScreen.getSplashScreen();
>>>> +                if (sp != null) {
>>>> +                    sp.close();
>>>> +                }
>>>> +            }
>>>
>>> This is really puzzling. Forks are never allowed for applets, so the
>>> first if statement should never evaluate to true. But if we could start
>>> new VMs for applets, we would need to prevent applets from running in
>>> this VM. We need to return from this method to avoid executing further
>>> code, but we also need to notify the plugin that another VM now handles
>>> the plugin.
>>
>> Really? Not even when launched from jnlfile even with xmx?? I thin there
>> is no reason  not to allow them...
>> However I have followed your advice. But I really think that applets
>> launched via jnlp should be able to fork...
>
> Ah, you are right, of course! Can you post this particular change as a
> separate patch? It is not related to the rest of the patch and I will
> okay it ASAP.
>
> But you still need to create a new JVM and make it run that applet and
> stop this JVM from running it.

I have reverted this section to previous state (without throw).

it  should be few lines to add xfork capability, but as you said, it is unrelated to this oone, and especially will need round of tests and reproducer. (== time :)
>
>>>> -    protected AppletInstance createApplet(JNLPFile file, boolean
>>>> enableCodeBase, Container cont) throws LaunchException {
>>>> +    protected  synchronized  AppletInstance createApplet(JNLPFile
>>>> file, boolean enableCodeBase, Container cont) throws LaunchException {
>>>
>>> Okay, this "synchronized" keyword here looks rather out of place. If
>>> synchronization is really needed, I don't think we should be adding it
>>> to this class.
>>
>> Well here we have an disagreeing. I have played with those two newly
>> synchronised methods again.
>> When two (or even more) applets are on same page, launched together,
>> without both method synchronised the splashscrens are initiated wiredly:(
>> After those tests I have kept those new synchronisation in code.
>>
>> What is actually your reason not to add them?
>
> As far as I can tell there are no existing 'synchronized' methods in
> this class. I am not sure if this class is meant to be thread safe or
> not. It forks a bunch of threads and then waits for them to join but I
> don't see any synchronization otherwise. This addition of synchronized
> adds additional responsibility to the class, but without any other
> details or explanation.
>
> I would be okay with adding them if you could clarify why they are
> needed here. Instead of adding 'synchronized' to two methods, lets find
> out why that weirdness is happening and the best way to fix it. Does
> this class need to be made thread-safe (and the documentation for this
> class updated)? Or do we need to use some higher level primitives to
> ensure exclusivity? Unless we understand the actual problem, this fix
> might turn out to be a bad decision. It may even cause deadlocks if we
> don't fully understand its consequences.
>
> If you really think this is needed right now, then please add a FIXME at
> least so we can double check this later. But I would really prefer to
> understand and fix the underlying problem correctly once and for all.

ok. I have removed synchronisation keywords, and added fixme.

Do you mind to eyball patch last times?
>
> I am okay with everything else in the patch.
>
> Cheers,
> Omair
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: splashscreenIntegration_XI.patch
Type: text/x-patch
Size: 34720 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121101/4821bf88/splashscreenIntegration_XI.patch