/hg/icedtea-web: Fixed "could not clear cache" message and cache...

Andrew Azores aazores at redhat.com
Thu Sep 12 07:13:12 PDT 2013


On 09/09/2013 11:16 AM, Jiri Vanek wrote:
> ..snip..
>> diff --git 
>> a/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java 
>> b/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java 
>>
>> --- 
>> a/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java
>> +++ 
>> b/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java
>> @@ -51,6 +51,7 @@
>>   import net.sourceforge.jnlp.ProcessResult;
>>   import net.sourceforge.jnlp.annotations.KnownToFail;
>>   import net.sourceforge.jnlp.config.Defaults;
>> +import net.sourceforge.jnlp.tools.MessageProperties;
>>   import org.junit.AfterClass;
>>   import org.junit.Assert;
>>
>> @@ -77,17 +78,6 @@
>>               "locks" + File.separator +
>>               "netx_running");
>>
>> -    private static final String messageResourcePath = 
>> "net/sourceforge/jnlp/resources/Messages.properties";
>> -    private static PropertyResourceBundle messageBundle = null;
>> -
>> -    static {
>> -        try {
>> -            messageBundle =
>> -                new 
>> PropertyResourceBundle(CacheReproducerTest.class.getClassLoader().getResourceAsStream(messageResourcePath));
>> -        } catch (IOException e) {
>> -        }
>> -    }
>> -
>>       String testS = "#netx file\n"
>>                  + "#Mon Dec 12 16:20:46 CET 2011\n"
>>                  + 
>> "1323703236508,0=/home/xp13/.icedtea/cache/0/http/localhost/ReadPropertiesBySignedHack.jnlp\n"
>> @@ -284,7 +274,7 @@
>>           Thread.sleep(1000);
>>           pr = tryToClearcache();
>>
>> -        String cacheClearError = 
>> messageBundle.getString("CCannotClearCache");
>
> This is incorrect. The message do not need to be en. Although most of 
> us have english locales, the framework should solve this.

As per Omair's suggestion (thanks! Didn't realize how much functionality 
the resource bundle classes actually provided :) ), this is taken care 
of quite neatly now.

>
>> +        String cacheClearError = 
>> MessageProperties.getMessage_en("CCannotClearCache");
>>           Assert.assertTrue("Stderr should contain " + 
>> cacheClearError + ", but did not.", 
>> pr.stderr.contains(cacheClearError));
>>           assertCacheIsNotEmpty();
>>       }
>> @@ -325,7 +315,6 @@
>>       }
>>
>>       @Test
>> -    @KnownToFail
>>       public void testAlreadyLoadedCached11() throws Exception {
>>           testsBody(CR11, 1);
>>           testsBody(CR11, 2);
>> diff --git 
>> a/tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java 
>> b/tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java 
>>
>> new file mode 100644
>> --- /dev/null
>> +++ 
>> b/tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java
>> @@ -0,0 +1,79 @@
>> +package net.sourceforge.jnlp.tools;
>> +
>> +import java.util.PropertyResourceBundle;
>> +import java.io.IOException;
>> +
>> +public class MessageProperties {
>
> I like this enum!
>
>> +
>> +    public enum Locale {
>> +        en,
>> +        de,
>> +        cs,
>> +        pl
>> +    }
>> +
>> +    private static final String resourcePath = 
>> "net/sourceforge/jnlp/resources/Messages";
>> +    private static final String resourceExtension = ".properties";
>
> I would like see the loaded resources as singletons, and so avoid 
> repeated loading. I would say static final Map<Locale, 
> PropertyResourceBundle> can be enough.

Apparently the ResourceBundle internally handles caching, and with the 
cleaner way it's implemented now using the getBundle factory method, 
loading singleton instances shouldn't be an issue. That's what the docs 
say at least :)

>> +
>> +    /* eg if given locale "de", returns a PropertyResourceBundle using
>> +     * "net/sourceforge/jnlp/resources/Messages_de.properties".
>> +     * if given "en", the same but using 
>> "net/sourceforge/jnlp/resources/Messages.properties"
>> +     */
>> +    private static PropertyResourceBundle 
>> getMessageBundleFromLocale(Locale locale) throws IOException {
>> +        final ClassLoader cl = 
>> MessageProperties.class.getClassLoader();
>> +
>> +        final String localizationCode;
>> +        if (locale == Locale.en) {
>> +            localizationCode = "";
>> +        } else {
>> +            localizationCode = "_" + locale.toString();
>> +        }
>> +
>> +        final String path = resourcePath + localizationCode + 
>> resourceExtension;
>> +        return new 
>> PropertyResourceBundle(cl.getResourceAsStream(path));
>> +    }
>> +
>> +    /**
>> +     * Retrieve a localized message from resource file
>> +     * @param locale the localization of Messages.properties to search
>> +     * @param key
>> +     * @return the message corresponding to the given key from the 
>> specified localization
>> +     * @throws IOException if the specified Messages localization is 
>> unavailable
>> +     */
>
> The base method for this should be
>
>  public static String getMessage(String key) throws IOException
> which will determine the locale and the call
>  getMessage(Locale locale, String key) throws IOException {
>
> Also I think I would abandon the  getMessage_XXX as they are easily to 
> be used via your enum.
> But as you already wrote them.. :)
>
>
> And sorry to dig it - this class need unittest.. :(

Hm, how to unit test this properly... the tests in the patch here depend 
on each localization of Messages.properties to contain a specific 
hardcoded value for a specific hard coded key. That's how I'm checking 
that specifying the locale manually is working (for both existing and 
non-existent localizations). The "default locale" test is probably even 
flakier. What do you think? Would it be better to have the test 
implement some other way of reading the .properties files and compare 
the results?

Also the SupportedLanguages enum might look a little useless now but 
it's just there as a hint for which Locales you can actually use and 
expect to get properly localized messages back out. I think it's maybe 
worth keeping just for that, and it's not cluttering up the class too 
much IMO.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: MessageProperties.patch
Type: text/x-patch
Size: 9731 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130912/63e56783/MessageProperties.patch 


More information about the distro-pkg-dev mailing list