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

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[:Margaret Leibovic]

Right now we run into an error in Fennec because the search service can't find this pref.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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.

Comment 4

[:Margaret Leibovic]

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.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(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 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8543344 [details] [diff] [review]
patch

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

r=me for the b2g/ changes.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/20dd052812f8

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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).

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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...

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Oh, it did show up on trunk. Sigh...

Comment 13

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/fa14cd998e39

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Followup to fix a harmless typo:
https://hg.mozilla.org/integration/fx-team/rev/a2719cbe55bd
https://hg.mozilla.org/releases/mozilla-aurora/rev/17c6cdb0f99f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20dd052812f8
https://hg.mozilla.org/mozilla-central/rev/fa14cd998e39
https://hg.mozilla.org/mozilla-central/rev/a2719cbe55bd

Comment 16

[Philip Chee]

https://hg.mozilla.org/mozilla-central/rev/20dd052812f8#l4.21
When building SeaMonkey (or Thunderbird) what do we use for %MOZILLA_API_KEY% ?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(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.