Closed Bug 1252627 Opened 4 years ago Closed 4 years ago

distribution.searchplugins.defaultLocale should take precedence over general.useragent.locale when looking for engines

Categories

(Firefox :: Distributions, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 3 obsolete files)

If you put an en-US directory in searchplugins/locales, it causes the distribution.searchplugins.defaultLocale pref to be ignored on an en-US build.

1. This should work.

2. We should be honoring distribution.searchplugins.defaultLocale above general.useragent.locale. You should be able to ship an en-US build with an en-HK search engine and use distribution.searchplugins.defaultLocale to make it the default.
The code only checks for the existence of the directory, not that it has files. We can't do much about that.

But I do firmly believe we should move the distribution locale code above the useragent locale.
Summary: The presence of an en-US searchplugins/locale directory in distribution dir causes distribution.searchplugins.defaultLocale to be ignored → distribution.searchplugins.defaultLocale should take precedence over general.useragent.locale when looking for engines
Attached patch Reverse order of engine seaarch (obsolete) — Splinter Review
This is the desktop patch. We'll need a similar patch for android.

This patch only affects affects distributions - there is no other reason someone would set distribution.searchplugins.defaultLocale.

Going forward, this allows us to properly have all locale specific engines in one build.
Attachment #8725697 - Flags: review?(mixedpuppy)
Attached patch Smaller diff (obsolete) — Splinter Review
The other diff had extra stuff in it.
Attachment #8725697 - Attachment is obsolete: true
Attachment #8725697 - Flags: review?(mixedpuppy)
Attachment #8726234 - Flags: review?(mixedpuppy)
Comment on attachment 8726234 [details] [diff] [review]
Smaller diff

this looks fine to me, are there tests that cover this code?
Attachment #8726234 - Flags: review?(mixedpuppy) → review+
> this looks fine to me, are there tests that cover this code?

Sadly no. There are some distribution tests, but they don't cover this.

I'll see what I can come up with.
Attached patch Diff with test (obsolete) — Splinter Review
This adds a test that verifies that even when there is an en-US directory, the default locale preference overrides the installed engine. That's why the test engine is named Google, to verify that.

I did a quick check of our distributions and not a single one of them uses the locale directory, so this will have no effect on them (and as a side note, we should fix that).

The Mozilla Chinese distribution uses the locale directory, but only has the zh-CN directory, so again, no effect.

This is mainly needed when we attempt to build larger distributions with multiple engines.
Attachment #8726234 - Attachment is obsolete: true
Attachment #8726815 - Flags: review?(mixedpuppy)
Comment on attachment 8726815 [details] [diff] [review]
Diff with test

Does the engine file need to be removed in the cleanup function?
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Comment on attachment 8726815 [details] [diff] [review]
> Diff with test
> 
> Does the engine file need to be removed in the cleanup function?

Yes, probably should. Adding.
I'll just remove the whole distribution dir.

Any tests that need it create it.
Attachment #8726815 - Attachment is obsolete: true
Attachment #8726815 - Flags: review?(mixedpuppy)
Attachment #8726886 - Flags: review?(mixedpuppy)
Attachment #8726886 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/integration/fx-team/rev/99e6ed34499483d39ada0c501ffd736116f35828
Bug 1252627 - distribution defaultLocale shoudld take precedence over user agent locale; r=mixedpuppy
https://hg.mozilla.org/integration/fx-team/rev/6f19b1a57959a2e918096c0d66c325ec39583e91
Bug 1252627 - Remove trailing space for eslint to fix bustage - CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/99e6ed344994
https://hg.mozilla.org/mozilla-central/rev/6f19b1a57959
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blocks: 1299234
You need to log in before you can comment on or make changes to this bug.