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

[Jie Kang]

----- 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.
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern