Bug 1460419 - Remove geoSpecificPref code from search service. r?florian draft
authorMichael Kaply <mozilla@kaply.com>
Fri, 29 Jun 2018 17:18:10 -0500
changeset 814992 21e3b3c735e0d3287644a81b5d14d23819a4af72
parent 814584 e8b7331398910233e3adaaed01cad6b75bb562a2
child 815007 df09174895d05b4412d85eede7fda085ce1d8323
push id115396
push usermozilla@kaply.com
push dateFri, 06 Jul 2018 14:23:37 +0000
reviewersflorian
bugs1460419
milestone63.0a1
Bug 1460419 - Remove geoSpecificPref code from search service. r?florian MozReview-Commit-ID: 1OfikXSwtAJ
toolkit/components/search/nsSearchService.js
toolkit/components/search/tests/xpcshell/head_search.js
toolkit/components/search/tests/xpcshell/test_location_funnelcake.js
toolkit/components/search/tests/xpcshell/test_location_partner.js
toolkit/components/search/tests/xpcshell/xpcshell.ini
--- a/toolkit/components/search/nsSearchService.js
+++ b/toolkit/components/search/nsSearchService.js
@@ -372,61 +372,16 @@ function isPartnerBuild() {
   return distroID && !distroID.startsWith("mozilla");
 }
 
 // Method to determine if we should be using geo-specific defaults
 function geoSpecificDefaultsEnabled() {
   return Services.prefs.getBoolPref("browser.search.geoSpecificDefaults", false);
 }
 
-// Some notes on countryCode and region prefs:
-// * A "countryCode" pref is set via a geoip lookup.  It always reflects the
-//   result of that geoip request.
-// * A "region" pref, once set, is the region actually used for search.  In
-//   most cases it will be identical to the countryCode pref.
-// * The value of "region" and "countryCode" will only not agree in one edge
-//   case - 34/35 users who have previously been configured to use US defaults
-//   based purely on a timezone check will have "region" forced to US,
-//   regardless of what countryCode geoip returns.
-// * We may want to know if we are in the US before we have *either*
-//   countryCode or region - in which case we fallback to a timezone check,
-//   but we don't persist that value anywhere in the expectation we will
-//   eventually get a countryCode/region.
-
-// A method to determine if we are in the United States (US) for the search
-// service.
-// It uses a browser.search.region pref (which typically comes from a geoip
-// request) or if that doesn't exist, falls back to a hacky timezone check.
-function getIsUS() {
-  // Regardless of the region or countryCode, non en-US builds are not
-  // considered to be in the US from the POV of the search service.
-  if (getLocale() != "en-US") {
-    return false;
-  }
-
-  // If we've got a region pref, trust it.
-  try {
-    return Services.prefs.getCharPref("browser.search.region") == "US";
-  } catch (e) {}
-
-  // So we are en-US but have no region pref - fallback to hacky timezone check.
-  let isNA = isUSTimezone();
-  LOG("getIsUS() fell back to a timezone check with the result=" + isNA);
-  return isNA;
-}
-
-// Helper method to modify preference keys with geo-specific modifiers, if needed.
-function getGeoSpecificPrefName(basepref) {
-  if (!geoSpecificDefaultsEnabled() || isPartnerBuild())
-    return basepref;
-  if (getIsUS())
-    return basepref + ".US";
-  return basepref;
-}
-
 // A method that tries to determine if this user is in a US geography.
 function isUSTimezone() {
   // Timezone assumptions! We assume that if the system clock's timezone is
   // between Newfoundland and Hawaii, that the user is in North America.
 
   // This includes all of South America as well, but we have relatively few
   // en-US users there, so that's OK.
 
@@ -435,21 +390,17 @@ function isUSTimezone() {
 
   // 600 minutes = 10 hours (UTC-10), which is
   // Hawaii-Aleutian Standard Time (http://www.timeanddate.com/time/zones/hast)
 
   let UTCOffset = (new Date()).getTimezoneOffset();
   return UTCOffset >= 150 && UTCOffset <= 600;
 }
 
-// A less hacky method that tries to determine our country-code via an XHR
-// geoip lookup.
-// If this succeeds and we are using an en-US locale, we set the pref used by
-// the hacky method above, so isUS() can avoid the hacky timezone method.
-// If it fails we don't touch that pref so isUS() does its normal thing.
+// A method that tries to determine our country-code via an XHR geoip lookup.
 var ensureKnownCountryCode = async function(ss) {
   // If we have a country-code already stored in our prefs we trust it.
   let countryCode = Services.prefs.getCharPref("browser.search.countryCode", "");
 
   if (!countryCode) {
     // We don't have it cached, so fetch it. fetchCountryCode() will call
     // storeCountryCode if it gets a result (even if that happens after the
     // promise resolves) and fetchRegionDefault.
@@ -492,16 +443,22 @@ var ensureKnownCountryCode = async funct
     });
   }
 
   // If gInitialized is true then the search service was forced to perform
   // a sync initialization during our XHRs - capture this via telemetry.
   Services.telemetry.getHistogramById("SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT").add(gInitialized);
 };
 
+// Some notes on countryCode and region prefs:
+// * A "countryCode" pref is set via a geoip lookup.  It always reflects the
+//   result of that geoip request.
+// * A "region" pref, once set, is the region actually used for search.  In
+//   most cases it will be identical to the countryCode pref.
+
 // Store the result of the geoip request as well as any other values and
 // telemetry which depend on it.
 function storeCountryCode(cc) {
   // Set the country-code itself.
   Services.prefs.setCharPref("browser.search.countryCode", cc);
   // And set the region pref if we don't already have a value.
   if (!Services.prefs.prefHasUserValue("browser.search.region")) {
     Services.prefs.setCharPref("browser.search.region", cc);
@@ -2756,19 +2713,18 @@ SearchService.prototype = {
     if (!defaultEngine) {
       // We only allow the old defaultenginename pref for distributions
       // We can't use isPartnerBuild because we need to allow reading
       // of the defaultengine name pref for funnelcakes.
       if (distroID) {
         let defaultPrefB = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
         let nsIPLS = Ci.nsIPrefLocalizedString;
 
-        let defPref = getGeoSpecificPrefName("defaultenginename");
         try {
-          defaultEngine = defaultPrefB.getComplexValue(defPref, nsIPLS).data;
+          defaultEngine = defaultPrefB.getComplexValue("defaultenginename", nsIPLS).data;
         } catch (ex) {
           // If the default pref is invalid (e.g. an add-on set it to a bogus value)
           // use the default engine from the list.json.
           // This should eventually be the common case. We should only have the
           // defaultenginename pref for distributions.
           // Worst case, getEngineByName will just return null, which is the best we can do.
           defaultEngine = this._searchDefault;
         }
@@ -3611,19 +3567,18 @@ SearchService.prototype = {
             if (!engine || engine.name in addedEngines)
               continue;
 
             this.__sortedEngines.push(engine);
             addedEngines[engine.name] = engine;
           }
         } catch (e) { }
 
-        let prefNameBase = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order");
         while (true) {
-          prefName = prefNameBase + "." + (++i);
+          prefName = `${BROWSER_SEARCH_PREF}order.{++i}`;
           engineName = getLocalizedPref(prefName);
           if (!engineName)
             break;
 
           engine = this._engines[engineName];
           if (!engine || engine.name in addedEngines)
             continue;
 
@@ -3760,19 +3715,18 @@ SearchService.prototype = {
           if (!(engineName in engineOrder))
             engineOrder[engineName] = i++;
         }
       } catch (e) {
         LOG("Getting extra order prefs failed: " + e);
       }
 
       // Now look through the "browser.search.order" branch.
-      let prefNameBase = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order");
       for (var j = 1; ; j++) {
-        let prefName = prefNameBase + "." + j;
+        let prefName = `${BROWSER_SEARCH_PREF}order.${j}`;
         engineName = getLocalizedPref(prefName);
         if (!engineName)
           break;
 
         if (!(engineName in engineOrder))
           engineOrder[engineName] = i++;
       }
     }
@@ -4165,20 +4119,19 @@ SearchService.prototype = {
           try {
             if (result.name == Services.prefs.getCharPref(prefName)) {
               sendSubmissionURL = true;
               break;
             }
           } catch (e) {}
         }
 
-        let prefNameBase = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order");
         let i = 0;
         while (!sendSubmissionURL) {
-          let prefName = prefNameBase + "." + (++i);
+          let prefName = `${BROWSER_SEARCH_PREF}order.{++i}`;
           let engineName = getLocalizedPref(prefName);
           if (!engineName)
             break;
           if (result.name == engineName) {
             sendSubmissionURL = true;
             break;
           }
         }
--- a/toolkit/components/search/tests/xpcshell/head_search.js
+++ b/toolkit/components/search/tests/xpcshell/head_search.js
@@ -432,43 +432,16 @@ var addTestEngines = async function(aIte
 function installTestEngine() {
   useHttpServer();
   return addTestEngines([
     { name: kTestEngineName, xmlFileName: "engine.xml" },
   ]);
 }
 
 /**
- * Set a localized preference on the default branch
- * @param aPrefName
- *        The name of the pref to set.
- */
-function setLocalizedDefaultPref(aPrefName, aValue) {
-  let value = "data:text/plain," + BROWSER_SEARCH_PREF + aPrefName + "=" + aValue;
-  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF)
-          .setCharPref(aPrefName, value);
-}
-
-/**
- * Installs two test engines, sets them as default for US vs. general.
- */
-function setUpGeoDefaults() {
-  const kSecondTestEngineName = "A second test engine";
-
-  setLocalizedDefaultPref("defaultenginename", "Test search engine");
-  setLocalizedDefaultPref("defaultenginename.US", "A second test engine");
-
-  useHttpServer();
-  return addTestEngines([
-    { name: kTestEngineName, xmlFileName: "engine.xml" },
-    { name: kSecondTestEngineName, xmlFileName: "engine2.xml" },
-  ]);
-}
-
-/**
  * Returns a promise that is resolved when an observer notification from the
  * search service fires with the specified data.
  *
  * @param aExpectedData
  *        The value the observer notification sends that causes us to resolve
  *        the promise.
  */
 function waitForSearchNotification(aExpectedData) {
deleted file mode 100644
--- a/toolkit/components/search/tests/xpcshell/test_location_funnelcake.js
+++ /dev/null
@@ -1,13 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-add_task(async function run_test() {
-  Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "US"}');
-  // funnelcake builds start with "mozilla"
-  Services.prefs.getDefaultBranch("").setCharPref("distribution.id", "mozilla38");
-
-  await setUpGeoDefaults();
-  await asyncReInit();
-
-  equal(Services.search.defaultEngine.name, "A second test engine");
-});
deleted file mode 100644
--- a/toolkit/components/search/tests/xpcshell/test_location_partner.js
+++ /dev/null
@@ -1,12 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-add_task(async function run_test() {
-  Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "US"}');
-  Services.prefs.getDefaultBranch("").setCharPref("distribution.id", "partner-1");
-
-  await setUpGeoDefaults();
-  await asyncReInit();
-
-  equal(Services.search.defaultEngine.name, "Test search engine");
-});
--- a/toolkit/components/search/tests/xpcshell/xpcshell.ini
+++ b/toolkit/components/search/tests/xpcshell/xpcshell.ini
@@ -42,18 +42,16 @@ support-files =
 [test_json_cache.js]
 [test_list_json_locale.js]
 [test_list_json_searchdefault.js]
 [test_list_json_searchdefault_distro.js]
 [test_list_json_searchorder.js]
 [test_location.js]
 [test_location_error.js]
 [test_location_malformed_json.js]
-[test_location_partner.js]
-[test_location_funnelcake.js]
 [test_location_sync.js]
 [test_location_timeout.js]
 [test_location_timeout_xhr.js]
 [test_nodb_pluschanges.js]
 [test_save_sorted_engines.js]
 [test_pref.js]
 [test_purpose.js]
 [test_defaultEngine.js]