<i18n dev> [10] RFR: 8160199: Language's script should be reflected in user.script on Mac OS X

[Brent Christian]

Thanks, Naoto.  The updated changes look good.

-Brent
On 06/28/2017 03:13 PM, Naoto Sato wrote:
> Thanks, Brent! Here is the updated webrev:
>
> http://cr.openjdk.java.net/~naoto/8160199/webrev.04/
>
> And my comments embedded below:
>
> On 6/28/17 2:16 PM, Brent Christian wrote:
>> Hi, Naoto
>>
>> This looks quite good.
>>
>> I will test a bit with some of the trickier locales.  BTW, the script
>> will now also be reflected in Locale.getScript() and
>> Locale.getDisplayScript().
>>
>> Just a few comments:
>>
>> ---
>> src/java.base/macosx/native/libjava/java_props_macosx.h
>>
>> Update copyright year
>>
>> ---
>> src/java.base/unix/native/libjava/locale_str.h
>>
>> Update copyright year
>
> Done.
>
>>
>> 214     "Hans", "Hans",
>> 215     "Hant", "Hant",
>> 216     "Latn", "Latn",
>> 217     "Cyrl", "Cyrl",
>> 218     "Arab", "Arab",
>>
>> MacOS 10.12 ("Sierra") has a number of new languages not present in
>> 10.11, including a few new scripts.  I believe these should also be
>> added here:
>>
>> Deva
>> Ethi
>> Sund
>> Syrc
>> Tfng
>
> Good catch! Added those and tested, except "Syrc" script (could not find
> on my machine).
>
>>
>> ---
>> src/java.base/macosx/native/libjava/java_props_macosx.c
>>
>> Similar to the previous code, the call to:
>>
>> 79 CFStringGetCString(CFLocaleGetIdentifier(CFLocaleCopyCurrent()),
>> 80                                localeString, LOCALEIDLENGTH,
>> CFStringGetSystemEncoding());
>>
>> can be moved inside the if():
>>
>> 84             if (hyphenPos == NULL || // languageString contains
>> ISO639 only, e.g., "en"
>> 85                 languageString + langStrLen - hyphenPos == 5) {
>
> Done.
>
>>
>>
>> FWIW, I believe this code path will only be taken for MacOS 10.11 and
>> earlier.  Per TN2418[1] (and my testing thus far), 10.12 will always
>> provide a region to 'languageString'.  10.11 and prior often return
>> just a two-character language code.
>
> Yes.
>
>>
>>
>> 134  * lang{_region}{@script}. e.g.,
>> 135  * for "zh-Hans-US" into "zh_US at Hans"
>>
>> This comment could fit on one line.
>>
>>
>> 152             char tmp[4];
>>
>> Maybe a more descriptive name ("tmpScript" ?)
>
> Done.
>
>>
>>
>> 173         assert((length == 3 || length == 8) &&
>> ...
>>
>> Idea: adding another set of ()s grouping:
>>
>>    length 3 or 8 case
>>    length 4 or 9 case
>>    length == 5 case
>>
>> would cause each case to highlight nicely in an IDE.  Just a thought.
>
> I grouped those, but haven't confirmed on an IDE :-)
>>
>>
>> A JDK 10 Reviewer will also need to take a look.
>
> Sure, wait for another review then.
>
> Naoto
>
>>
>> Thanks,
>> -Brent
>>
>> 1.
>> https://developer.apple.com/library/content/technotes/tn2418/_index.html
>>
>>
>> On 6/28/17 10:51 AM, Naoto Sato wrote:
>>> Hello,
>>>
>>> Please review the fix to the following issue:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8160199
>>>
>>> The fix is located at:
>>>
>>> http://cr.openjdk.java.net/~naoto/8160199/webrev.03/
>>>
>>> The gist of the issue was that the script code has not been properly
>>> treated in macOS specific code. Since it shares the same code with
>>> other Unix, the locale format should follow POSIX, such as az-Cyrl-AZ
>>> needing to be az_AZ at Cyrl.
>>>
>>> Naoto