[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-i18n-dev
Subject:    Re: <i18n dev> [8]Request for review - 8008576: Calendar mismatch using Host LocaleProviderAdapter
From:       Masayoshi Okutsu <masayoshi.okutsu () oracle ! com>
Date:       2013-03-14 8:06:48
Message-ID: 51418518.2010509 () oracle ! com
[Download RAW message or body]

Looks good to me.

Thanks,
Masayoshi

On 3/13/2013 7:26 AM, Naoto Sato wrote:
> 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
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]