Closed Bug 1420255 Opened 2 years ago Closed 2 years ago

Regression in testing for default locale from bug 1410736

Categories

(Core :: Internationalization, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 blocking verified
firefox59 --- verified

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Assignee: nobody → gandalf
Blocks: 1410736
Status: NEW → ASSIGNED
Priority: -- → P2
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 ===.
Attachment #8931461 - Flags: review?(florian) → review+
[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.
Keywords: regression
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
Attachment #8931461 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/c2832d8c0798
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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.
Flags: qe-verify+
serious issue; marking it as blocking the release
Comment on attachment 8931461 [details]
Bug 1420255 - Fix a regression from bug 1410736.

Fix a 58 blocker. Beta58+.
Attachment #8931461 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.