[RFC][icedtea-web] Fix PR955: regression: SweetHome3D fails to run

[Danesh Dadachanji]

On 02/08/12 09:57 AM, Jiri Vanek wrote:
> On 08/01/2012 05:13 PM, Danesh Dadachanji wrote:
>> Hello,
>>
>> Here is a fix for PR955. [1]
>>
>> This was quite a tricky patch to figure out. Proprietary webstart handles locales.. interestingly.. I've made ours use a fallback
>> mechanism to find any information in the information tag. For each item it searches for (e.g. the title), it will try to find an
>> information tag which has the closest possible locale that looks like your JVM's locale. Please see the unit tests for examples.
>>
>> I have changed a few other things too with how we setup locales, the variant was not working properly. It was assumed to be 2
>> characters long which is definitely not the case. My locale is en_CA.utf8 <-- variant has 4 chars.
>>
>> My changes to the localeMatches method cover both the implementation I needed (being able to match multiple a locale by a given
>> precision - the enum), as well as making it more easily understood. I hope the switch cases do this, it was a bit of a brain teaser
>> figuring out those if statements and double negatives. :/
>>
>> All other changes are in the ChangeLog below:
>> +2012-08-01 Danesh Dadachanji <ddadacha at redhat.com>
>> +
>> + Fix PR955: regression: SweetHome3D fails to run
>> + * NEWS: Added entry for PR955
>> + * netx/net/sourceforge/jnlp/JNLPFile.java: New enum Match that represents
>> + the level of precision to use when matching locales.
>> + (localMatches): Renamed to localeMatches, added matchLevel paramater
>> + and updated conditionals to handle the level of precision specified by it.
>> + (getVendor): New method that returns an information's vendor text.
>> + (getInformation): Added override methods for getTitle and getVendor
>> + that are used by the anonymous class to filter by locale. All three
>> + methods now go through all levels of precision to search for the best
>> + fitted locale.
>> + (getResources), (getResourcesDescs): Updated to check if any level of
>> + precision matches when searching for locales.
>> + (parse): Added call to checkForTitleVendor.
>> + * netx/net/sourceforge/jnlp/Parser.java
>> + (checkForTitleVendor): New method to check for availability of localized
>> + title and vendor from the information tags. Throws ParseException.
>> + (getInfo): Replace loop with foreach loop.
>> + (getInformationDesc): Remove check for present title and vendor.
>> + (getLocale): Variant returned can now use everything after the eigth
>> + element of the locale's string.
>> + * netx/net/sourceforge/jnlp/resources/Messages.properties:
>> + Update missing title and vendor messages to mention localization.
>> + * tests/reproducers/simple/InformationTitleVendorParser/testcases/InformationTitleVendorParserTest.java:
>> + Update output string as per new changes to Messages internationalizations.
>> + * tests/netx/unit/net/sourceforge/jnlp/JNLPFileTest.java:
>> + New unit test that checks the localesMatches method in JNLPFile.
>> + * tests/netx/unit/net/sourceforge/jnlp/MockJNLPFile.java:
>> + New class used to create a mock JNLPFile object.
>> + * tests/netx/unit/net/sourceforge/jnlp/ParserTest.java:
>> + New unit test that checks that the return of getTitle and getVendor
>> + have localized information.
>>
>> I would like to push this to HEAD and backport it to 1.3. The regression isn't present in 1.2 and before. Any thoughts?
>>
>> Regards,
>> Danesh
>> [1] http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=955
>
> Hi! Nice work, I have just few really minor stuff for patch itself and several more for tests.
>
> in JNLPFile there are iner classes ( you have touched them) InformationDesc and ResourcesDesc which are overriding methods but are
> missing annotations. Can you please add those notations? ( I have counted 5 of them)

Ah yes, good catch, done.

> You are throwing ParseException  if venodr/title is not found, and later in tests you are testing result against internationalize-able
> text. What do you think about more modular  exception chain?
> eg ParseException -> RequiredElementException --> (MissingTitleException + MissingVendorException) and then in tests check just for
> those exceptions without real text?

Amazing idea, this also makes the unit tests cleaner! I added a third, MissingInformationException is needed as well.

> In Message.properties you are writing
> "The title/vendor section has not been defined for your locale in the JNLP file." which looks wrong to me or I understand badly your
> code. Your code will fallback to the not-localised  if no locale (ab_CD.efg -> ab_CD -> ab -> defualt) or not?

You are correct, I added that change before I fully understood the fallbacks, changing it now.

> This should be a bit reflecting in error message. eg "The title/vendor section has not been defined for your locale nor is existing its
> default value in yourJNLP file."
> You can also save duplicate entries in properties by something like :
>
> PVendor=vendor
> PTitle=title
> PNoVendorTitleElement=The {1} section has not been defined for your locale in the JNLP file.

Neat, I did not know about this. I think it's {0} though for param1.

>
> and
>
> throw new SomeBetterThenParseException(R("PNoVendorTitleElement",R("PTitle"/"PVendor")));

Done.

>
> but as you wish....
>
>
>
> Tests:
> MockJNLPFile is in tests directory but is not a test. So test runner will fail on it. This is not what is wanted. To prevent this yo
> can make this class as inner or move to test-extensions. because this file is already shared I'm for second approach. lets move it eg
> to test-extensions/net.sourceforge.jnlp.mocks

Great idea, done.

> Unittests will probably also deserve tests with new Locale("X") and new Locale("x","y") (you are now just testing new Locale("x","y","z");

Okay, I duplicated all the tests to use Locale("EN") and to use Locale("EN", "CA"). The tests are the almost identical, I had to swap a 
few expected failures around. (e.g. Locale("EN", "CA") should fail if the info tag had "en_CA.utf8"). Apart from this, they're pretty 
much the same tests copy pasted. =)

> Reproducer test - reproducer is missing:(( I tought that you will include two or three (both failing and passing) with the Locales
> hack. I have posted repreoducer (without hack!) some time ago, it is not in. Feel free to ruse. But imho reproducer is still worthy.
>
>

I thought the unit tests + the general info/title/vendor reproducer would be enough so I didn't try to write one earlier. I gave 
writing one a shot but did not get very far. I'd like to finish off the tests but I just don't have enough cycles left to finish them 
off. Sorry! Please feel free to take over the rest of the patch.

I've attached the latest patch here, hopefully you or someone else can pick it up. Thanks very much for all the reviews!

Regards,
Danesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: title-vendor-required-07.patch
Type: text/x-patch
Size: 99130 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120807/dbd66dc6/title-vendor-required-07.patch