Bug 1575554 - Support default region in search configuration. r=Standard8
authorDale Harvey <dale@arandomurl.com>
Tue, 17 Sep 2019 17:58:23 +0000
changeset 493641 3531d3ae4b585ce1fe313d078416708b09e80cd9
parent 493640 9fd2cf480008255cbf6f27fb03cdbc8ae8eb00ee
child 493642 cf5be9ebbf098aec010a06e9bfb5bb69d53b8953
push id95616
push userdharvey@mozilla.com
push dateTue, 17 Sep 2019 20:14:36 +0000
treeherderautoland@3531d3ae4b58 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8
bugs1575554
milestone71.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1575554 - Support default region in search configuration. r=Standard8 Differential Revision: https://phabricator.services.mozilla.com/D45537
browser/components/search/extensions/engines.json
toolkit/components/search/SearchEngineSelector.jsm
toolkit/components/search/SearchService.jsm
toolkit/components/search/docs/SearchEngineConfiguration.rst
toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js
toolkit/components/search/tests/xpcshell/test_engine_selector.js
--- a/browser/components/search/extensions/engines.json
+++ b/browser/components/search/extensions/engines.json
@@ -48,16 +48,18 @@
               "lt", "mai", "mk", "ml", "ms", "my", "nb-NO", "ne-NP", "nl",
               "nn-NO", "oc", "or", "pa-IN", "pt-BR", "rm", "ro", "son",
               "sq", "sr", "sv-SE", "th", "tl", "trs", "uk", "ur", "uz",
               "wo", "xh", "zh-CN"
             ],
             "startsWith": ["bn", "en"]
           }
         }
+      }, {
+        "included": { "regions": ["default"] }
       }],
       "aliases": ["@bing"]
     },
     {
       "engineName": "\u767E\u5EA6",
       "webExtensionId": "baidu@search.mozilla.org",
       "webExtensionVersion": "1.0",
       "appliesTo": [{
@@ -72,16 +74,18 @@
       "aliases": ["@\u767E\u5EA6", "@baidu"]
     },
     {
       "engineName": "Amazon.com",
       "orderHint": 500,
       "webExtensionId": "amazondotcom@search.mozilla.org",
       "webExtensionVersion": "1.1",
       "appliesTo": [{
+        "included": { "regions": ["default"] }
+      }, {
         "included": {
           "locales": {
             "matches": [
               "ach", "af", "ar", "az", "bg", "cak", "en-US", "eo",
               "es-AR", "fa", "gn", "hy-AM", "ia", "is", "ka", "km",
               "lt", "mk", "ms", "my", "ro", "si", "th", "tl",
               "trs", "uz"
             ]
@@ -234,16 +238,18 @@
       "aliases": ["amazon"]
     },
     {
       "engineName": "eBay",
       "orderHint": 500,
       "webExtensionId": "ebay@search.mozilla.org",
       "webExtensionVersion": "1.0",
       "appliesTo": [{
+        "included": { "regions": ["default"] }
+      }, {
         "included": { "locales": { "matches": ["en-US"] } },
         "excluded": { "regions": ["ru", "tr", "by", "kz"] }
       }, {
         "included": {
           "locales": { "matches": ["en-US"] },
           "regions": ["au"]
         },
         "webExtensionLocales": ["au"]
@@ -747,20 +753,20 @@
       "appliesTo": [{ "included": { "locales": { "matches": ["ga-IE"]}}}]
     },
     {
       "engineName": "twitter",
       "webExtensionId": "twitter@search.mozilla.org",
       "webExtensionVersion": "1.0",
       "appliesTo": [{
         "included": { "locales": { "matches": [
-          "en-US", "ach", "an", "bs", "ca", "ca-valencia", "crh",
-          "en-CA", "ga-IE", "gn", "hr", "ia", "ka", "kk", "km", "lo",
-          "lt", "mai",  "ms", "my", "ne-NP", "oc", "pt-BR", "sl", "tl",
-          "tr", "ur", "uz", "wo"
+          "default", "en-US", "ach", "an", "bs", "ca", "ca-valencia",
+          "crh", "en-CA", "ga-IE", "gn", "hr", "ia", "ka", "kk", "km",
+          "lo", "lt", "mai",  "ms", "my", "ne-NP", "oc", "pt-BR",
+          "sl", "tl", "tr", "ur", "uz", "wo"
         ]}}
       }, {
         "included": { "locales": { "matches": ["ja-JP-mac", "ja"]}},
         "webExtensionLocales": ["ja"]
       }]
     },
     {
       "engineName": "tyda-sv-SE",
@@ -810,35 +816,32 @@
       "webExtensionVersion": "1.2",
       "appliesTo": [{ "included": { "locales": { "matches": ["sk"]}}}]
     },
     {
       "engineName": "wikipedia",
       "webExtensionId": "wikipedia@search.mozilla.org",
       "webExtensionVersion": "1.0",
       "appliesTo": [ {
+        "included": { "everywhere": true }
+      }, {
         "included": { "locales": { "matches": [
           "af", "an", "ar", "as", "ast", "az", "be", "bg",
           "br", "bs", "crh", "cy", "da", "de", "dsb",
           "el", "eo", "et", "eu", "fa", "fi", "fy-NL", "ga-IE",
           "gd", "gl", "gn", "he", "hr", "hsb", "hu", "ia",
           "id", "is", "it", "ka", "kab", "kk", "km", "kn",
           "lij", "lo", "lt", "ltg", "lv", "mk", "ml", "mr",
           "ms", "my", "nl", "oc", "or", "pl", "rm", "ro", "ru",
           "si", "sk", "sl", "sq", "sr", "sv-SE", "ta", "te",
           "th", "tl", "tr", "uk", "ur", "uz", "vi", "wo",
           "zh-CN", "zh-TW"
         ]}},
         "webExtensionLocales": ["$USER_LOCALE"]
       }, {
-        "included": { "locales": { "matches": [
-          "en-CA", "en-GB", "en-US", "en-ZA", "ach", "xh"
-          ]}},
-          "webExtensionLocales": ["en"]
-      }, {
         "included": { "locales": { "matches": [ "be" ] } },
         "webExtensionLocales": ["be", "be-tarask"]
       }, {
         "included": { "locales": { "matches": [ "bn", "bn-BD", "bn-IN" ] } },
         "webExtensionLocales": ["bn"]
       }, {
         "included": { "locales": { "matches": [ "ca", "ca-valencia" ] } },
         "webExtensionLocale": "ca"
--- a/toolkit/components/search/SearchEngineSelector.jsm
+++ b/toolkit/components/search/SearchEngineSelector.jsm
@@ -42,26 +42,23 @@ class SearchEngineSelector {
    * @param {string} url - Location of the configuration.
    */
   async getEngineConfiguration(url) {
     const response = await fetch(url);
     return (await response.json()).data;
   }
 
   /**
+   * @param {string} locale - Users locale.
    * @param {string} region - Users region.
-   * @param {string} locale - Users locale.
    * @returns {object} result - An object with "engines" field, a sorted
    *   list of engines and optionally "privateDefault" indicating the
    *   name of the engine which should be the default in private.
    */
-  fetchEngineConfiguration(region, locale) {
-    if (!region || !locale) {
-      throw new Error("region and locale parameters required");
-    }
+  fetchEngineConfiguration(locale, region = "default") {
     log(`fetchEngineConfiguration ${region}:${locale}`);
     let cohort = Services.prefs.getCharPref("browser.search.cohort", null);
     let engines = [];
     for (let config of this.configuration) {
       const appliesTo = config.appliesTo || [];
       const applies = appliesTo.filter(section => {
         let included =
           "included" in section &&
--- a/toolkit/components/search/SearchService.jsm
+++ b/toolkit/components/search/SearchService.jsm
@@ -1659,25 +1659,22 @@ SearchService.prototype = {
     return engines;
   },
 
   async _findEngineSelectorEngines() {
     SearchUtils.log("_findEngineSelectorEngines: init");
 
     let engineSelector = new SearchEngineSelector();
     let locale = Services.locale.appLocaleAsBCP47;
-    // TODO: engineSelector needs to support default region, we cant
-    // just use "us" as a default region.
-    // https://bugzilla.mozilla.org/show_bug.cgi?id=1575554
-    let region = Services.prefs.getCharPref("browser.search.region", "us");
+    let region = Services.prefs.getCharPref("browser.search.region", "default");
 
     await engineSelector.init();
     let { engines, privateDefault } = engineSelector.fetchEngineConfiguration(
-      region,
-      locale
+      locale,
+      region
     );
 
     this._searchDefault = engines[0].engineName;
     this._searchOrder = engines.map(e => e.engineName);
     if (privateDefault) {
       this._searchPrivateDefault = privateDefault;
     }
     return engines;
--- a/toolkit/components/search/docs/SearchEngineConfiguration.rst
+++ b/toolkit/components/search/docs/SearchEngineConfiguration.rst
@@ -107,16 +107,19 @@ located specific regions or with certain
 
 In this case users identified as being in the US region would use the WebExtension
 with identifier ``webext-engine1``, version 1.1. GB region users would get
 ``webext-gb`` version 1.2, and all other users would get ``webext`` version 1.0.
 
 Special Attributes
 ------------------
 
+$USER_LOCALE
+------------
+
 If a ``webExtensionLocales`` attribute contains an element with the value
 ``"$USER_LOCALE"`` then the special value will be replaced in the
 configuration object with the users locale. For example:
 
 .. code-block:: js
 
     "engine1": {
       "webExtensionId": "webext",
@@ -127,16 +130,22 @@ configuration object with the users loca
             "matches": ["us", "gb"]
           },
           "webExtensionLocales": ["$USER_LOCALE"],
         },
 
 Will report either ``[us]`` or ``[gb]`` as the ``webExtensionLocales``
 depending on the user.
 
+"default"
+---------
+
+You can specify ``"default"`` as a region in the configuration if
+the engine is to be included when no region is specified.
+
 Experiments
 -----------
 
 We can run experiments by giving sections within ``appliesTo`` a
 ``cohort`` value, the Search Service can then optionally pass in a
 matching ``cohort`` value to match those sections.
 
 Sections which have a ``cohort`` will not be used unless a matching
--- a/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js
+++ b/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js
@@ -158,18 +158,18 @@ class SearchConfigTest {
       "Should not have test debug turned on in production"
     );
   }
 
   async _getEngines(useEngineSelector, region, locale) {
     if (useEngineSelector) {
       let engines = [];
       let configs = await engineSelector.fetchEngineConfiguration(
-        region,
-        locale
+        locale,
+        region
       );
       for (let config of configs.engines) {
         let engine = await this._getExtensionEngine(config);
         engines.push(engine);
       }
       return engines;
     }
     return Services.search.getVisibleEngines();
--- a/toolkit/components/search/tests/xpcshell/test_engine_selector.js
+++ b/toolkit/components/search/tests/xpcshell/test_engine_selector.js
@@ -39,16 +39,19 @@ const CONFIG_URL =
       {
         engineName: "altavista",
         orderHint: 2000,
         defaultPrivate: "yes",
         appliesTo: [
           {
             included: { locales: { matches: ["en-US"] } },
           },
+          {
+            included: { regions: ["default"] },
+          },
         ],
       },
       {
         engineName: "excite",
         default: "yes-if-no-other",
         appliesTo: [
           {
             included: { everywhere: true },
@@ -67,48 +70,62 @@ const CONFIG_URL =
   });
 
 const engineSelector = new SearchEngineSelector();
 
 add_task(async function() {
   await engineSelector.init(CONFIG_URL);
 
   let { engines, privateDefault } = engineSelector.fetchEngineConfiguration(
-    "us",
-    "en-US"
+    "en-US",
+    "us"
   );
   Assert.equal(
     privateDefault,
     "altavista",
     "Should set altavista as privateDefault"
   );
   let names = engines.map(obj => obj.engineName);
   Assert.deepEqual(names, ["lycos", "altavista", "aol"], "Correct order");
   Assert.deepEqual(
     engines[2].webExtensionLocales,
     ["en-US"],
     "Subsequent matches in applies to can override default"
   );
 
   ({ engines, privateDefault } = engineSelector.fetchEngineConfiguration(
-    "kz",
-    "zh-CN"
+    "zh-CN",
+    "kz"
   ));
   Assert.equal(engines.length, 2, "Correct engines are returns");
   Assert.equal(privateDefault, null, "There should be no privateDefault");
   names = engines.map(obj => obj.engineName);
   Assert.deepEqual(
     names,
     ["excite", "aol"],
     "The engines should be in the correct order"
   );
 
   Services.prefs.setCharPref("browser.search.cohort", "acohortid");
   ({ engines, privateDefault } = engineSelector.fetchEngineConfiguration(
-    "us",
-    "en-US"
+    "en-US",
+    "us"
   ));
   Assert.deepEqual(
     engines.map(obj => obj.engineName),
     ["lycos", "altavista", "aol", "excite"],
     "Engines are in the correct order and include the cohort engine"
   );
+
+  ({ engines, privateDefault } = engineSelector.fetchEngineConfiguration(
+    "en-US"
+  ));
+  Assert.deepEqual(
+    engines.map(obj => obj.engineName),
+    ["lycos", "altavista", "aol", "excite"],
+    "The engines should be in the correct order"
+  );
+  Assert.equal(
+    privateDefault,
+    "altavista",
+    "Should set altavista as privateDefault"
+  );
 });