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
