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

[Jie Kang]

----- Original Message -----
> On 03/30/2015 02:52 PM, Jiri Vanek wrote:
> > On 03/27/2015 09:01 PM, Jie Kang wrote:
> >>
> >>
> >> ----- Original Message -----
> >>> 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?
> >>
> >> Hello,
> >>
> >> Nice! Review below:
> >>
> >>
> >> +     this(PathsAndFiles.getRECENTLY_USED_FILE().getFile(),
> >> PathsAndFiles.CACHE_DIR.getFile());
> >>
> >> Can you explain a little more on why there is a getter for recently used
> >> file. Where is the later-lazy loading and what problem is occurring that
> >> makes it so you need to have a getter?
> >
> > It is using  PathsAndFiles.CACHE_DIR.getFile() for its creation.
> > It is based on property value
> > So in this moment the jnlpconfig have to exists, that means that
> > deployment.properties must be read.
> > So laxy loading menas as late as possible. Loading it in static phase, will
> > create pretty nice initialization error.
> >>
> >> Also please don't capitalize words in the function:
> >> s/getRECENTLY_USED_FILE/getRecentlyUsedFile
> >>
> >>
> > will be done
> >> +            return gcpd(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> >>
> >> Please give the function gcpd(...) a descriptive name that gives me an
> >> idea of what it's doing without having to look at the function's
> >> implementation.
> >
> > Itis just shortuct method getConfgiPropertyDescriptor.
> >
> > I think it will be removed in future iterations of this. I'm now working on
> > PArt three, and it is already gone...
> >
> >>
> >> +        //its not recomanded to override default locations methods
> >>
> >> s/recomanded/recommended
> >
> > will be done.
> >>
> >>
> >> +        public String getDefaultFullPath() {
> >>               return clean(systemPathStub + File.separator + pathStub +
> >>               File.separator + fileName);
> >>
> >> For the function clean(...), can you please either rename it to be more
> >> descriptive or add comments to the function. Just seeing the function
> >> name I have very little clue as to what it does.
> >
> > Javadoc will be added.
> >>
> >> +        private String getStub() {
> >>
> >> Can you add comments to this? I don't know what you mean by 'stub'.
> >
> > I can't? :)
> >
> > Sure. I will write javadoc.  It is method, which is providing the nice
> > variable-like outputs to docs.
> >>
> >> Other parts look okay, but please make sure you test this thoroughly. Are
> >> there tests for this?
> >>
> >
> > Hard to test in this time. I did as throughlky as I could. I think it is
> > good approach to follow.
> > With part two done (already on review) it is much more testable, and seems
> > to be working fine.
> 
> I will add much more tests during parts II-IV
> >
> >
> > Thanx for check!
> >
> 
> Here you go. Jsut moinors as you insisted.

Hi,

Thanks!

There is one thing I just noticed:

     public static Map<String, Setting<String>> getDefaults() {
         SecurityManager sm = System.getSecurityManager();
         if (sm != null) {
+            sm.checkRead(USER_DEPLOYMENT_FILE.getDefaultFullPath());
             sm.checkRead(USER_DEPLOYMENT_FILE.getFullPath());
         }

Are you sure this is correct to check both default filepath and possibly non-default filepath? What happens when default doesn't exist, or when non-default doesn't exist? To me it should just check whichever actually is being used, not both.

Everything else looks fine.


Regards,

> 
> Ok for head?
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern