[rfc][icedtea-web] fixing CacheReproducerTest and improving VersionedJarTest

[Jiri Vanek]

snip
>
> +    private final PropertiesFile recentlyUsedPropertiesFile;
> +    private final File cacheDir;
> +    private final File recentlyUsedFile;
>
> +    /**
> +     * @return the recentlyUsedFile
> +     */
> +    public File getRecentlyUsedFile() {
> +        return recentlyUsedFile;
> +    }
>
> Instead of keeping recentlyUsedFile in memory, can you change it to:
>
> +    public File getRecentlyUsedFile() {
> +        return recentlyUsedPropertiesFile.getStoreFile();
> +    }
>
> Then you can remove that variable.

As    you wish, done.
>
>
> -        if (cacheOrder.containsKey(key)) {
> +        if (getRecentlyUsedPropertiesFile().containsKey(key)) {
>
> AFAICT this code is inside CacheLRUWrapper so it should have access to 'recentlyUsedPropertiesFile'. It shouldn't use the getter if it doesn't need to, no?
>
> A lot of the code changes are like this. Can you change them all back?

With the getter you wished no. And I vote for keeping the getter rather
>
>
>
>
> Also:
>
> +    public CacheLRUWrapper() {
> +     this(PathsAndFiles.RECENTLY_USED_FILE.getFile());
> +    }
>
> In terms of the constructor, the dependencies are opposite.
>
> Right now, you get recently_used file, and then get cacheDir off it.
>
> Instead, it should be get cacheDir, and then get recently_used file off it.
>
> E.g.
>
> RecentlyUsedFile is always inside the Cache.  (this is correct)
>
> as opposed to
>
> Cache is always parent file of RecentlyUsedFile. (In most cases this ends up being the same thing, but it's not as correct)

Right. I removed misleading single-file constructor.
>
>
>
> The default constructor usage ends up breaking things when you use it in CacheUtil.
>
>      private static final String setCacheDir = JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
>      private static final String cacheDir = new File(setCacheDir != null ? setCacheDir : System.getProperty("java.io.tmpdir")).getPath(); // Do this with file to standardize it.
>      private static final CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
>
>   You need to also add changes to CacheUtil to use lruHandler's cache directory.
>
>   At the moment, CacheUtil.cacheDir != lruHandler.getCacheDir() when someone sets DeploymentConfiguration.KEY_USER_CACHE_DIR to non-default.

Yes. Fixed.
>
>
> This patch is broken here and needs to be fixed.
>
>
> Why would you push this without a +1 from anyone? You also seem to be doing this with a number of other patches recently. If you need a review, ping someone.

:( Rush is an eneymy. I need to calm bit down with all those failures...


See the patch for this commit.
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: removeDuplicateDefinitionsInCacheUtilsAndUseDefaultsFromLruWrapper.patch
Type: text/x-patch
Size: 6806 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150304/f91fe6db/removeDuplicateDefinitionsInCacheUtilsAndUseDefaultsFromLruWrapper.patch>