<i18n dev> RFR: 8248695: HostLocaleProviderAdapterImpl provides invalid date-only

[naoto.sato at oracle.com]

Hi Joe,

On 7/13/20 7:28 PM, Joe Wang wrote:
> 
> 
> On 7/13/2020 7:01 PM, naoto.sato at oracle.com wrote:
>> Hi Joe,
>>
>> Thank you for your review.
>>
>> On 7/13/20 3:55 PM, Joe Wang wrote:
>>> Hi Naoto,
>>>
>>> Would it make sense to provide an additional test using the public 
>>> APIs similar to the one provided in the bug report? I'm sure yours is 
>>> correct and covers more cases than the original, but it would be nice 
>>> to have an actual use case and use the public APIs. The report showed 
>>> it was failed somewhere down the stream than when it is run against 
>>> the current build, which produces IAE "Too many pattern letters: a" 
>>> instead of what's reported.
>>
>> I am not quite sure I got your suggestion, but the test case uses the 
>> same API as in the bug report (LocaleProviders.java, line 421-428) 
>> that are supposed to catch two cases, i.e., 
>> ofLocalizedDate()/ofLocalizedTime() tests catch "unsupported temporal 
>> field" exception, such as HourOfDay, and ofLocalizedDateTime() catches 
>> "Too many pattern letters: aa" which you saw in (possibly) en_US 
>> locale. To make it clearer, I added some comments to it.
> 
> Yes, the test covered the cases. What I was suggesting was an additional 
> test that uses only public APIs similar to that in the bug report, that 
> is what users would do and how they may encounter this issue, e.g.
>      System.setProperty("java.locale.providers", "HOST");
>      DateTimeFormatter formatter = 
> DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL);
> 
> It's unlikely for an user application to refer to a private 
> package/class like sun.util.locale.provider.LocaleProviderAdapter.
> 
> I understand it's been that way for these test. But to me, a real world 
> use case is always nice to have. Your call whether you want to add it or 
> not. There's no missing coverage.

Now I got what you are talking about. The way the bug report is doing is 
in fact discouraged, as once it is read at the startup, it won't be read 
again with later setProperty() call. Apps are supposed to specify the 
property as -D launcher parameter. (And the test case is doing it, at 
line 184 of LocaleProvidersRun.java) Internal interface is used only to 
tell whether HOST provider is actually used or not, so apps should not 
depend on it.

> 
>>
>>>
>>> HostLocaleProviderAdapter_md:849 - 865: may be compacted into one if 
>>> statement, (bCal && getCalendarInfoWrapper(...) || 
>>> getLocaleInfoWrapper(...)).
>>
>> Quite right. Modified.
>>
>> The updated webrev is located at:
>>
>> https://cr.openjdk.java.net/~naoto/8248695/webrev.01/
> 
> Looks good to me.

Thanks!

Naoto

> 
> Best,
> Joe
> 
>>
>> Naoto
>>
>>>
>>> Regards,
>>> Joe
>>>
>>> On 7/13/2020 5:54 AM, naoto.sato at oracle.com wrote:
>>>> Ping.
>>>>
>>>> On 7/7/20 3:55 PM, naoto.sato at oracle.com wrote:
>>>>> Hello,
>>>>>
>>>>> Please review the fix to the following issue:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8248695
>>>>>
>>>>> The proposed changeset is located at:
>>>>>
>>>>> http://cr.openjdk.java.net/~naoto/8248695/webrev.00/
>>>>>
>>>>> There were two causes that resulted in throwing exceptions. One was 
>>>>> that the Host adapter for Windows always produced Date and Time 
>>>>> combined patterns, so formatting a LocalDate ended up with 
>>>>> unsupported temporal field for HourOfDay (reported in the bug), and 
>>>>> the other cause was the pattern for am/pm was "aa", which was not 
>>>>> valid as a DateTimeFormatter pattern.
>>>>>
>>>>> Besides these issues, localized DayOfWeek/AM_PM names have not been 
>>>>> correctly implemented in the host adapter. Now those names are 
>>>>> correctly returned from Windows.
>>>>>
>>>>> Naoto
>>>
>