REGRESSION(r185816): In the Hong Kong locale, navigator.language reports it's in the Taiwan locale

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

In the Hong Kong locale, navigator.language reports it's in the Taiwan locale

[Myles C. Maxfield]

Created attachment 374701 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 374701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374701&action=review

> Source/WTF/ChangeLog:14
> +        Nowadays, CFLocaleCopyPreferredLanguages() reports real BCP47 language codes, so we don't have to do any

Does it actually perform the normalization, or is it just that System Preferences writes BCP47 language codes in new OS versions? People who've been using macOS for a while may still have non-BCP47 preferred languages.

One way to test would be to override AppleLanguages.

> Source/WTF/wtf/cf/LanguageCF.cpp:-59
> -    // 3. This should probably match what is sent by the network layer as Accept-Language, but currently, that's implemented separately.

This one remains true.

[Alexey Proskuryakov]

As this adds differentiation especially for languages that are less frequently used, some people may now be uniquely identified with low-tech methods like looking at navigator.userAgent and navigator.language.

I'm all for improving language support, especially when removing such ugly hacks, however we may want to consult with privacy experts.

[EWS Watchlist]

Comment on attachment 374701 [details]
Patch

Attachment 374701 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12796133

New failing tests:
js/dom/navigator-language.html

[EWS Watchlist]

Created attachment 374708 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 374701 [details]
Patch

Attachment 374701 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12796226

New failing tests:
js/dom/navigator-language.html

[EWS Watchlist]

Created attachment 374711 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 374701 [details]
Patch

Attachment 374701 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12796336

New failing tests:
js/dom/navigator-language.html

[EWS Watchlist]

Created attachment 374713 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Created attachment 381336 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/44119496>

[Ryosuke Niwa]

Comment on attachment 381336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381336&action=review

> Source/WTF/ChangeLog:17
> +        Reviewed by NOBODY (OOPS!).

Nit: This line should appear before the long description.

[Myles C. Maxfield]

Oh, I forgot to update a test result.

[Myles C. Maxfield]

Created attachment 381351 [details]
Patch for reviewing

[Myles C. Maxfield]

Comment on attachment 381336 [details]
Patch

We're going to solve this a different way, which protects privacy better.

[Myles C. Maxfield]

Created attachment 386021 [details]
Patch

[Myles C. Maxfield]

Created attachment 386027 [details]
Patch

[Ryosuke Niwa]

The test failure looks real:

--- /Volumes/Data/worker/macOS-High-Sierra-Release-WK1-Tests-EWS/build/layout-test-results/js/dom/navigator-language-expected.txt
+++ /Volumes/Data/worker/macOS-High-Sierra-Release-WK1-Tests-EWS/build/layout-test-results/js/dom/navigator-language-actual.txt
@@ -2,7 +2,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS language is 'en'
+FAIL language should be en-US. Was en-us.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Myles C. Maxfield]

Created attachment 387174 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

Just a comment about something that caught my eye, didn’t attempt to review the rest of the patch.

> LayoutTests/fast/text/international/system-language/navigator-language/navigator-language-en-GB.html:4
> +<script src="../../../../../resources/js-test-pre.js"></script>

Can all of the tests use js-test.js instead of pre/post? That’s generally preferred due to simplicity.

[Dean Jackson]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

>> LayoutTests/fast/text/international/system-language/navigator-language/navigator-language-en-GB.html:4
>> +<script src="../../../../../resources/js-test-pre.js"></script>
> 
> Can all of the tests use js-test.js instead of pre/post? That’s generally preferred due to simplicity.

Or just write your own small .js file in this directory to expose shouldBeEqualToString and do the dump as text stuff.

[Myles C. Maxfield]

Committed r254389: <https://trac.webkit.org/changeset/254389>

[Myles C. Maxfield]

Committed r254391: <https://trac.webkit.org/changeset/254391>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 206134

[Myles C. Maxfield]

This broke internal builds. Investigating now.

[Simon Fraser (smfr)]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

> Source/WTF/wtf/cocoa/LanguageCocoa.mm:36
> +    static NeverDestroyed<bool> result = [NSLocale respondsToSelector:@selector(minimizedLanguagesFromLanguages:)];

static bool should be enough.

[Myles C. Maxfield]

https://bugs.webkit.org/show_bug.cgi?id=206135 should fix the internal builds.

[Myles C. Maxfield]

Committed r254412: <https://trac.webkit.org/changeset/254412>

[Myles C. Maxfield]

Revision list:

r254389
r254391
r254411
r254412

[Myles C. Maxfield]

Committed r254413: <https://trac.webkit.org/changeset/254413>

[Myles C. Maxfield]

Revision list:

r254389
r254391
r254411
r254412
r254413

[Myles C. Maxfield]

Committed r254415: <https://trac.webkit.org/changeset/254415>

[Myles C. Maxfield]

Revision list:

r254389
r254391
r254411
r254412
r254413
r254415

[Brent Fulgham]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

> Source/WTF/wtf/spi/cocoa/NSLocaleSPI.h:30
> +#import <InternationalSupport/NSLocale+InternationalSupportExtensions.h>

Does this exist on all supported macOS build targets?

[Chris Dumez]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

> Source/WTF/wtf/spi/cocoa/NSLocaleSPI.h:35
> ++ (nonnull NSArray<NSString *> *)minimizedLanguagesFromLanguages:(nonnull NSArray<NSString *> *)languages;

Note that on my macOS SDK, even though I do have the <InternationalSupport/NSLocale+InternationalSupportExtensions.h> header, it does not declare this method (minimizedLanguagesFromLanguages). My iOS SDK does have it though. Does this fix really work on macOS? Is it supposed to be iOS-only.

[Chris Dumez]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

>> Source/WTF/wtf/cocoa/LanguageCocoa.mm:36
>> +    static NeverDestroyed<bool> result = [NSLocale respondsToSelector:@selector(minimizedLanguagesFromLanguages:)];
> 
> static bool should be enough.

I guess this would just return false on macOS since it would not be able to find this method.

[Myles C. Maxfield]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

>> Source/WTF/wtf/spi/cocoa/NSLocaleSPI.h:35
>> ++ (nonnull NSArray<NSString *> *)minimizedLanguagesFromLanguages:(nonnull NSArray<NSString *> *)languages;
> 
> Note that on my macOS SDK, even though I do have the <InternationalSupport/NSLocale+InternationalSupportExtensions.h> header, it does not declare this method (minimizedLanguagesFromLanguages). My iOS SDK does have it though. Does this fix really work on macOS? Is it supposed to be iOS-only.

Your macOS SDK is probably too old.

+ (nonnull NSArray <NSString *> *)minimizedLanguagesFromLanguages:(nonnull NSArray <NSString *> *)languages API_AVAILABLE(macos(10.15.4), ios(13.4), watchos(6.4), tvos(13.4));

[Chris Dumez]

It seems this may have made navigator.language / navigator.languages return all lowercase strings again? (see Bug 163096). Per spec, it should be "en-US" not "en-us".

[Myles C. Maxfield]

(In reply to Chris Dumez from comment #38)
> It seems this may have made navigator.language / navigator.languages return
> all lowercase strings again? (see Bug 163096). Per spec, it should be
> "en-US" not "en-us".

I don’t know about “again,” but yes, this lowercases the strings:

CFStringLowercase(mutableLanguageCode.get(), nullptr);

I did it for consistency with the behavior before this patch. Before this patch, the strings were lower-cased, too.

[Chris Dumez]

Comment on attachment 387174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387174&action=review

> LayoutTests/js/dom/navigator-language.html:-21
> -    shouldBe("language", "'en-US'");

Really?

> LayoutTests/js/dom/navigator-language.html:21
> +    shouldBe("language", "'en-us'");

Then why this?

[Chris Dumez]

(In reply to Myles C. Maxfield from comment #39)
> (In reply to Chris Dumez from comment #38)
> > It seems this may have made navigator.language / navigator.languages return
> > all lowercase strings again? (see Bug 163096). Per spec, it should be
> > "en-US" not "en-us".
> 
> I don’t know about “again,” but yes, this lowercases the strings:

Again because I fixed it once here: Bug 163096.

[Chris Dumez]

(In reply to Chris Dumez from comment #41)
> (In reply to Myles C. Maxfield from comment #39)
> > (In reply to Chris Dumez from comment #38)
> > > It seems this may have made navigator.language / navigator.languages return
> > > all lowercase strings again? (see Bug 163096). Per spec, it should be
> > > "en-US" not "en-us".
> > 
> > I don’t know about “again,” but yes, this lowercases the strings:
> 
> Again because I fixed it once here: Bug 163096.

I have fixed Bug 225461.

[Chris Dumez]

(In reply to Chris Dumez from comment #42)
> (In reply to Chris Dumez from comment #41)
> > (In reply to Myles C. Maxfield from comment #39)
> > > (In reply to Chris Dumez from comment #38)
> > > > It seems this may have made navigator.language / navigator.languages return
> > > > all lowercase strings again? (see Bug 163096). Per spec, it should be
> > > > "en-US" not "en-us".
> > > 
> > > I don’t know about “again,” but yes, this lowercases the strings:
> > 
> > Again because I fixed it once here: Bug 163096.
> 
> I have fixed Bug 225461.

I have *filed* Bug 225461.