Bug 146121 – REGRESSION (r172975): navigator.language unable to tell region for Traditional Chinese users

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

navigator.language now returns "zh-hant" when system language is set to Chinese (Traditional, Hong Kong) or to Chinese (Traditional, Taiwan). This breaks iCloud, which now displays a Simplified Chinese UI to users with these primary languages.

We used to return "zh-tw" for both these languages, which was wrong, but didn't cause any known problems in practice.

rdar://problem/21395180

[Alexey Proskuryakov]

Created attachment 255135 [details]
proposed fix

[WebKit Commit Bot]

Attachment 255135 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/mac/NavigatorLanguage.mm:116:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/mac/NavigatorLanguage.mm:119:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/spi/cf/CFBundleSPI.h:41:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cf/CFBundleSPI.h:42:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Created attachment 255178 [details]
proposed fix

With a build fix.

[WebKit Commit Bot]

Attachment 255178 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/mac/NavigatorLanguage.mm:116:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/mac/NavigatorLanguage.mm:119:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/spi/cf/CFBundleSPI.h:41:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cf/CFBundleSPI.h:42:  The parameter name "stringEncoding" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 255178 [details]
proposed fix

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

> Source/WebCore/ChangeLog:9
> +        Revert to previous behavior, which is wring in many ways, but not as wrong as the new one.

Typo here: "wring".

> Source/WebCore/platform/mac/Language.mm:91
> +    // FIXME: This transformation is very wrong:
> +    // 1. There is no reason why CFBundle localization names would be at all related to language names as used on the Web.
> +    // 2. Script Manager codes cannot represent all languages that are now supported by the platform, so the conversion is lossy.
> +    // 3. This should probably match what is sent by the network layer as Accept-Language, but currently, that's implemented separately.

A problem with this comment is that it says what’s wrong with the code below, but nothing about what’s right about it. I think it’s useful to say why this is a good choice for now (if it is) in the comment, also, not just what’s wrong.

After all, the whole point of this current patch is to switch back to this! So there must be some value in it.

> Source/WebCore/platform/spi/cf/CFBundleSPI.h:40
> +// FIXME: Stop using these, as Script Manager codes cannot reperesent all supported languages.

Typo in the word represent.

I suggest putting this kind of comment at the call site, not in the SPI.h header.

[Alexey Proskuryakov]

Committed http://trac.webkit.org/r185816

> A problem with this comment is that it says what’s wrong with the code
> below, but nothing about what’s right about it. I think it’s useful to say
> why this is a good choice for now (if it is) in the comment, also, not just
> what’s wrong.

Unsure what good to say here. I can see how it is surprising to see a comment with so many downsides in a patch, however I don't see a lasting value for such an explanation in code. We don't normally have anything like that near FIXMEs.

Instead, the regression test would catch potential future mistakes. I think that adding a way to test navigator.language is the more important part of this patch.

Addressed the other suggestions when landing.

[Darin Adler]

(In reply to comment #6)
> Committed http://trac.webkit.org/r185816
> 
> > A problem with this comment is that it says what’s wrong with the code
> > below, but nothing about what’s right about it. I think it’s useful to say
> > why this is a good choice for now (if it is) in the comment, also, not just
> > what’s wrong.
> 
> Unsure what good to say here.

My request here was to add the kind of comment that would prevent a future programmer from repeating the same mistake we just made, again. I think that would be straightforward.

[Darin Adler]

I suppose it’s the test that prevents the future mistake.