Move browser.search.geoip.url pref to all.js

RESOLVED FIXED in Firefox 36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Gavin)

Tracking

Trunk
Firefox 37
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox36+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Right now we run into an error in Fennec because the search service can't find this pref.
Created attachment 8543344 [details] [diff] [review]
patch

This does a bit of a broader consolidation of search prefs, moving the shared ones to all.js where they should be (given that the search service is a toolkit component). It leaves the app-specific prefs to app-specific pref files. I will attach a narrower fix for uplifting.

Summary of changes:

* Fennec (r?margaret):
- browser.search.log, browser.search.update.log, and browser.search.official match the new all.js values, so I've removed them from mobile.js.
- browser.search.updateinterval is no longer used (since https://hg.mozilla.org/mozilla-central/rev/0c8d99d73f09) so I just removed it
- all other browser.search prefs are either non-default or Fennec-specific

* B2G (r?fabrice)
- browser.search.log, browser.search.update.log, and browser.search.suggest.enabled match the new all.js defaults, so I removed them from b2g.js.
- browser.search.updateinterval is no longer used (since https://hg.mozilla.org/mozilla-central/rev/0c8d99d73f09) so I just removed it

* all.js/firefox.js (r?markh)
- this is a straight move of non-app-specific defaults from firefox.js to all.js
- except for: added a global default for browser.search.geoSpecificDefaults (false), but continue to override it for Firefox to make it true there
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8543344 - Flags: review?(mhammond)
Attachment #8543344 - Flags: review?(margaret.leibovic)
Attachment #8543344 - Flags: review?(fabrice)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1)
> - browser.search.updateinterval is no longer used (since
> https://hg.mozilla.org/mozilla-central/rev/0c8d99d73f09) so I just removed it

FWIW the new pref is browser.search.update.interval, but both Fennec and B2G don't need to override it because they both disable updating completely.
Created attachment 8543346 [details] [diff] [review]
patch for aurora

Minimal patch that just moves the additions from bug 1109120 to all.js.

Other search service consumers don't yet use geo-specific defaults, but I think they should still do the lookups and cache the values in case that is useful to them later.

I will make sure to coordinate the privacy notice changes required for all affected apps.
Attachment #8543346 - Flags: review?(mhammond)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8543344 [details] [diff] [review]
patch

Review of attachment 8543344 [details] [diff] [review]:
-----------------------------------------------------------------

mobile.js changes look fine to me.

::: mobile/android/app/mobile.js
@@ -259,5 @@
>  pref("browser.search.order.2", "chrome://browser/locale/region.properties");
>  pref("browser.search.order.3", "chrome://browser/locale/region.properties");
>  
>  // disable updating
>  pref("browser.search.update", false);

What does this do and do you know why we disabled it for Fennec?

::: modules/libpref/init/all.js
@@ +4445,5 @@
> +pref("browser.search.update.log", false);
> +pref("browser.search.update.interval", 21600);
> +pref("browser.search.suggest.enabled", true);
> +pref("browser.search.geoSpecificDefaults", false);
> +pref("browser.search.geoip.url", "https://location.services.mozilla.com/v1/country?key=%MOZILLA_API_KEY%");

I wonder if we should try to use this new geo ip service for our snippets country code to avoid making redundant network requests. We should remember to look into this if we enable this for Fennec in the future.
Attachment #8543344 - Flags: review?(margaret.leibovic) → review+
Attachment #8543346 - Flags: review?(mhammond) → review+
Attachment #8543344 - Flags: review?(mhammond) → review+
(In reply to :Margaret Leibovic from comment #4)
> >  pref("browser.search.update", false);
> 
> What does this do and do you know why we disabled it for Fennec?

Allows OpenSearch-installed plugins to update themselves. Not sure why Fennec disabled it specifically. Very few plugins use this in practice, we should probably consider ripping out this code. (We originally planned to use it for built-in plugins too, but never got around to doing that.)

> I wonder if we should try to use this new geo ip service for our snippets
> country code to avoid making redundant network requests. We should remember
> to look into this if we enable this for Fennec in the future.

As-is, this patch will result in redundant GeoIP requests for Fennec (it enables the search service one when you already have a snippets one). If you want to avoid that until you really need this, then I guess you should override the default value for browser.search.geoip.url to be "".
Comment on attachment 8543344 [details] [diff] [review]
patch

Review of attachment 8543344 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the b2g/ changes.
Attachment #8543344 - Flags: review?(fabrice) → review+
I pushed an additional change to disable the geoip request for Fennec until bug 1117186 is fixed:

https://hg.mozilla.org/integration/fx-team/rev/20dd052812f8#l3.40
Created attachment 8544105 [details] [diff] [review]
patch for aurora that also disable for mobile

This is a tweak to the Aurora patch that is consistent with the trunk patch (disable the geoip request for Fennec).
Attachment #8543346 - Attachment is obsolete: true
Attachment #8544105 - Flags: review?(margaret.leibovic)
(Reporter)

Updated

4 years ago
Attachment #8544105 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/releases/mozilla-aurora/rev/fdcd5874a93d
status-firefox36: --- → fixed
tracking-firefox36: --- → +
Pushed a followup to do the standard geoip request disabling in services/healthreport/tests/xpcshell/test_provider_searches.js:

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9fe9b0078ec

Not sure why this didn't crop up on trunk...
Oh, it did show up on trunk. Sigh...
Iteration: --- → 37.3 - 12 Jan
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify-

Comment 16

4 years ago
https://hg.mozilla.org/mozilla-central/rev/20dd052812f8#l4.21
When building SeaMonkey (or Thunderbird) what do we use for %MOZILLA_API_KEY% ?
(In reply to Philip Chee from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/20dd052812f8#l4.21
> When building SeaMonkey (or Thunderbird) what do we use for
> %MOZILLA_API_KEY% ?

I'm not sure if this answers your question, but configure.in has a default value for MOZILLA_API_KEY=no-mozilla-api-key - https://dxr.mozilla.org/mozilla-central/source/configure.in#4026.  This is what desktop is currently using and I guess/hope that SeaMonkey/TB will use this initially.

Bug 1113606 exists to get a "real" key for each of our channels (although IIUC, Nightly will always still get that default rather than a nightly-specific key) - I suspect that we need matching bugs for those other products, but also suspect that the default value will continue to work for some time simply as it will need to for Nightly.

Updated

4 years ago
Blocks: 1115575
You need to log in before you can comment on or make changes to this bug.