[rfc][icedteaweb] support for entry-point manifest atribute

[Jiri Vanek]

On 03/18/2015 04:57 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>>
>>
>> ----- Original Message -----
>>> On 02/25/2015 06:19 PM, Jiri Vanek wrote:
>>>> Hello, this is impl of another security manifest att:
>>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#entry_pt
>>>> I would like to go with it to 1.5 too.
>>>>
>>>> Speaking about the attributes:
>>>> does http://www.geogebra.org/webstart/geogebra.jnlp library loading promt
>>>> seems correct to you?
>>>>
>>>> J.
>>> Crap - improved one
>>>
>>> The check is valid only if RIA is signed. I had it removed for testing...
>>
>> Hello,
>>
>>
>> Nice patch :D
>>
>> It might be nice for 1.5 but isn't 1.5 released already? We shouldn't really
>> add new functionality, especially something like this which could easily
>> break user experience.
>>
>> Ie. apps that worked with 1.5.(x) don't work with 1.5.(x+1) anymore (even
>> though it is correct).
>>
>>
>> +    static String[] splitEntryPoints(String eps) {
>>
>> The use of protected static here seems to be only so it's easier to test,
>> yes? Please add a comment saying something like //static and package-private
>> for testing
>>
>> Also, s/eps/entryPointString or something like that. No need to shorten it to
>> something unreadable nowadays.
>>
>>
>> +        throw new LaunchException("None of '" +
>> file.getManifestsAttributes().getEntryPointString() + "' matched yours " +
>> mainClass + ". Thats fatal.");
>>
>>
>> It's not clear to me what exactly is happening with the current message.
>>
>> Can you change it to something similar to:
>>
>>
>> "None of the entry points matched the main class and the applet is signed.
>> This is a security error and the applet will not be launched."
>>
>>
>>
>> +        @Test
>>
>> Please remove the extra tab.
>>
>> +    public void splitEntryPointsReturnsTests() throws Exception {
>> +        Assert.assertArrayEquals(null, JNLPFile.splitEntryPoints("  "));
>> +        Assert.assertArrayEquals(null, JNLPFile.splitEntryPoints(null));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c"},
>> JNLPFile.splitEntryPoints("  a.b.c  "));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c"},
>> JNLPFile.splitEntryPoints("a.b.c"));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c"},
>> JNLPFile.splitEntryPoints("  a.b.c"));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c"},
>> JNLPFile.splitEntryPoints("a.b.c  "));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c", "cde"},
>> JNLPFile.splitEntryPoints("  a.b.c     cde"));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c", "cde"},
>> JNLPFile.splitEntryPoints("  a.b.c cde    "));
>> +        Assert.assertArrayEquals(new String[]{"a.b.c", "cde"},
>> JNLPFile.splitEntryPoints("a.b.c         cde    "));
>>
>> As easy as it is, it's bad style to put all your tests in one place. Readers
>> can't as easily tell what cases you're testing when you do this. You
>> generally want to minimize a test to a single assert if possible, and split
>> them into multiple test methods that can be descriptively named.
>>
>> Can you put them in separate tests with appropriate names?
>>
>> +        Assert.assertNull("classlaoder attached, but should be null",
>> jnlpFile.getManifestsAttributes().getAttribute(new
>> Attributes.Name(JNLPFile.ManifestsAttributes.ENTRY_POINT)));
>>
>> While you're here can you
>>
>> s/classlaoder/classloader
>>
>> Not really required but it looks nicer with correct spelling;;
>>
>>
>> Rest looks great!
>>
>>
>> Regards,
>>
>>>
>>> J.
>
> Hello,
>
> Did you see this review? I noticed it was pushed without any response here and just wanted to make sure.
>
I'm just replying to you. I fixed everything you wonted!

Thanx for eye.

>
> Regards,
>
>>>
>>
>> --
>>
>> Jie Kang
>>
>> OpenJDK Team - Software Engineering Intern
>>
>