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

[Lukasz Dracz]

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 :) !

Thank you,
Lukasz Dracz