Closed Bug 1405685 Opened 7 years ago Closed 6 years ago

Geolocation search pref (browser.search.geoSpecificDefaults) should not live in firefox-l10n.js

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 + fixed
firefox60 + fixed

People

(Reporter: flod, Assigned: mkaply)

References

Details

Attachments

(2 files)

Bug 1405685 - Use geo specific defaults for all locales.
59 bytes, text/x-review-board-request
mconnor
: review+
mkaply
: review+
Details
Bug 1405685 - Use geospecific defaults for all locales.
59 bytes, text/x-review-board-request
florian
: review+
ritu
: approval-mozilla-beta+
Details 
I've spent the afternoon cleaning up firefox-l10n.js files in l10n repositories, where browser.search.geoSpecificDefaults was added incorrectly by a localization tool (Pootle).

The only locales with that setting are now be, kk, ru, tr, uk, zh-TW

firefox-l10n.js doesn't work in build+language pack scenarios, and we need to figure out a better way to centralize this setting if the goal is to create multi-locales builds of Firefox.
What is browser.search.geoSpecificDefaults set to in those locales?
(In reply to Mike Kaply [:mkaply] from comment #1)
> What is browser.search.geoSpecificDefaults set to in those locales?

It's set to true
https://dxr.mozilla.org/l10n-central/source/zh-TW/browser/firefox-l10n.js

Other locales had true as well, since it was copied from en-US. This is the changeset removing it
https://hg.mozilla.org/l10n-central/ar/rev/9886b64e06b7

en-US: http://searchfox.org/mozilla-central/source/browser/locales/en-US/firefox-l10n.js

Default value seems false on browser, but true on mobile?
http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#394
http://searchfox.org/mozilla-central/source/mobile/android/app/mobile.js#279
Everyone sets it to true. We should just default it to true and remove any custom settings.

(I don't want to remove the pref, as it is needed for testing).

Note that if we move it to true, we need to make sure tests still pass.
(In reply to Mike Kaply [:mkaply] from comment #3)
> Everyone sets it to true. We should just default it to true and remove any
> custom settings.

I tried to look at the code but got lost. If we set the default to true:
* Are we going to send unnecessary requests to a server to get region search settings?
* Does the request take the UI locale into account?
(In reply to Francesco Lodolo [:flod] from comment #4)
> * Does the request take the UI locale into account?

Answering my own question
geoSpecificDefaults.url = https://search.services.mozilla.com/1/%APP%/%VERSION%/%CHANNEL%/%LOCALE%/%REGION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%

The only doubt is about making unnecessary requests, even if they seem cached.
> * Are we going to send unnecessary requests to a server to get region search settings?

I thought every locale sets it to true anyway?

> Does the request take the UI locale into account?

Yes. It returns correct search settings based on the locale of the browser combined with the true region of the user.
(In reply to Mike Kaply [:mkaply] from comment #6)
> > * Are we going to send unnecessary requests to a server to get region search settings?
> 
> I thought every locale sets it to true anyway?

Uhm, no. I interpreted "those locales" in comment 1 as "be, kk, ru, tr, uk, zh-TW".

Right now, they're the only locale set to true, since I've fixed all the others, and the majority was already not setting a custom value in firefox-l10n.js and inheriting false.
I'm even more confused now.

According to this:

https://dxr.mozilla.org/l10n-mozilla-release/search?q=browser.search.geoSpecificDefaults&redirect=false

42 locales set it to true?
(In reply to Mike Kaply [:mkaply] from comment #8)
> I'm even more confused now.
> 
> According to this:
> 
> https://dxr.mozilla.org/l10n-mozilla-release/search?q=browser.search.
> geoSpecificDefaults&redirect=false
> 
> 42 locales set it to true?

You're looking at release, I've fixed l10n-central (which covers both Beta and Nightly, and will be used for Release starting from 57). Also, DXR is heavily cached
https://dxr.mozilla.org/l10n-central/search?q=browser.search.geoSpecificDefaults&redirect=false

e.g. ach shows up here, but it's fixed
https://hg.mozilla.org/l10n-central/ach/filelog/default/browser/firefox-l10n.js
So going forward, we are going to just turn on geolocation for everyone and remove the pref from l10n.

We should probably do this very early in a nightly cycle in case there are any issues, so maybe right after 59 goes to nightly?
(In reply to Mike Kaply [:mkaply] from comment #10)
> We should probably do this very early in a nightly cycle in case there are
> any issues, so maybe right after 59 goes to nightly?

Sounds reasonable to me, no point in investing resources while the focus is still on shipping 57.
We're going to do this for 58. I'm setting the pref to true and moving it completely out of l10n.
Assignee: nobody → mozilla
Attachment #8923840 - Flags: review?(mconnor) → review+
Comment on attachment 8923840 [details]
Bug 1405685 - Use geo specific defaults for all locales.

https://reviewboard.mozilla.org/r/195000/#review208554

r+ by mconnor in bug.
Attachment #8923840 - Flags: review+
Attachment #8923840 - Attachment is obsolete: true
Attachment #8923840 - Attachment is obsolete: false
Depends on: 1428830
No longer depends on: 1428830
firefox-l10n.js can't be removed from the build until it's not used in any locale.
From a quick look, firefox-l10n.js is used for

browser.fixup.alternate.suffix
be, cs, da, nb-NO, nn-NO, sk

browser.search.geoSpecificDefaults
be, kk, ru, tr, uk, zh-TW

browser.search.order.*
ja, ja-JP-mac

browser.startup.homepage
zh-CN

fonts
zh-TW
https://dxr.mozilla.org/l10n-central/source/zh-TW/browser/firefox-l10n.js

* Fonts seems used only on XP for zh-TW
* browser.search.order should be obsolete after bug 1352539
* browser.search.geoSpecificDefaults should become obsolete in this bug

That leaves us with zh-CN homepage, for which we also have an extra string in browser.properties (confusing…), and the alternate suffix.
Comment on attachment 8946942 [details]
Bug 1405685 - Use geospecific defaults for all locales.

https://reviewboard.mozilla.org/r/216794/#review222726

Should we just remove the preference and all the calls to geoSpecificDefaultsEnabled in the search service? Or do we somehow still need this preference for linux distributions?
Comment on attachment 8946942 [details]
Bug 1405685 - Use geospecific defaults for all locales.

https://reviewboard.mozilla.org/r/216794/#review222726

I think we should keep the pref for now because we might create a policy around it to disable it in enterprises. So for now, I think we should make it default to true.
We need to get this in for 59.
Flags: needinfo?(florian)
Tracking this for 59. Do we need testing in 60/Nightly before uplift or is this something we need to uplift, then test in 59 beta?
status-firefox59: --- → affected
status-firefox60: --- → affected
tracking-firefox59: --- → +
tracking-firefox60: --- → +
Comment on attachment 8946942 [details]
Bug 1405685 - Use geospecific defaults for all locales.

https://reviewboard.mozilla.org/r/216794/#review224274
Attachment #8946942 - Flags: review?(florian) → review+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Tracking this for 59. Do we need testing in 60/Nightly before uplift or is
> this something we need to uplift, then test in 59 beta?

Testing on a en-US nightly won't help. If we want to test on Nightly, we should test a localized build for a locale that wasn't setting this pref before.
Flags: needinfo?(florian)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/ecd8c15f4c4d
Use geospecific defaults for all locales. r=florian
https://hg.mozilla.org/mozilla-central/rev/ecd8c15f4c4d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment on attachment 8946942 [details]
Bug 1405685 - Use geospecific defaults for all locales.

Approval Request Comment
[Feature/Bug causing the regression]: Cause all builds in all locales to look geo specific search defaults
[User impact if declined]: None
[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?]: Low
[Why is the change risky/not risky?]: 
[String changes made/needed]:

Currently we only get geo specific search defaults for certain locale builds. This change allows us to get geo specific defaults for all locale builds.

This is a business related change.
Attachment #8946942 - Flags: approval-mozilla-beta?
Comment on attachment 8946942 [details]
Bug 1405685 - Use geospecific defaults for all locales.

Business decision, Beta59+
Attachment #8946942 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: