On 8/28/14, 5:11 PM, Mandy Chung wrote:
Naoto,
This looks better. Thanks for the update.
The getSupportedLocaleString method in both EnLocaleDataMetaInfo and
NonEnLocaleDataMetaInfo has the javadoc that missing the description.
sun.util.locale.provider is a package in java.base and
NonEnLocaleDataMetaInfo has to be in a different package as it's in the
jdk.localedata
Previously the list of locales was generated at build time.
NonEnLocaleDataMetaInfo now hardcodes the list in the source. How often
do we need to update this list? Just wonder whether there is a better
way to maintain this list and catch regression. You have a new test
Bug8038436.java to compare the golden list which is good. However, the
golden list is hardcoded too that seems to be error-prone. Does
Bug8038436.java pass with jdk9?
JRE's locale data will be just in maintenance mode as CLDR will become
the defualt, so it won't change much, and the test case does pass with jdk9.
Having said that, I just reverted to the dynamic list generation at the
build time. Just leave the test case though.
I will address your other suggestions in the next revision.
Naoto
The current source layout implies the jdk.localedata module contains
both cldr and localedata while they are generated as two separate JAR
files in the jdk image. Would you propose to split CLDR out as
jdk.cldrdata? Currently you have two service config files providing the
same service type and leads to the awkward location.
If they should be kept under jdk.localedata, we could live with the two
service config files until the modular image is in place.
I would suggest renaming
src/jdk.localedata/share/classes/sun/util/cldr/sun.util.locale.provider.LocaleDataMetaInfo
->
src/jdk.localedata/META-INF/cldrdata-services/sun.util.locale.provider.LocaleDataMetaInfo
src/jdk.localedata/share/classes/sun/util/locale/provider/META-INF/services/sun.util.locale.provider.LocaleDataMetaInfo
->
src/jdk.localedata/META-INF/localedata-services/sun.util.locale.provider.LocaleDataMetaInfo
Mandy
On 8/28/14 11:34 AM, Naoto Sato wrote:
Thank you, Mandy, Masayoshi, and Alan for your comments. I revised the
changes based on your suggestions as follows:
http://cr.openjdk.java.net/~naoto/8038436/webrev.4/
Here are the changes since webrev.3
- CLDRLocaleProviderAdapter.java: modified to throw
UnsupportedOperationException with the actual exception set to its
"cause." More descriptive comment on it.
- *ProviderImpl.java: Removed changes to them. Initial thought was to
make them performant by changing the langtags to static, but it turned
out that wasn't necessary.
- JREENLocaleDataMetaInfo.java/JRENonENLocaleDataMetaInfo.java:
Renamed to EnLocaleDataMetaInfo and NonEnLocaleDataMetaInfo
respectively for readability. String constants are wrapped for
readability as well. Used getOrDefault() for getSupportedLocaleString().
- Added test cases for SecurityException and JRE's supported locales
for each service.
I'd appreciate it if someone in the build-dev could review the
makefile changes.
Naoto
On 8/22/14, 11:46 AM, 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.
Naoto
