[rfc] [icedtea-web] make PAthsAndFilles to follow seted paths IV - bubble to CacheLruWrapper
Jie Kang
jkang at redhat.com
Wed Apr 8 19:02:20 UTC 2015
Hello,
Good work!
Nits below:
For this patch, the class DeploymentPropetiesModifier needs to be fixed to use the new InfrastructureFileDescriptor. It currently doesn't compile. Also if you could: s/Propeties/Properties
diff -r 85505d8c9f3c tests/netx/unit/net/sourceforge/jnlp/controlpanel/CommandLineTest.java
--- a/tests/netx/unit/net/sourceforge/jnlp/controlpanel/CommandLineTest.java Thu Apr 02 21:28:10 2015 +0200
+++ b/tests/netx/unit/net/sourceforge/jnlp/controlpanel/CommandLineTest.java Thu Apr 02 21:57:28 2015 +0200
@@ -30,6 +30,7 @@
import net.sourceforge.jnlp.OptionsDefinitions;
import net.sourceforge.jnlp.config.PathsAndFiles;
+import net.sourceforge.jnlp.util.logging.NoStdOutErrTest;
import net.sourceforge.jnlp.util.logging.OutputController;
import net.sourceforge.jnlp.util.optionparser.OptionParser;
import net.sourceforge.jnlp.util.optionparser.UnevenParameterException;
@@ -37,7 +38,7 @@
import org.junit.BeforeClass;
import org.junit.Test;
-public class CommandLineTest {
+public class CommandLineTest extends NoStdOutErrTest{
public static final int ERROR = 1;
public static final int SUCCESS = 0;
@@ -91,7 +92,7 @@
ByteArrayOutputStream outStream = getOutputControllerStream();
String[] args = {
- "set", "unknown", "ALLOW_UNSIGNED"
+ "set", "unknown", "does_not_metter"
s/metter/matter/
};
OptionParser optionParser = new OptionParser(args, OptionsDefinitions.getItwsettingsCommands());
CommandLine commandLine = new CommandLine(optionParser);
@@ -150,7 +151,7 @@
@Test
public void testSetPropertyWithIncorrectValue() throws IOException {
String[] args = {
- "set", "deployment.security.level", "ALLOW_ONLY_SAFE_APPLETS"
+ "set", "deployment.security.level", "INTENTIONALLY_INCORRECT"
};
Is this changeset for CommandTest at all related to your patch? If not, this should be a separate patch.
diff -r 85505d8c9f3c tests/netx/unit/net/sourceforge/jnlp/security/KeyStoresTest.java
--- a/tests/netx/unit/net/sourceforge/jnlp/security/KeyStoresTest.java Thu Apr 02 21:28:10 2015 +0200
+++ b/tests/netx/unit/net/sourceforge/jnlp/security/KeyStoresTest.java Thu Apr 02 21:57:28 2015 +0200
@@ -37,6 +37,7 @@
package net.sourceforge.jnlp.security;
import java.security.Permission;
+import net.sourceforge.jnlp.config.InfrastructureFileDescriptor;
import net.sourceforge.jnlp.config.PathsAndFiles;
import org.junit.AfterClass;
import org.junit.Assert;
@@ -62,7 +63,7 @@
//TODO once setConfig is removed, ensure SM is enforced also from PathsAndFiles
Has this TODO been completed? Or is it still valid?
+ //he underlying InfrastructureFileDescriptor is still poinitng to the same file, use current proeprties file
s/he/The
s/poinitng/pointing
s/proeprties/properties
+ //the InfrastructureFileDescriptor was set to different location, move to it
+ //is there something needed to close previous instance?
I wouldn't put a question in the comment unless there is a TODO or FIXME. Otherwise it's confusing to other developers (should they be looking at fixing this or not?)
I don't think anything else is needed. Saving the properties to file using store() is nice to do, but I wouldn't say it's necessary.
Also, the word 'move' isn't too correct as nothing is being moved. The variable is just pointing to a new location. s/move to it/use the new location
+ cachedPropertiesFile.store();
+ cachedPropertiesFile.unlock();
In order to store you must have the lock. So you'd need to lock() or tryLock() before calling store().
Also, the variable cachedPropertiesFile refers to the RecentlyUsed file that is of type PropertiesFile. I'd rename the variable to cachedRecentlyUsedFile, or something like that.
+ //above were just shotted here
I don't understand this comment. Can you reword it?
+ * @return true if proeprtieswere sucesfullty stored, fasle otherwise
s/proeprtieswere/properties were
s/sucesfullty/successfully
s/fasle/false
+ PropertiesFile props = getRecentlyUsedPropertiesFile();
+ if (!props.containsKey(oldKey)) return false;
Please don't use one line if statements.
+ rStr =lruHandler.getCacheDir().getFullPath()+ rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
Please fix the spacing near the '=' character as well.
+ //simple constructor to allow testing instances base don overrides
s/base don/based on
+ //simple constructor to allow testing instances base don overrides
+ protected InfrastructureFileDescriptor() {
+ this("nope", "nope", "nope", "nope");
+ }
I'd use empty string "" instead of "nope". The "nope" string doesn't look as professional IMO.
+ final private DeploymentConfiguration config;
s/final private/private final
+ @Override
+ public String getFullPath() {
+ return backend.getAbsolutePath();
+ }
+
+
+
+
+
Please remove the extra lines.
Thanks for the patch!
Regards,
----- Original Message -----
> On 04/02/2015 09:54 PM, Jiri Vanek wrote:
> > Adapted to head.
>
> forgot hg add
> >
> > However I have not checked removal of constructors properly... BUt Maybe I
> > missed none. TY!
> >
> > J.
> >
> >
> > On 04/01/2015 04:34 PM, Jiri Vanek wrote:
> >> Seems like I forgot an attachment...
> >>
> >> On 03/31/2015 03:51 PM, Jiri Vanek wrote:
> >>> There are/were/reappeared two issues in LRUwrapper and CacheUtils
> >>>
> >>> First the file - however crated was as value,not as pointer. So whatever
> >>> happened to original, had not bubbled to lruwrapper/utils.
> >>> Second - the methods were using getters on field, whcch could change
> >>> during the method invocation.
> >>>
> >>> This pathc should fix both of them.
> >>>
> >>> The main refactoring is
> >>> - public CacheLRUWrapper(final File recentlyUsed, final File cacheDir)
> >>> {
> >>> - recentlyUsedPropertiesFile = new PropertiesFile(recentlyUsed);
> >>> + public CacheLRUWrapper(final InfrastructureFileDescriptor
> >>> recentlyUsed, final InfrastructureFileDescriptor cacheDir) {
> >>> + recentlyUsedPropertiesFile = recentlyUsed;
> >>>
> >>> Now the cache lru wrapper is handling the "pointers" to sources. So any
> >>> call to its "getFiles" get real value fromm PathsAndFiles
> >>> When any method is using its getFile it alwasy save current copy, co
> >>> during runtime, it will not be affected by possible change.
> >>>
> >>> The exception is
> >>>
> >>> - public PropertiesFile getRecentlyUsedPropertiesFile() {
> >>> - return recentlyUsedPropertiesFile;
> >>> + PropertiesFile getRecentlyUsedPropertiesFile() {
> >>> + if (cachedPropertiesFile == null) {
> >>> + //no properties file yet, create it
> >>> + cachedPropertiesFile = new
> >>> PropertiesFile(recentlyUsedPropertiesFile.getFile());
> >>> + return cachedPropertiesFile;
> >>> + }
> >>> + if
> >>> (recentlyUsedPropertiesFile.getFile().equals(cachedPropertiesFile.getStoreFile())){
> >>> + //he underlying InfrastructureFileDescriptor is still
> >>> poinitng to the same file, use current proeprties file
> >>> + return cachedPropertiesFile;
> >>> + } else {
> >>> + //the InfrastructureFileDescriptor was set to different
> >>> location, move to it
> >>> + //is there something needed to close previous instance?
> >>> + cachedPropertiesFile.store();
> >>> + cachedPropertiesFile.unlock();
> >>> + //above were just shotted here
> >>> + cachedPropertiesFile = new
> >>> PropertiesFile(recentlyUsedPropertiesFile.getFile());
> >>> + return cachedPropertiesFile;
> >>> + }
> >>> +
> >>>
> >>>
> >>> When something change value path to properties file *and* call
> >>> getRecentlyUsedPropertiesFile then the file will store and unlock. (note
> >>> - I dont know for sure if those two are needed, but afaik yes). So
> >>> actions upo previous Prop.File may misbehave.
> >>> However .. The setter to the RECENTLY_USED_FILE and KEY_USER_CACHE_DIR is
> >>> used *only* in tests (and should never be used in ITW itself)
> >>> Afaik - whole those will work if you run javaws (keep running) change
> >>> proeprties, run second javaws. I noticed no issues. Everything was
> >>> correctly cached twice
> >>>
> >>>
> >>> There is one aditional refactoring in this patch - sorry for that -
> >>> InfrastructureFileDescriptor moved from inner class to outer class. With
> >>> one change -
> >>> + //simple constructor to allow testing instances base don overrides
> >>> + protected InfrastructureFileDescriptor() {
> >>> + this("nope", "nope", "nope", "nope");
> >>> + }
> >>>
> >>>
> >>> At the end, I consider this 4 pathces + lrucaches singleton +
> >>> PAthsAndFiles as reallly good thing which happend to itw.
> >>>
> >>>
> >>> Jie - Is this enough for your "[rfc][icedtea-web] Use temporary cache in
> >>> *" or is there something more to be done?
> >>>
> >>>
> >>> J.
> >>
> >
>
>
--
Jie Kang
OpenJDK Team - Software Engineering Intern
More information about the distro-pkg-dev
mailing list