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

[Jie Kang]

Hi,

Thanks!

Tiny nits below:

diff -r cbc450f7203f netx/net/sourceforge/jnlp/controlpanel/DebuggingPanel.java
--- a/netx/net/sourceforge/jnlp/controlpanel/DebuggingPanel.java	Wed Apr 01 18:39:20 2015 +0200
+++ b/netx/net/sourceforge/jnlp/controlpanel/DebuggingPanel.java	Wed Apr 01 19:37:03 2015 +0200
@@ -38,8 +38,8 @@

This class still contains DeploymentConfiguration instance but has changes to use PathsAndFiles instead. Please remove the DeploymentConfiguration variable too.


Apart from that, the KeyStores unit tests should be going in before this. If there are any changes to this patch please resubmit for quick review. Otherwise, if the unit tests all pass with this patch applied then this looks good to go.


Regards,

----- Original Message -----
> Here you go. All what you suggested fixed.
> See inline
> 
> On 04/01/2015 03:25 PM, Jie Kang wrote:
> >
> >
> > ----- 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.
> 
> as you wish,.
> >
> > +        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.
> 
> uf. nope? It is strictly targeted utility method. MAy grow or shrink with ,
> or even die or whatever with InfrastructureDescriptor
> 
> >
> > -                    File netxRunningFile = new
> > File(config.getProperty(DeploymentConfiguration.KEY_USER_NETX_RUNNING_FILE));
> > +                    File netxRunningFile
> > =PathsAndFiles.MAIN_LOCK.getFile();
> >
> > Spacing here near the '=' character
> 
> ok
> 
> >
> > -        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.
> 
> Yah. Interesting idea. I had applied it. It will need some more tuning after
> the setter is included in part III - maybe more of those will be removed...
> Now I like this changeset a bit more.... But In past it had no reason to
> supply the param, because it was static.... And there are no unittests for
> it (it would be sense to test those on false settings...)
> 
> Still - removed from all palces where it is currently valid and looks better
> :)
> 
> >
> > 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.
> >
> 
> To tired to do this today, but I will crate test as separate changeset and
> push /before/ this changeset itself.
> > +    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.
> 
> What I was somking, dont you know? :) fixed....
> >
> >
> > 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