<i18n dev> [8]Request for review - 8008576: Calendar mismatch using Host LocaleProviderAdapter

[Naoto Sato]

Webrev is updated according to your suggestions:

http://cr.openjdk.java.net/~naoto/8008576/webrev.02/

Please see each comment embedded below:

On 3/12/13 1:44 AM, Masayoshi Okutsu wrote:
> 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.

No, Calendar.Builder works fine with the JRE LocaleProviderAdapter. 
Issue was JapaneseImperialCalendar could not be instantiated from a 
sun.* package. Now I use Builder for consistency.

> - 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?

Fixed.

>
> - 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)

Changed them to the public API, i.e., TimeZone.getDefault(), and removed 
the code that sets "sharedZone" field to "true". Kept the shared zone in 
the Calendar constructors intact, as they won't be leaked outside of 
Calendar.

>
> - 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.

Added the try-catch clause. Fall back to the default impl when the 
exception occurs.

>
> - 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
> replaceAll("_", "-").)

Applied your suggestions to the applicable sites. One question, though, 
is that why Calendar.getAvailableCalendarTypes() does not return 
aliases, as I could not eliminate all the replaceFirst("gregorian", 
"gregory") calls.

>
> - 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?

Fixed.

Naoto

>
> Thanks,
> Masayoshi
>
> 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())).
>>
>> Fixed.
>>
>>> - 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:
>>
>> http://cr.openjdk.java.net/~naoto/8008576/webrev.01/
>>
>> Naoto
>>
>>>
>>> Otherwise, the change looks good to me.
>>>
>>> Thanks,
>>> Masayoshi
>>>
>>> On 3/9/2013 8:45 AM, Naoto Sato wrote:
>>>> Hello,
>>>>
>>>> Please review the changes for the following CR:
>>>>
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008576
>>>>
>>>> Here is the webrev for the changes:
>>>>
>>>> http://cr.openjdk.java.net/~naoto/8008576/webrev.00/
>>>>
>>>> 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.
>>>>
>>>> Naoto
>>>
>>
>