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@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@Cyrl.
Naoto
