<i18n dev> Request for review - 8008576: Calendar mismatch using Host LocaleProviderAdapter
masayoshi.okutsu at oracle.com
Tue Mar 12 01:44:49 PDT 2013
Here are my comments on the revised webrev, including a couple of
oversights in my previous review.
- It's OK to use the existing code path of Calendar for JRE. I thought
there might be some different reasons that Calendar.Builder wouldn't
work for JRE.
- JRELocaleProviderAdapter.getCalendarProvider() should use the
"CalendarData" language tag set.
- Should CalendarProviderImpl.isSupportedLocale always return true
because it supposed to be able to create a GregorianCalendar as fallback?
- When Calendar uses the default TimeZone, it uses a reference to the
default TimeZone rather than its clone. But instantiation of Calendar in
the locale service adapters means to leak the instance outside of a
Calendar. Could you remove the use of TimeZone.getDefaultRef in
Calendar. (OK to keep it in java.util.Date)
- CalendarProvider.getInstance may throw an IllegalArgumentException
which should be caught in Calendar.createCalendar. Note that
Calendar.Builder.setCalendarType throws an IAE if any invalid calendar
type is given.
- Calendar.Builder.setCalendarType() accepts "gregorian" as an alias of
"gregory". It's not necessary to replace "gregorian" with "gregory" in
HostLocaleProviderAdapterImpl (macosx). (String.replaceFirst is
expensive. In that sense, replace('_', '-') is cheaper than
- I know we can't change the Mac OS X documentation, but the method name
can be changed. How about convertMacOSLocaleToJavaLocale(String macos)
or something else which doesn't contain PosixLocale?
On 3/12/2013 8:04 AM, Naoto Sato wrote:
> Thank you for the review. Please find my comments below:
> On 3/11/13 12:47 AM, Masayoshi Okutsu wrote:
>> Here are my review comments.
>> - The Calendar.createCalendar comment says, "but it's not possible since
>> JapaneseImperialCalendar is package private." If Calendar.Builder
>> doesn't work, should the reason be different? Otherwise, the host locale
>> adapters won't be able to create a JapaneseImperialCalendar, either.
> The code intended to use the same code path for JRE as before, but on
> second thought, there is no reason not to use Calendar.Builder() even
> for JRE. Modified the fix as such.
>> - When building a Calendar with Calendar.Builder, the current time needs
>> to be set (.setInstant(System.currentMillis())).
>> - The usage of term "POSIX locale" in Mac OS sounds strange. POSIX
>> locale means C locale...
> Right. But that's not under our control :-)
> Revised webrev is located at:
>> Otherwise, the change looks good to me.
>> On 3/9/2013 8:45 AM, Naoto Sato wrote:
>>> Please review the changes for the following CR:
>>> Here is the webrev for the changes:
>>> The gist of the changes is to instantiate a calendar instance using
>>> the new Calendar.Builder class that suits the underlying OS's calendar
>>> for the current user's locale.
More information about the i18n-dev