[rfc][icedtea-web] Cache viewer cleanup

[Jiri Vanek]

On 09/16/2013 02:17 AM, Jakob Wisor wrote:
> Hello there!
>
> I have checked and corrected the code's formatting, and followed Jiri's reasonable advice.
>
> Jiri Vanek wrote:
>> On 07/31/2013 01:47 AM, Jacob Wisor wrote:
>>> Hello,
>>>
>>> * Added closing of cache viewer by ESC key
>>> * Added proper dis-/enabling of cache viewer's buttons
>>> * Added busy mouse cursor indicator when populating the cache viewer
>>>
>>> Everything is done on purpose on the AWT thread as before. Although I am personally not in favor
>>> of anonymous classes, I have tried to stick to the prevalent coding style.
>>>
>>> Unfortunately, I was unable to test the cache viewer's behavior because I do not know how to put
>>> synthetic cache entries into the cache. Can anyone help me with that? What does a the cache look
>>> like when filled with resources? Is there any index file that keeps track of the cache?

As this is gui change only, it shouldbe ok without test.
However cache is set of jars and jnlp files saved in url-like-filestrucutre

eg http:\\my.domain\myApp.jnlp with myJAr in "." codebase will be saved like
cache/http/my/domain/myApp.jnlp
cache/http/my/domain/myJar.jnlp

and yes, there is index file in tom cache directory

I have some memorry that there are already tests for it - most simple case how to import jar into 
cache is to lunch javaws ;)

>>>
...,snip...
>> Those should be in finally block
>>> +        parent.getContentPane().setCursor(Cursor.getDefaultCursor());
>
> Done. Please check whether I have done it correctly or the way you have meant.
>
>>>       }

Thank you.
>>>       /**
>>> @@ -305,4 +363,4 @@
>>>               defaultFocusComponent.requestFocusInWindow();
>>>           }
>>>       }
>>> -}
>>> +}
>>> \ No newline at end of file
>>> diff --git a/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
>>> b/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
>>> --- a/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
>>> +++ b/netx/net/sourceforge/jnlp/controlpanel/CacheViewer.java
>>> @@ -22,7 +22,10 @@
>>>   import java.awt.Frame;
>>>   import java.awt.GridBagConstraints;
>>>   import java.awt.GridBagLayout;
>>> +import java.awt.KeyEventDispatcher;
>>> +import java.awt.KeyboardFocusManager;
>>>   import java.awt.Toolkit;
>>> +import java.awt.event.KeyEvent;
>>>   import java.awt.event.WindowAdapter;
>>>   import java.awt.event.WindowEvent;
>>> @@ -53,8 +56,9 @@
>>>        */
>>>       public CacheViewer(DeploymentConfiguration config) {
>>>           super((Frame) null, dialogTitle, true); // Don't need a parent.
>>> +        if ((this.config = config) == null)
>>> +            throw new IllegalArgumentException("config: " + config);
>>>           setIconImages(ImageResources.INSTANCE.getApplicationImages());

Please for head - split this to onemore line

+            this.config = config;
+        if (this.config  == null)
+            throw new IllegalArgumentException("config: " + config);

for possible backport as as you mentioned) please dont add this change of exception. 1.4 expect NPE 
later or some error.

>>> -        this.config
>>> +        });
>>> +
>>>           initialized = true;
>>>       }
>>> @@ -116,4 +145,4 @@
>>>       private void centerDialog() {
>>>           ScreenFinder.centerWindowsToCurrentScreen(this);
>>>       }
>>> -}
>>> +}
>>> \ No newline at end of file
>>
>> Please mention both Cache viewer cleanup and  CerViewr viewer cleanup in news.
>
> Will do but they are separate patches.
>
> Happy reviewing!

For _this_ patch, the NEWS line should be attached explicitly.
For CerViewr the separate patch is ok.


No more review needed. With above fixed, ok to head (and 1.4 if wonted)


J.