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

[Jie Kang]

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


Regards,

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

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern