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

[Masayoshi Okutsu]

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

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

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