[rfc][icedtea-web] Stripping semicolon tags from jar urls

[Andrew Azores]

Hi,

I actually am already working on producing unit testing for this change, 
but hadn't included it yet by the time I sent the previous email. Next 
time I will wait and include unit testing code first. Also thanks very 
much for the input on how to produce unit tests properly, I'll take that 
into account as I continue to work on it.

> I'm not 100% sure what this means - you are checking of last 
> appearence of dot, why? You wan to avoid case like 
> this;is;my.jar;and_this_not ?
>
> Then maybe little bit of sophistication like 
> href.toUpperCase.lastIndexof(".JAR") is worhty.
I was not sure if every case of file extension would contain the ".jar" 
substring. I suppose a JAR filename should have a file extension 
containing this substring, or perhaps not have a file extension, but 
should not have a file extension which does not contain the ".jar" 
substring. eg "file.jar" or "file.jar.gz" or "file.jar.pack.gz" are all 
good names, "file" alone is reasonable, but "file.zip" is not a 
reasonable name to be specified as the URL for a JAR. I will change the 
check to search for the .jar substring rather than the last occurrence 
of a '.'.

> If so, are you really trying to avoid case like 
> starngejar;ignored;ignored;ignored ? I must say I do not like this, 
> but maybe your aporach is good middle way.
> If nothing else comment would be nice.
In such a case, no "file extension" is found, and so none of the 
"stripping" logic would be performed and the file's URL would be left 
unchanged. If the file was for example specified in the .jnlp as 
"strange.jar;ignored;ignored," then the parser would write this URL as 
"strange.jar" instead.

>
>> +            int firstSemiColon = lastPeriod == -1 ? -1 : 
>> href.indexOf(';', lastPeriod);
>
>
> Please avoid ?: construction as much as possible. This is exactly the 
> case where it should not be.
>
>> +            href = firstSemiColon == -1 ? href : href.substring(0, 
>> firstSemiColon);
>
> Again... not shortest possible code, most readable code :)
Okay, thank you for the feedback, I will make this more readable and 
avoid ternary operators.

>
>> +            // Strip any characters after the file extension iff 
>> there is a semicolon after the extension.
>> +            // eg "lib/applet.jar;no_javaws_cheat" should become 
>> "lib/applet.jar"
>> +        }
>> +
>>           if (href == null)
>>               return null; // so that code can throw an exception if 
>> attribute was required
>>
>
> Anyway I'm quite wondering if this is best place for this. This is 
> transforming all urls which appears in jnlp file. Cant there be some 
> case where ";" is valid part?
> I can imagine some obscure 
> http://my.url.net/myPage?my.script_returning;jar;version;15
>
>
> I would maybe rather see this as fall back when jar is not found, then 
> try to cut it later. But I do not insist, and am open to rfc as I'm 
> not sure here :)
>
> J.
>
Yes, my first thought was putting this check somewhere like the 
ResourceTracker, so that if downloadResource fails due to this bug, then 
we can apply this logic and try again. I'm not sure that ResourceTracker 
would have been the best location in the first place, but I don't really 
know if Parser is any better as you pointed out. I moved the check based 
on other feedback I received, so I will keep looking into better places 
to perform this.

Thanks very much,

Andrew A