[rfc] [icedtea-web] make PAthsAndFilles to follow seted paths II

[Jie Kang]

----- Original Message -----
> On 03/27/2015 03:54 PM, Jiri Vanek wrote:
> > So this is part II
> >
> > It is moving remianing keys to paths and files.
> >
> > The cache, logs and so on are now correctly changeable.
> >
> > The setters into those fields, however, remained property based.
> >
> > Thinking about third part of this patch...  Probably add setter to those
> > setupable files will be
> > best...
> >
> > And if dangling pointr will be done properly, then it should work perfectly
> > without any hacking.
> >
> 
> little bit of update - removed unused imports.

Hello,

Nice patch, few nits below:


-        public final String fileName;
-        public final String pathStub;
-        public final String systemPathStub;
+        protected final String fileName;
+        private final String pathStub;
+        private final String systemPathStub;
         protected final String descriptionKey;
         private final Target[] target;

I'd prefer private final String with a public/protected getter over a protected variable. 

+        protected String clean(String s) {
             while (s.contains(File.separator + File.separator)) {
                 s = s.replace(File.separator + File.separator, File.separator);

When looking at this change to protected instead of private wouldn't it make more sense if it were in some Utility class instead? It doesn't seem very specific to this class.

-                    File netxRunningFile = new File(config.getProperty(DeploymentConfiguration.KEY_USER_NETX_RUNNING_FILE));
+                    File netxRunningFile =PathsAndFiles.MAIN_LOCK.getFile();

Spacing here near the '=' character

-        final String fileUrlString = config.getProperty(DeploymentConfiguration.KEY_USER_SECURITY_POLICY);
+        final String fileUrlString = PathsAndFiles.JAVA_POLICY.getFullPath();

When looking at this change, does the variable 'DeploymentConfiguration config' still get used? When looking at usages in head, with this change I think the 'config' variable should be removed as well. (change constructor, etc.)

This applies for the other places in this patch too.

diff -r 2c28c32f7180 netx/net/sourceforge/jnlp/security/KeyStores.java
--- a/netx/net/sourceforge/jnlp/security/KeyStores.java	Fri Mar 27 14:05:14 2015 +0100
+++ b/netx/net/sourceforge/jnlp/security/KeyStores.java	Mon Mar 30 16:00:47 2015 +0200

I like the changes here but I'd say the KeyStores stuff is quite important. Could you please add some unit tests (KeyStoresTest) to make sure this works as expected? Writing tests for the function

-    public static final String getKeyStoreLocation(Level level, Type type) {
+    public static final PathsAndFiles.InfrastructureFileDescriptor getKeyStoreLocation(Level level, Type type) {

should be pretty easy.

+    private String removeFileProtocol(String s) {
+        if (s.equals(null)){

For null checking it's (s == null), no?

s.equals(null) wouldn't work if s=null as there is no equals method for the null object.


The changes look quite nice!


Regards,

> >
> > J.
> >
> >  >
> >  > Hi!
> >  >
> >  > This is first of at least three pathces tro make it work
> >  > this one - i adding difference between getFile/Path and
> >  > getDefaultFile/DefaultPAth + adapting few
> >  > (the most important) PathsAndFiles
> >  > So this one is especially about concept. There rised issue, that
> >  > PathsandFiles become depnding on
> >  > JnlpRuntime.getConfig() - the result of it is later-lazy loading of
> >  > RECENTLY_USED_FILE.
> >  > Also note the behaviour of deployment.user.security.policy
> >  >
> >  > Second part of this work will be focused on adapting missing "adjustable
> >  > files"
> >  >  deployment.user.security.trusted.cacerts
> >  > deployment.user.security.trusted.certs
> >  > deployment.user.security.trusted.clientauthcerts
> >  > deployment.user.security.trusted.jssecacerts
> >  > deployment.user.security.trusted.jssecerts
> >  >
> >  > And ensure, that those are never ever used in itw - only the
> >  > PathsAndFiles
> >  >
> >  > ThirdPart will be then focused on being sure, no dangling insatnces are
> >  > around - taht everything
> >  > is using PAthsAndFiles directly, and so runtime change to any
> >  > InfrastructureFileDescriptor will >
> > take imidiate effect. (aka changing cache in runtime durng testing)
> >  >
> >  > But this this third part is far faraway...
> >  >
> >  > BAck to this patch:
> >  >  I let Defaults.java use CACHE_DIR.getDefaultFullPath() instead of
> >  >  CACHE_DIR.getFullPath().
> >  > Tahts actually very correct - because those are default values. Then I
> >  > have added possibility to
> >  > override getFullPath by get from properties + few small changes for docs
> >  > generation.
> >
> >  >  Thoughts?
> >
> >
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern