RFR 8038436: Re-examine the mechanism to determine available localedata and cldrdata
naoto.sato at oracle.com
Fri Aug 22 22:37:49 UTC 2014
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:
> I skimmed on the patch and have a few initial comment/questions.
> - are the lists of locale names generated previously?
> 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
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
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.
> 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
> 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.
More information about the build-dev