[RFC][icedtea-web] Fix for single instance service with applets using jnlp_href

[Danesh Dadachanji]

On Wed, Sep 5, 2012 at 7:02 AM, Jiri Vanek <jvanek at redhat.com> wrote:
> On 09/04/2012 10:48 PM, adomurad wrote:
>>
>> Comments inline.
>>
>
> Thanx. Pushed to head and 1.3
>
>

Thanks so much for finishing this off Jiri!

>>> So here is his patch with finished and test:
>>>
> ...
>
>>> +            if (JNLPRuntime.isDebug()) {
>>> +                System.out.println("Single instance applet is already
>>> running.");
>>> +            }
>>> +            throw launchError(new LaunchException(file, ieex,
>>> R("LSFatal"), R("LCLaunching"), R("LCouldNotLaunch"),
>>> R("LSingleInstanceExists")));
>>
>> A new exception type for these calls (as you said) would probably be good.
>
>
> Not done - there is already InstanceRunning exception, and Danesh have
> wrapped it (see ieex above)
> So I'm considering it as more correct.
>
>

FWIW, I copied existing code from the webstart calls to this. IIRC,
the Launcher class needs a LaunchError to be caught but uses the ieex
as the reason afterwards. That's where the "single instance exists"
mention in the exception message comes from.

Cheers,
Danesh

>>>           } catch (LaunchException lex) {
>>>               throw launchError(lex);
>
> ...
>
>>> +        }
>>> +    }
>>> +
>>> +    //killer is started when params are recieved, or when application is
>>> running to long
>>
>> recieved -> received :) (nit, non-blocker)
>>>
>>> +    private void startKiller(int a) {
>>> +        synchronized (self) {
>>> +            if (killer == null) {
>
> ...
>
>>> +        }
>>> +        System.out.println("Paramaters received by
>>> SingleInstanceChecker:" + paramsString);
>>
>> paramaters -> parameters in a few places (nit, non-blocker)
>
>
> Both fixed. Thanx
>>>
>>>
>>> +        startKiller(1);
>>> +    }
>
> ...
>
>>> +
>>> +    }
>>
>> I don't know how I feel about this main 'test'. I suppose theres no better
>> place for it.
>
>
> Ok. Kept and moved to the Javadoc of main method (as You wished?) Removed
> from bottom where you didnt like it.
>
>>> +    /**
>>> +     *Passed /SingleInstanceTest.jnlp x /SingleInstanceTest.jnlp
>>> +     *Passed /SingleInstanceTest.jnlp x /SingleInstanceTestWS.jnlp
>>> +     *FAILED /SingleInstanceTest.jnlp x
>>> /SingleInstanceTest_jnlpHref.html - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTest.jnlp x
>>> /SingleInstanceTest_clasical.html - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *Passed /SingleInstanceTestWS.jnlp x /SingleInstanceTest.jnlp
>>> +     *Passed /SingleInstanceTestWS.jnlp x /SingleInstanceTestWS.jnlp
>>> +     *java.lang.NoSuchMethodException: SingleInstanceTest.access$000()
>>> +     *FAILED /SingleInstanceTestWS.jnlp x
>>> /SingleInstanceTest_jnlpHref.html - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTestWS.jnlp x
>>> /SingleInstanceTest_clasical.html - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTest_jnlpHref.html x
>>> /SingleInstanceTest.jnlp - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTest_jnlpHref.html x
>>> /SingleInstanceTestWS.jnlp - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *Passed /SingleInstanceTest_jnlpHref.html x
>>> /SingleInstanceTest_jnlpHref.html
>>> +     *FAILED /SingleInstanceTest_jnlpHref.html x
>>> /SingleInstanceTest_clasical.html - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTest_clasical.html x
>>> /SingleInstanceTest.jnlp - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTest_clasical.html x
>>> /SingleInstanceTestWS.jnlp - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *FAILED /SingleInstanceTest_clasical.html x
>>> /SingleInstanceTest_jnlpHref.html - java.lang.AssertionError:
>>> SingleInstanceTest.main's first PR stdout should contain Paramaters received
>>> by SingleInstanceChecker but didn't
>>> +     *Passed /SingleInstanceTest_clasical.html x
>>> /SingleInstanceTest_clasical.html
>>> +     */
>>> +}
>>
>> Please put a comment on the top of this block or remove it ... its
>> rather confusing to see when skimming the test.
>>
>> Fine to push assuming tests pass - the only thing that I really want
>> changed before pushing is a comment before the block of Pass/FAIL's, or
>> its removal. Exception could be good to add too but its up to you.
>
>
> Yap they are passing everywhere except epiphany in f16 (whic is ignoring
> --new-tab)
>>
>>
>