On 28/08/2014 19:34, 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.
I looked through the updated webrev and it looks much better (and much
easier to review too).
I assume CLDRLocaleProviderAdapter.metaInfo should be final.
I'm still not sure about CLDRLocaleProviderAdapter throwing UOE when
iterating over the installed LocaleDataMetaInfo implementations fail. It
almost feels like it should be an InternalError as it could only mean
something wrong with the JDK code or something broken in the JDK
installation. I have the same concern in
JRELocaleProviderAdapter.createSupportedLocaleString where it would be
even more difficult to diagnose why non-English locales appear to be
missing.
Typo in JRELocaleProviderAdapter, installede -> installed.
I see Mandy has pointed out the location of the services configuration
files, it would be good to check into that.
Otherwise I think this looks reasonable, you've addressed the comments I
had in the original webrev.
-Alan.
