Thank you for your review. My comments are embedded below:
On 10/23/12 2:53 PM, Masayoshi Okutsu wrote:
Here are my comments.
SPILocaleProviderAdapter.java:
- Should "." be used for composing a class name of a Delegate instead of
"$"?
Since those delegates are inner classes within SPILocaleProviderAdapter,
it has to be "$", otherwise the class cannot be found with
Class.forName() method.
- Is it necessary to iterate locale candidate in getImpl? (Iteration is
performed outside of adapters when getting an adapter or a provider of
pools.)
Yes. Delegates have to check lookup locales within themselves in order
to decide which implementation should be returned.
- addImpl() overrides any previous impl if there are multiple
implementations for the same Locale. Should the first one preserved?
There is no rule which implementation is used when there are multiple
implementations for a particular locale, so I think overriding is OK.
Since in JDK7, the logic used to use Set interface, there is no way to
keep compatibility.
- map.keySet().contains(key) should be map.containsKey(key).
Fixed.
- When a factory of a provider returns null where null is not allowed,
should the provider be removed from the map?
I think this should be fine, since they are just the "delegates."
Instead I added assertion to each getImpl() return value, which should
always be non null.
The modified webrev is located here. Only SPILocaleProviderAdapter.java
is modified this time.
http://cr.openjdk.java.net/~naoto/8000997/webrev.01/
Naoto
Thanks,
Masayoshi
On 2012/10/22 15:26, Naoto Sato wrote:
Hello,
I need a reviewer for the changes for 8000997, where the symptom is:
"With the locale data deployment feature, locale sensitive services
SPI implementations cannot be loaded more than one implementation.
This is due to the fact that SPILocaleProviderAdapter only uses one
instance of SPI implementations. It should load all the available
installed providers as in JDK7, and correctly selects the instance
depending on the requester's locale."
The gist of the fix is to prepare LocaleServiceProvider delegates for
each services in SPILocaleProviderAdapter, and they work with the real
SPI implementations. The proposed change is located here:
http://cr.openjdk.java.net/~naoto/8000997/webrev.00/
Naoto
