Bug 1577733 - Change SearchService's _searchOrder to reference engines by ascii identifier instead of display names for the modern config. r=mikedeboer" -a

Differential Revision: https://phabricator.services.mozilla.com/D50296

--HG--
extra : moz-landing-system : lando

toolkit/components/search/SearchService.jsm

@@ -635,6 +635,10 @@ SearchService.prototype = {
 
   /**
    * The suggested order of engines from the configuration.
+   * For modern configuration:
+   *   This is an array of objects containing the WebExtension ID and Locale.
+   * For legacy configuration:
+   *   This is an array of strings which are the display name of the engines.
    */
   _searchOrder: [],
 
@@ -1777,7 +1781,13 @@ SearchService.prototype = {
           ? defaultEngine.webExtensionLocales[0]
           : DEFAULT_TAG,
     };
-    this._searchOrder = engines.map(e => e.engineName);
+    this._searchOrder = engines.map(e => {
+      return {
+        id: e.webExtensionId,
+        locale:
+          "webExtensionLocales" in e ? e.webExtensionLocales[0] : DEFAULT_TAG,
+      };
+    });
     if (privateDefault) {
       this._searchPrivateDefault = {
         id: privateDefault.webExtensionId,
@@ -2145,14 +2155,26 @@ SearchService.prototype = {
         }
       }
 
-      for (let engineName of this._searchOrder) {
-        let engine = this._engines.get(engineName);
-        if (!engine || engine.name in addedEngines) {
-          continue;
-        }
+      if (gModernConfig) {
+        for (const details of this._searchOrder) {
+          let engine = this._getEngineByWebExtensionDetails(details);
+          if (!engine || engine.name in addedEngines) {
+            continue;
+          }
 
-        this.__sortedEngines.push(engine);
-        addedEngines[engine.name] = engine;
+          this.__sortedEngines.push(engine);
+          addedEngines[engine.name] = engine;
+        }
+      } else {
+        for (let engineName of this._searchOrder) {
+          let engine = this._engines.get(engineName);
+          if (!engine || engine.name in addedEngines) {
+            continue;
+          }
+
+          this.__sortedEngines.push(engine);
+          addedEngines[engine.name] = engine;
+        }
       }
     }
 
@@ -2296,8 +2318,17 @@ SearchService.prototype = {
     }
 
     // Now look at list.json
-    for (let engineName of this._searchOrder) {
-      engineOrder[engineName] = i++;
+    if (gModernConfig) {
+      for (const details of this._searchOrder) {
+        const engine = this._getEngineByWebExtensionDetails(details);
+        if (engine) {
+          engineOrder[engine.name] = i++;
+        }
+      }
+    } else {
+      for (let engineName of this._searchOrder) {
+        engineOrder[engineName] = i++;
+      }
     }
 
     SearchUtils.log(

toolkit/components/search/nsISearchService.idl

@@ -384,6 +384,7 @@ interface nsISearchService : nsISupports
 
   /**
    * Returns an array of all installed search engines.
+   * The array is sorted either to the user requirements or the default order.
    *
    * @returns an array of nsISearchEngine objects.
    */
@@ -392,6 +393,7 @@ interface nsISearchService : nsISupports
   /**
    * Returns an array of all installed search engines whose hidden attribute is
    * false.
+   * The array is sorted either to the user requirements or the default order.
    *
    * @returns an array of nsISearchEngine objects.
    */
@@ -400,6 +402,7 @@ interface nsISearchService : nsISupports
   /**
    * Returns an array of all default search engines. This includes all loaded
    * engines that aren't in the user's profile directory.
+   * The array is sorted to the default order.
    *
    * @returns an array of nsISearchEngine objects.
    */

toolkit/components/search/tests/xpcshell/test_getDefaultEngines.js

@@ -0,0 +1,123 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/* Check the correct default engines are picked from the configuration list. */
+
+"use strict";
+
+const modernConfig = Services.prefs.getBoolPref(
+  SearchUtils.BROWSER_SEARCH_PREF + "modernConfig",
+  false
+);
+
+// With modern configuration, we have a slightly different order, since the
+// default engines will get place first, regardless of the specified orders
+// of the other engines.
+const EXPECTED_ORDER = modernConfig
+  ? [
+      // Default engines
+      "Test search engine",
+      "engine-pref",
+      // Two engines listed in searchOrder.
+      "engine-resourceicon",
+      "engine-chromeicon",
+      // Rest of the engines in order.
+      "engine-rel-searchform-purpose",
+      "Test search engine (Reordered)",
+    ]
+  : [
+      // Two engines listed in searchOrder.
+      "engine-resourceicon",
+      "engine-chromeicon",
+      // Rest of the engines in alphabetical order.
+      "engine-pref",
+      "engine-rel-searchform-purpose",
+      "Test search engine",
+      "Test search engine (Reordered)",
+    ];
+
+add_task(async function setup() {
+  await AddonTestUtils.promiseStartupManager();
+
+  useTestEngineConfig();
+
+  Services.prefs.setBoolPref(
+    SearchUtils.BROWSER_SEARCH_PREF + "separatePrivateDefault",
+    true
+  );
+});
+
+async function checkOrder(expectedOrder) {
+  const sortedEngines = await Services.search.getDefaultEngines();
+  Assert.deepEqual(
+    sortedEngines.map(s => s.name),
+    expectedOrder,
+    "Should have the expected engine order"
+  );
+}
+
+add_task(async function test_getDefaultEngines_no_others() {
+  await checkOrder(EXPECTED_ORDER);
+}).skip();
+
+add_task(async function test_getDefaultEngines_with_non_builtins() {
+  await Services.search.addEngineWithDetails("nonbuiltin1", {
+    method: "get",
+    template: "http://example.com/?search={searchTerms}",
+  });
+
+  // We should still have the same built-in engines listed.
+  await checkOrder(EXPECTED_ORDER);
+}).skip();
+
+add_task(async function test_getDefaultEngines_with_distro() {
+  Services.prefs.getDefaultBranch("distribution.").setCharPref("id", "test");
+  Services.prefs.setCharPref(
+    SearchUtils.BROWSER_SEARCH_PREF + "order.extra.bar",
+    "engine-pref"
+  );
+  Services.prefs.setCharPref(
+    SearchUtils.BROWSER_SEARCH_PREF + "order.extra.foo",
+    "engine-resourceicon"
+  );
+  let localizedStr = Cc["@mozilla.org/pref-localizedstring;1"].createInstance(
+    Ci.nsIPrefLocalizedString
+  );
+  localizedStr.data = "engine-rel-searchform-purpose";
+  Services.prefs.setComplexValue(
+    SearchUtils.BROWSER_SEARCH_PREF + "order.1",
+    Ci.nsIPrefLocalizedString,
+    localizedStr
+  );
+  localizedStr = Cc["@mozilla.org/pref-localizedstring;1"].createInstance(
+    Ci.nsIPrefLocalizedString
+  );
+  localizedStr.data = "engine-chromeicon";
+  Services.prefs.setComplexValue(
+    SearchUtils.BROWSER_SEARCH_PREF + "order.2",
+    Ci.nsIPrefLocalizedString,
+    localizedStr
+  );
+
+  // TODO: Bug 1590860. The order of the lists is wrong - the order prefs should
+  // be overriding it, but they don't because the code doesn't work right.
+  const expected = modernConfig
+    ? [
+        "Test search engine",
+        "engine-pref",
+        "engine-resourceicon",
+        "engine-chromeicon",
+        "engine-rel-searchform-purpose",
+        "Test search engine (Reordered)",
+      ]
+    : [
+        "engine-pref",
+        "engine-rel-searchform-purpose",
+        "engine-resourceicon",
+        "engine-chromeicon",
+        "Test search engine",
+        "Test search engine (Reordered)",
+      ];
+
+  await checkOrder(expected);
+});

toolkit/components/search/tests/xpcshell/xpcshell-common.ini

@@ -19,6 +19,7 @@ skip-if = true # Is confusing
 [test_engine_selector.js]
 [test_engine_set_alias.js]
 [test_engineUpdate.js]
+[test_getDefaultEngines.js]
 [test_hasEngineWithURL.js]
 [test_identifiers.js]
 [test_ignorelist_update.js]