[rfc][icedtea-web] ResourceTracker Download Refactor

[Jie Kang]

----- Original Message -----
> On 08/15/2014 05:50 PM, Jie Kang wrote:
> > Hello,
> >
> >
> > The attached patch makes what looks like a huge refactor to
> > ResourceTracker's function: 'downloadResource'.
> >
> > Previously downloadResource was a single function over 100 lines long.
> > Inspired by the book 'Clean Code' by Robert C. Martin, I believe this to
> > be quite bad. The patch simply extracts portions of the function into
> > other functions with descriptive names.
> >
> > As 'downloadResource' is extremely important to ITW's function, I want to
> > stress that there is NO new code and that when refactoring I made sure
> > that there was NO reordering of code. The execution is the exact same
> > prior to this patch.
> >
> > The reason for this patch is that the codebase for ITW is in some places
> > quite old (code from 2003?) and unnecessarily complicated. This is a small
> > part of an attempt to clean the code up and increase extensibility for
> > future enhancements (ie. supporting more of JNLP's spec).
> >
> >
> > PS. I have manual tested ITW with the patch applied and have not seen any
> > regressions.
> >
> >
> >
> > Thanks,
> >
> Please adapt to a head and test again. I think there is no longer any need to
> delay this good patch.
> 
> Sory for dealying!
> 
> J.
> 

Hello,


The attached patch is a refactor to ResourceTrackers download function.

Also attached is a copy of the results from one run of reproducers with the patch applied. Since this affects a critical part of ITW, I have tried to be extremely careful in order to make sure there are no regressions. I have done a large amount of testing prior to posting this patch to the list. 

The main purpose of this patch is to make it easier to understand what the download function is doing, in order to make further improvements on it in the future.


Thoughts? If you could apply the patch and read over the new code, and see if you can understand what is going on, it would be greatly appreciated.



PS Notes:

There is a function added to CacheUtil: public static File getCacheFile(URL source)

This calls the previously existing function: getCacheFile(URL source, Version version) with version==null. This might seem dangerous but, if you read through the code-lines for getCacheFile, you will notice that 'version' is never used. 'version' is passed to two functions, 'isCacheable' and 'makeNewCacheFile', both of which do not use version at all.

I am not sure what the intentions of the previous developers were with the Version object so I have chosen to keep the code intact for now.




Thanks,


-- 

Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-rt-download-refactor-2.patch
Type: text/x-patch
Size: 18429 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141029/9db4b3cf/itw-rt-download-refactor-2-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw.dist.rt.download.0
Type: application/octet-stream
Size: 294201 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141029/9db4b3cf/itw.dist.rt.download-0001.0>