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

[Naoto Sato]

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