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

[Jiri Vanek]

On 08/08/2012 04:50 PM, Danesh Dadachanji wrote:
> On 08/08/12 08:11 AM, Jiri Vanek wrote:
>> On 08/07/2012 08:20 PM, Danesh Dadachanji wrote:
>>>
>>>
>>
>> I'm ok for head and in some time also for 1.3
>>
>
> Awesome, thanks very much for the review! I have one more minor update in the attached patch, I've
> added printing of homepage and description when checking for title/vendor if in debug mode. This
> will make regression tests a bit easier. =)
>
> Can I get another okay?

yy, go on.
>
> Thanks again for all the comments!
>
> Cheers,
> Danesh
>
>>> 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.
>>
>> sorry, sure.
>>>
>>>>
>>>> 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
>>