[rfc][icedtea-web] Refactor initializeResources in ResourceDownloader

[Jiri Vanek]

On 02/11/2015 08:45 PM, Lukasz Dracz wrote:
> Hello,
>
>> Hello,
>>
>>
>> After the lengthy discussion in the thread for "[rfc][icedtea-web] Check
>> online detection even if checked before", I present another solution.
>>
>> The initializeResources method is refactored to incorporate online/offline
>> more explicitly and the JNLPRuntime has a new method 'isConnectable(URL)'
>> that is used to determine whether or not a connection can be made to the
>> host of a supplied URL. The previous functionality of IT-W's online/offline
>> system is maintained with no functional changes to the other methods in
>> JNLPRuntime.
>>
>> Important bits:
>>
>>      private void initializeResource() {
>>          if (!JNLPRuntime.isOfflineForced() &&
>>          JNLPRuntime.isConnectable(resource.getLocation())) {
>>              initializeOnlineResource();
>>          } else {
>>              initializeOfflineResource();
>>          }
>>      }
>>
>> This new initial method makes it very clear what's going to happen. Rather
>> than using JNLPRuntime.detectOnline && JNLPRuntime.isOnline, we now use the
>> new JNLPRuntime.isConnectable. The code within these methods is basically
>> the same as the code within the old initializeResource, just copy-pasted
>> into appropriate section.
>>
>>      public static void detectOnline(URL location) {
>>          if (onlineDetected != null) {
>>              return;
>>          }
>>
>>          JNLPRuntime.setOnlineDetected(isConnectable(location));
>>      }
>>
>>      public static boolean isConnectable(URL location) {
>>          if (location.getProtocol().equals("file")) {
>>              return true;
>>          }
>>
>>          try {
>>              OutputController.getLogger().log(OutputController.Level.ERROR_ALL,
>>              "The host of " + location.toExternalForm() + " file should be
>>              located seems down, or you are simply offline.");
>>              InetAddress.getByName(location.getHost());
>>          } catch (UnknownHostException e) {
>>              return false;
>>          }
>>
>>          return true;
>>      }
>>
>> Note that detectOnline has been refactored to call isConnectable. The
>> functionality is the same though as 'isConnectable' is copy-pasted code from
>> the original detectOnline.
>>
>>
>> So end-result is that the original framework still exists and is the same.
>> The part that was broken for ResourceTracker has been fixed using the
>> refactored 'initializeResource' along with the 'isConnectable' method.
>>
>> Thoughts?
>
> Looks good,
>
> A few nits,
>
> diff --git a/netx-dist-tests-whitelist b/netx-dist-tests-whitelist
> --- a/netx-dist-tests-whitelist
> +++ b/netx-dist-tests-whitelist
> @@ -1,1 +1,1 @@
> -.*
> +JSToJSet.*
>
> I don't think you meant this to be changed.
>
> +    private void initializeOfflineResource() {
> +        CacheEntry entry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
> +        entry.lock();
> +
> +        try {
> +            File localFile = CacheUtil.getCacheFile(resource.getLocation(), resource.getDownloadVersion());
> +
> +            if (localFile != null && localFile.exists()) {
> +                long size = localFile.length();
> +
> +                synchronized (resource) {
> +                    resource.setLocalFile(localFile);
> +                    // resource.connection = connection;
> +                    resource.setSize(size);
> +                    resource.changeStatus(EnumSet.of(PREDOWNLOAD, DOWNLOADING), EnumSet.of(DOWNLOADED));
>
> Since this is a refactor can we get rid of // resource.connection = connection ? or is there a reason to keep this ?
>
> +                synchronized (lock) {
> +                    lock.notifyAll(); // wake up wait's to check for completion
> +                }
> +                resource.fireDownloadEvent(); // fire CONNECTED
> +
> +            } else {
> +                OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "You are trying to get resource " + resource.getLocation().toExternalForm() + " but it is not in cache and could not be downloaded. Attempting to +continue, but you may expect failure");
> +
> +                resource.changeStatus(EnumSet.noneOf(Resource.Status.class), EnumSet.of(ERROR));
> +                synchronized (lock) {
> +                    lock.notifyAll(); // wake up wait's to check for completion
> +                }
> +                resource.fireDownloadEvent(); // fire ERROR
> +            }
>
> synchronized(lock) ... to .. resource.fireDownloadEvent() is duplicated in the if and else, Could you move that out of the if statement to reduce the code duplication.
>
> Other than that the refactor looks good to me :) !
>
Thak you for review, both poatch and review rally looks good.

I have only one solid concern - always you wrote COPYPASTED you meant MOVED, right?

If there is anything really copypasted, please change it to calls to methods.


Also I have one idea:

Although the static impl of isConnectable may remain where it is, it would be nice to have shortcut 
to it via

resource.isConnectable() (as url is hiddne in resource)


hmm?