[rfc][icedtea-web] ResourceTracker Download Refactor

[Jiri Vanek]

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'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.

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.

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...

Thank you for doing this extremely necessary stuff!
    J.