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

[Jiri Vanek]

On 06/06/2013 05:50 PM, Jiri Vanek wrote:
> On 06/06/2013 05:07 PM, Andrew Azores wrote:
>> Changelog:
>>
>> * tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java: added stripHttpPathParams method to
>> remove semicolon-delimited "tags" from end of JAR URLs
>>
>> * tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java: added test case for new
>> method in TinyHttpdImpl
>>
>> * tests/reproducers/simple/StripHttpPathParams/resources/StripHttpPathParams.html: browser-launched
>> applet test case for reproducer
>> * tests/reproducers/simple/StripHttpPathParams/resources/StripHttpPathParams.jnlp: JNLP test case
>> for reproducer
>> * tests/reproducers/simple/StripHttpPathParams/srcs/StripHttpPathParams.java: reproducer
>> * tests/reproducers/simple/StripHttpPathParams/testcases/StripHttpPathParamsTest.java: reproducer
>>
>>
>> Moved the semicolon-stripping logic out of Parser since it turns out it wasn't needed there anymore,
>> but it was needed for TinyHttpdImpl. Changed unit testing to target this instead. Fixed existing
>> reproducer and added HTML applet test case.
>>
> After discussion
>
>
>
> Nice work on this already !
>
>
>
>                           FileInputStream f = new FileInputStream(pp);
>                           f.read(b);
> +                        f.close();
>                           String content = ""
>
>
> Are you sure with this close? I'm not.... I'm actually against  this line unless it is defended.
> Before so - please run all reproducers and double check that this line have not caused any issues.
>
>
>
> rest is only cosmetic changes please:
>
>   public static String stripHttpPathParams(String url) {
> +        if (url == null)
> +            return null;
> +
>
> Brackets needed around condition body
>
> +    public void testStripHttpPathParamsLaunch() throws Exception {
> +        ProcessResult pr = server.executeJavawsHeadless("/StripHttpPathParams.jnlp");
> +        Assert.assertTrue("stdout should contain \"running\" but did not",
> pr.stdout.contains("running"));
> +    }
>
>
> Indentation issue
>
> +        System.out.println("*** APPLET RUNNING ***");
> +        System.exit(0);
>
> The exit is redundant for applets. Pelase remove
>
> +        Assert.assertTrue("stdout should contain \"*** APPLET RUNNING ***\" but did not",
> pr.stdout.contains("*** APPLET RUNNING ***"));
> +        Assert.assertTrue(pr.wasTerminated);
>
> Please do not copypaste - the *** APPLET RUNNING ***  is known consatnt in test-extensions.
>
>
> +        Assert.assertTrue(pr.wasTerminated);
>
> I know this is wide spread across the tests, but I believe it is wrong. Unless you strongly desire
> to keep it here, please remove.
>
> Unittests do also have strange indentation. Are you using some formatting tool? Both Netbens and
> Eclipse can do the job for you.  In icedtea-web is .settiings directory with config files for ITW if
> you wont.
>
> After above is fixed, then I will push for you.
>
> Also one more nit. As separate changeset, can you please
>
> extract the:
>
>                           p = URLDecoder.decode(p, "UTF-8");
>                           p = p.replaceAll("\\?.*", "");
>                           p = (".".concat((p.endsWith("/")) ? p.concat("index.html") :
> p)).replace('/', File.separatorChar);
>                          p = stripHttpPathParams(p);
>
> (and maybe few more lines)
>
> To some method urlToFile or similar and - especially -  with unittes in test-extensions-tests as you
> are already skilled in.  The future urlToFile have already grown a bit and it is not clear what is
> is supposed to do :)
>
>
> Good job! Thankx for your code!
> J.


One more nit - +		pr.process.destroy(); is not needed. It is done by engine in correct time. (again, 
I know it was left in some older testcases)

Sorry for this.

J.