<i18n dev> [9] RFR 8038436: Re-examine the mechanism to determine available localedata and cldrdata

[Naoto Sato]

Thank you Mandy for a quick review. Please see my comments below.

On 8/22/14, 2:26 PM, Mandy Chung wrote:
> On 8/22/14 11:46 AM, Naoto Sato wrote:
>>
>> http://cr.openjdk.java.net/~naoto/8038436/webrev.3/
>>
> I skimmed on the patch and have a few initial comment/questions.
>
> JREENLocaleDataMetaInfo
> JRENonENLocaleDataMetaInfo
>     - are the lists of locale names generated previously?

Yes.

>     The long lines need to be broken up.  Perhaps defining them
>     as string constants and grouped them per language (or some
>     fashion) would make it easier to read and maintain.
>
>     It would be good to define string constants for the en-* locale
>     names that are shared by FormatData, CurrencyNames, AvailbleLocales.
>
>     The @since 1.6 is not updated and not needed.

Will modify the string constants.

>
>     I wonder ifavailableLanguageTags andgetSupportedLocaleString
>     should return a list or an array of String (see comment below).
>
>
> There are two provider implementations for
> sun.util.locale.provider.LocaleDataMetaInfo and two service config
> files as you want to put them in cldrdata.jar and localedata.jar.
> I wonder if you want to merge them into a single service config
> for now until you decide to separate them into separate modules
> in the future.
>
> You could simply put it under
> src/jdk.localedata/share/META-INF/services/sun.util.locale.provider.LocaleDataMetaInfo

I tried to merge the config file, but it seems not possible to me to 
have two implementation definitions in a single file, e.g., if the 
config file has two entries

sun.util.locale.provider.JRENonENLocaleDataMetaInfo
sun.util.cldr.CLDRLocaleDataMetaInfo

and localedata.jar/cldrdata.jar have one on those each, ServiceLoader 
lookup failed with the exception (sorry forgot the actual name) saying, 
e.g., CLDRLocaleDataMetaInfo cannot be instantiated from localedata.jar. 
Thus I had to have two service config files.

>
> sun/util/cldr/CLDRLocaleProviderAdapter.java
>     line 67: why do you silently ignore any exception? In that
>     case, it will throw UOE in line 71.  Is UOE caught somewhere?
>     Assume there is a good reason to ignore the exception,
>     can you add comment to explain it?

UOE is caught in JRELocaleProviderAdapter. Will add some more comments 
there.

>
> sun/util/locale/provider/JRELocaleProviderAdapter.java
>     line 391: what exceptions do you expect to be caught here?
>     Do you have a test case for this to demonstrate why you
>     have to ignore the exception?

The one we have been discussing: AccessClassInPackage security exception 
for sun.util.locale.provider classes from localedata.jar/cldrdata.jar. 
Anyway, I followed the ResourceBundle.loadBundle()'s behavior here, 
where it ignores any Exception instances.

>
>     LocaleDataMetaInfo.availableLanguageTags returns a string
>     of space-separated names.  Would it be better to return
>     a List<String> or String[]

I think having a single string is preferred as to the performance, 
especially in CLDR's case there are hundreds of locales involved.

>
> When you have an image build, it'd be useful to test without
> cldrdata.jar and localedata.jar from the extension directory
> and run the tests to use the default EN locale.

Although I don't have any regression tests, I manually tested such 
situations and confirmed it worked correctly.

Naoto