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

[Jiri Vanek]

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.