<i18n dev>  RFR 8038436: Re-examine the mechanism to determine available localedata and cldrdata
Alan.Bateman at oracle.com
Mon Aug 25 16:28:31 UTC 2014
On 22/08/2014 19:46, Naoto Sato wrote:
> Please review the changes for the following issue:
> The proposed changes are located at:
> The idea is to introduce an SPI so that supported locales are
> dynamically searched at runtime, not depending on the existence of
> physical jar files.
> I'd appreciate it if build folks could review the makefile related
> changes, i18n forks to review locale framework files, and Mandy from
> modularization point of view.
I've skimmed over the changes (not a detailed review, I see Mandy and
Masayoshi are doing that). It is good to see this code changed to use
ServiceLoader and dropping the direct access to localeddata.jar and
cldrdata.jar. It would be useful to see if you can run a few startup
performance tests to see that the changes have any impact.
On JREENLocaleDataMetaInfo then I wonder if you've considering using
camel case instead. Maybe "JRE" could be dropped (I realize the type is
"JRE") and it can be renamed to EnLocaleDataMetaInfo to avoid all caps
A minor comment on JREENLocaleDataMetaInfo.getSupportedLocaleString is
that it could be changed to return
For the initialization then this might be help with some of the long
lines and might make the publication a bit clearer too:
private static final Map<String, String> resourceNameToLocales ;
Map<String, String> map = new HashMap<>();
resourceNameToLocales = map;
On the long lines then JRENonENLocaleDataMetaInfo will need attention as
the 872 character line will make future side-by-side reviews fun :-)
In CLDRLocaleProviderAdapter's constructor then it is catches/ignores
exceptions, the subsequent UOE does not help diagnose any issues.
Hopefully this can be re-examined before these changes are pushed as any
issue here would be difficult to diagnose.
One thing that isn't clear to me is the getLangTags implementations in
several classes where it has to cast to JRELocaleProviderAdapter. It's
not clear to the reader why this is necessary and how it would behave
More information about the i18n-dev