On 22/08/2014 19:46, Naoto Sato wrote:
Hello,
Please review the changes for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8038436
The proposed changes are located at:
http://cr.openjdk.java.net/~naoto/8038436/webrev.3/
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
"JREEN".
A minor comment on JREENLocaleDataMetaInfo.getSupportedLocaleString is
that it could be changed to return
resourceNameToLocales.getOrDefault(resourceName, "");
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 ;
static {
Map<String, String> map = new HashMap<>();
map.put(...);
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
with CLDRLocaleProviderAdapter.
-Alan.
