1237149 - Incorrect search engines are shown for zh-CN

Status: VERIFIED FIXED

(Firefox for iOS :: General, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

For zh-CN we show an alternative list of search engines and a different default. 

The files in  Client/Assets/SearchPlugins/zh-CN/ look correct but when I change my iPhone to Simplified Chinese and China as the Region, I still get the US specific search engines and Yahoo as a default.

I checked NSLocale.currentLocale().localeIdentifier and it is correctly set to zh_CN.

I also checked the built .ipa file and all the SearchPlugins/zh-CN files are correctly included.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Oh so when we use

  let language = NSLocale.preferredLanguages().first!

We get zh-Hans-CN back.

So that will not match any of the directories in SearchPlugins.

Is there a reason we don't use NSLocale.currentLocale().localeIdentifier here?

Comment 2

[Stephan Leroux [:sleroux]]

Since this is related I figured I should ask here and get some additional verification. While I was uplifting the patches for the search engine .xml changes there were conflicts. Who would be the best person to NI to double check these?

https://github.com/mozilla/firefox-ios/tree/v1.x/Client/Assets/SearchPlugins/zh-CN

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/1409

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Patch comment:

This patch introduces a `directoriesForLanguageIdentifier()` function that returns a list of `SearchPlugins` directories to search for the given *language identifier*.

This change was made so that we correctly handle the `zh-Hans-CN` *language identifier* which does not map to any of the `SearchPlugins` subdirectories but should default to the `zh-CN` one.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8704857 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1409

Looks good, a couple minor comments in the PR.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Merged.

Comment 7

[Stephan Leroux [:sleroux]]

v1.x 9d61f45

Comment 8

[Bingqing Li]

Reopen it because it still shows the incorrect search engine on iOS 8.4.1.

Comment 9

[DongShengXue]

Attachment: Pull request for comment 8

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8705916 [details] [review]
Pull request for comment 8

I am ok with this change, but:

Please fix the testDirectoriesForLanguageIdentifier() test, which currently fails because directoriesForLanguageIdentifier() now returns a different preference for language identifiers.

Comment 11

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/1427

This is an alternative for the existing pull request that renames the directories.

I think I would prefer to just normalize the language identifier to get some commonality between iOS 8 and iOS 9. Specially because the search engine directories are generated by a script and are based on the Android ones. So I would prefer to keep the same locale identifiers in their names.

Comment 12

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8705916 [details] [review]
Pull request for comment 8

See newly attached pull request with new and simpler approach.

Comment 13

[DongShengXue]

I am ok with this change.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8706161 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1427

LGTM. Might be worth exploring using currentLocale soon so we can properly handle other languages that have similar IDs.

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

https://github.com/mozilla/firefox-ios/commit/35967ec6edbae6a9d0618216b3dcf139354f4828

Comment 16

[Stephan Leroux [:sleroux]]

v1.x e59f3b5

Comment 17

[Bingqing Li]

Verified on v1.4(1404).

It can shows the right search engines for zh-CN both on iOS 8.4.1 and iOS 9.2.

Comment 18

[Catalin Suciu [:csuciu]]

Changing the status to Verified based on the previous comment