[rfc][icedtea-web] u51 classpath manifest entry implementation

[Jiri Vanek]

hmhm.

I think That I owe the testcase when jnlp file is lunched like file, but in fact it is connecting 
normlay. Imho the codebase begave well, but worthy to try.

...
>
> Why isn't the first argument to "replace" also a DELIMITER? Sorry to hammer on this point :(
>
Oh no Tahnx. My wrong!

> Also why is the parameter here final, and basically nothing else in the class is final? It doesn't
> seem any more necessary in this method... either use "final" liberally and apply it wherever
> feasible, or use it only when needed. I'd prefer the liberal use, but it's up to you. Marking this
> parameter alone as final like this makes it stand out and look like something special is happening
> with it, but nothing really is.

I made it final because at first I did some obscure things with it.Now it better, but Still I kept it.

>
...
>> ClasspathMatcher.extractPath("some.correct.url:5050://full/path"));
>> +        Assert.assertEquals("/ect.url:5050/full/path",
>> ClasspathMatcher.extractPath("some.corr://ect.url:5050/full/path"));

Well those are so unlikely that I can live with it.
>
> :/ I see you've commented that this case is hard to solve, but this ^ test in particular looks
> concerning. Is it not worthwhile to do some stricter URL validation so we can avoid this kind of
> result? Because to me, this input and its result don't really match up very well. Perhaps there's
> some way you could leverage the standard URL class to do some of the heavy lifting? I'm not sure how
> it will respond to having a protocol like "some.corr" though - probably just going to throw an
> exception. But it's probably worth looking into if you haven't already. You could do some checking
> on the other components of the URL first maybe, and in this case see that the protocol is bad, and
> then either construct a URL without any protocol, or replace the protocol with dummy data, and then
> get the path part from the URL instance. I'm not sure how well this approach will work in general
> though, when you're extracting other parts of the URL, and there are other various malformed pieces.

Nope - the asterix and possibility to miss parts is making URL calss unusable. I tried.
>
>> +
>> +    //total trap url
>> +    @Test
>> +    public void madUrls() throws MalformedURLException {
>> +        String trapUrl1 = "*://:&:://%%20";
>> +        ClasspathMatcher.Parts p1 = ClasspathMatcher.splitToParts(trapUrl1);
>> +        Assert.assertEquals("*", p1.protocol);
>> +        Assert.assertEquals("", p1.domain);
>> +        Assert.assertEquals("&::", p1.port);
>
> Perhaps there should be some validation that the port is numeric...
>

nope. There can be nothing.

>
>> +
>> +        private Boolean processBooleanAttribute(String id) throws IllegalArgumentException {
>> +            String s = getAttribute(id);
>> +            if (s == null) {
>> +                return null;
>> +            } else {
>> +                s = s.trim();
>> +                if (s.equalsIgnoreCase("true") || s.equalsIgnoreCase("false")) {
>> +                    //return ((name != null) && name.equalsIgnoreCase("true"));
>> +                    return Boolean.parseBoolean(s);
>> +                } else {
>> +                    throw new IllegalArgumentException("Unknown value of " + id + " attribute " +
>> s + ". Expected true or false");
>> +                }
>> +            }
>> +        }
>
> Remove the commented out line here as well.

nope - added one more line.
>
> Next few comments are suggestions on Messages wording :)
>
>> diff -r 1e0507976663 netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties    Wed Feb 05 13:55:28 2014 +0100
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties    Wed Feb 05 18:24:20 2014 +0100
>> @@ -569,6 +569,14 @@
>>   SPLASHmissingInformation = Information element is missing, verify source rather
>>   SPLASHchainWas = This is the list of exceptions that occurred launching your applet. Please
>> note, those exceptions can originate from multiple applets. For a helpful bug report, be sure to
>> run only one applet.
>> +CBCheckFile = This application is local file. The codebase check is disabled
>
> The application is a local file. Codebase validation is disabled.
>
>> +CBCheckNoEntry = This application do not have codebase manifest entry specified. Please verify
>> vendor. Continuing
>
> This application does not specify a Codebase in its manifest. Please verify with the applet's
> vendor. Continuing
>
>> +CBCheckUnsignedFail= Codebase DO NOT matches codebase manifest attribute, but application is
>> unsigned. Continuing
>
> The application's codebase does NOT match the codebase specified in its manifest, but the
> application is unsigned. Continuing
>
>> +CBCheckSignedAppletDontMatchException = java applet is not allowed to run when is signed, have
>> codebase manifest entry ({0}), and this do not match to real codebase - {1}
>
> Signed applets are not allowed to run when their actual Codebase does not match the Codebase
> specified in their manifest. Expected: {0}. Actual: {1}
>
>> +CBCheckSignedFail = Codebase DO NOT matches codebase manifest attribute and  application is
>> signed. You are strongly encouraged to terminate application
>
> Application Codebase does NOT match the Codebase specified in the application's manifest, and this
> application is signed. You are strongly discouraged from running this application.

followed
>
> And I think that's about all I have to say (so far). Nice work overall!

All indentation and requested javadoc fixed I hope.
Sharp eye tahnx!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: classpathAttributeAll-2.diff
Type: text/x-patch
Size: 151122 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140206/dc26d371/classpathAttributeAll-2-0001.diff