1420255 - Regression in testing for default locale from bug 1410736

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P2)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

In bug 1410736 we migrated the remaining uses of general.useragent.locale in anticipation for bug 1414390.

:florian pointed out that we introduced a regression in https://hg.mozilla.org/mozilla-central/rev/f401d9f8a87d#l10.48

Let's fix it.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1420255 - Fix a regression from bug 1410736.

Review commit: https://reviewboard.mozilla.org/r/202608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202608/

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8931461 [details]
Bug 1420255 - Fix a regression from bug 1410736.

https://reviewboard.mozilla.org/r/202608/#review207924

The change looks good to me. I'm a bit worried this isn't covered by a test, but bug 1289240 comment 3 explains why this is difficult to test.

::: toolkit/components/search/nsSearchService.js:2273
(Diff revision 1)
>  
>      // If we are using a non-default locale or in the xpcshell test case,
>      // we'll accept as a 'default' engine anything that has been registered at
>      // resource://search-plugins/ even if the file doesn't come from the
>      // application folder.  If not, skip costly additional checks.
> -    if (Services.locale.defaultLocale !== Services.locale.getRequestedLocale() &&
> +    if (Services.locale.defaultLocale === Services.locale.getRequestedLocale() &&

nit: Our front-end code usually uses == rather than === for comparisons, unless there's a specific reason to need ===.

Comment 3

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: I haven't verified this, but I'm afraid this may cause the search reset code in bug 1419941 to be triggered for engines shipped by language packs (which we want to treat as default engines, and not affect with the reset). I'm glad 57 isn't affected.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8931461 [details]
Bug 1420255 - Fix a regression from bug 1410736.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202608/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2832d8c0798
Fix a regression from bug 1410736. r=florian

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8931461 [details]
Bug 1420255 - Fix a regression from bug 1410736.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1410736
[User impact if declined]: the non-default locale default search engine selection is broken.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a simple regression with a single character fix that the author and reviewer of the patch understand.
[String changes made/needed]: none

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c2832d8c0798

Comment 8

[Florian Quèze [:florian]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)

> [Needs manual test from QE? If yes, steps to reproduce]: no

Given this bug has a significant impact on search reset, I think we should verify it please.

Steps to reproduce:

1. Install a beta build.
2. Install a langpack. Some are available at https://tools.taskcluster.net/index/artifacts/releases.v1.mozilla-beta.latest.firefox.latest.l10n_artifacts.macosx64.4
3. In about:config, set general.useragent.locale to the locale of the langpack you just installed.
4. Restart the build to have Firefox started in the locale of the langpack.
5. From the browser console (you need to enable it from the devtools settings), verify the following:

Services.search.originalDefaultEngine.wrappedJSObject._isDefault
This should always be true. Without the fix, it's false.

Services.search.getDefaultEngines().map(e => e.name)

This should list the names of the available search engines for the current locale. Without the fix this gives an empty list.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

serious issue; marking it as blocking the release

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8931461 [details]
Bug 1420255 - Fix a regression from bug 1410736.

Fix a 58 blocker. Beta58+.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b83e2c69374

Comment 12

[Valentina Claudia Ona]

I verified this issue using Latest Nightly 59.0a1 and Latest Firefox Beta 58.0b7 with Build ID 20171127220446 on Mac OS X 10.12.6 and Mac OS X 10.12.
I will mark this as verified fixed.