[rfc][icedtea-web] ResourceTracker Download Refactor

[Jie Kang]

----- Original Message -----
> On 10/29/2014 09:45 PM, Jie Kang wrote:
> >
> >
> > ----- 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.
> 
> Yes, the code is much more readable!
> >
> >
> >
> > 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.
> >
> >
> >
> >
> 
> Hi!
> 
> The patch looks good.
> 
> Few nits:
> You are removing some comments or renaming variables (like e to ex) whcih
> reallys eems to be
> unnecessary. I would let them as they are. But depends on you.

I have changed them back.

> 
> I'm not sure what to think about versionless method overloading:( How rest of
> ITW is doing it?
> If the (string,null) call is more wide spread, then I'm for it, and also to
> replace other calls like
> that.

I have spent a lot of time looking at Resource:requestVersion and Resource:downloadVersion fields and it seems to me, that downloadVersion is useless, and requestVersion is used to support versioned JARs. 

I personally think we should change the functions to not require Version since they do not even use the field. Maybe I can post a patch so you can see what it looks like?

> 
> Last nit you will not like - unit XOR reproducers for your new methods is
> really wonted. I think
> that reproducer - moreover being written as unittestm and only using prepared
> resources, would be
> completely enough.

Technically, nearly every reproducer for IT-W tests this code.

I am having a lot of trouble trying to create tests for this since there aren't any public methods changed. As well, I haven't been able to write any unit tests using local files because the Resource/Cache system prevents download+caching of local files from occurring. 

Given the number of reproducers we have, I feel like it is decently tested already.

Can you provide any suggestions or hints on what kind of tests to write?

> 
> Also you did few:
> - catch (Exception ex) {
> +        } catch (IOException e) {
> I would strongly discourage you from doing so...  Not only IO exception can
> happen here.... And we
> do not wont to die...


Right. I have changed it back.

> 
> Thank you for doing this extremely necessary stuff!
>     J.
> 
> 

-- 

Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-rt-download-refactor-3.patch
Type: text/x-patch
Size: 20270 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20141106/471562a7/itw-rt-download-refactor-3-0001.patch>