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

[Jiri Vanek]

On 09/12/2013 04:13 PM, Andrew Azores wrote:
> 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.
>

I'm happy with it as it is.

Thank you.