[rfc][icedtea-web] Refactor initialize/download out of ResourceTracker and add tests

[Lukasz Dracz]

Hello,

> The code looks good !

> I just have a few small nits
 
> +        CodeWithRedirect result = new CodeWithRedirect();
 
I think this should be renamed to something a bit more descriptive since you end up with
 
+            result.result = responseCode;

I think renaming the result to something like redirectCode or the second result to responseCode (result.responseCode) would help in the readability of the code

+synchronized (resource) {
+                resource.setLocalFile(localFile);
+                // resource.connection = connection;

I think you forgot to remove a comment here

+                resource.setSize(size);
+                resource.changeStatus(EnumSet.of(PRECONNECT, CONNECTING), EnumSet.of(CONNECTED, PREDOWNLOAD));

+        while (-1 != (rlen = in.read(buf))) {
+            resource.incrementTransferred(rlen);
+            out.write(buf, 0, rlen);
+        }
A suggestion that you can choose to follow/not is that
I think it might give better readability to move the assignments down in the code block.

so something like
int rlen = in.read(buf);

while (-1 != rlen) {
    CODE
    rlen = in.read(buf);
}

Another suggestion you can choose to follow/ignore is maybe making the -1 into a constant to help with readability.

Hmm upon further inspection seems all my nits are not even directed at code you wrote, but at the code you are simply moving over.
I will leave it up to you whether to include it in this patch or as a separate patch if you deem them worthy of considering, I suppose as they are very minor nits.

> 
> ----- Original Message -----
> > From: "Jie Kang" <jkang at redhat.com>
> > To: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> > Sent: Tuesday, December 30, 2014 3:17:23 PM
> > Subject: [rfc][icedtea-web] Refactor initialize/download out of
> > 	ResourceTracker and add tests
> > 
> > Hello,
> > 
> > The attached patch moves the download/initialize runnable into a separate
> > class named ResourceDownloader. The tests applicable to the new
> > ResourceDownloader have also been moved from ResourceTrackerTest to
> > ResourceDownloaderTest.
> > 
> > As well a number of tests have been added to ResourceDownloaderTest in
> > order
> > to ensure the capabilities of the downloader are maintained.
> > 
> > 
> > New tests of note:
> > 
> > testDownloadResource
> > testDownloadPackGzResource
> > testDownloadVersionedResource
> > testDownloadVersionedPackGzResource
> > testDownloadLocalResourceFails
> > testDownloadNotExistingResourceFails

Thanks for all the tests !

> > 
> > There are two cases that I will add in a separate patch:
> > 
> > 1 : test redownloading using new setting in TinyHttpdImpl
> > 2 : test download of gzip file (not packgz)
> > 
> > These both need extra work in code outside of ResourceDownloader
> > 
> > 
> > How does it look?

Looks good to me, I think it might be a good idea to get another
set of eyes to also confirm or make sure I didn't miss anything.

Thanks for this patch.

> > 
> > Regards,
> > 
> > --
> > 
> > Jie Kang
> 

Regards,
Lukasz Dracz