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
