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

[Jiri Vanek]

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.


Thanx for check!

J.