Bug 1598571 - Make MozParams be able to be updated dynamically, without needing a reinitialisation of the cache nor a reload. r=mikedeboer

This merges the previously two overlapping lists into one, with different classes to make obtaining the results easier.

As a side effect, preference based MozParams could now also be specified with a purpose.

Preference caching/observation to be added in the next changeset.

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

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

browser/components/search/test/browser/browser_amazon.js

@@ -68,7 +68,6 @@ add_task(async function test() {
               purpose: undefined,
             },
           ],
-          mozparams: {},
         },
         {
           type: "application/x-suggestions+json",

browser/components/search/test/browser/browser_bing.js

@@ -90,7 +90,6 @@ add_task(async function test() {
               purpose: "newtab",
             },
           ],
-          mozparams: {},
         },
         {
           type: "application/x-suggestions+json",

browser/components/search/test/browser/browser_ddg.js

@@ -84,7 +84,6 @@ add_task(async function test() {
               purpose: "newtab",
             },
           ],
-          mozparams: {},
         },
         {
           type: "application/x-suggestions+json",

browser/components/search/test/browser/browser_eBay.js

@@ -61,7 +61,6 @@ add_task(async function test() {
               purpose: undefined,
             },
           ],
-          mozparams: {},
         },
       ],
     },

toolkit/components/search/SearchEngine.jsm

@@ -299,38 +299,92 @@ function sanitizeName(name) {
 }
 
 /**
- * Retrieve a pref from the search param branch. Returns null if the
- * preference is not found.
- *
- * @param {string} prefName
- *   The name of the pref.
- * @returns {string|null}
- *   The value of the preference.
- **/
-function getMozParamPref(prefName) {
-  let branch = Services.prefs.getDefaultBranch(
-    SearchUtils.BROWSER_SEARCH_PREF + "param."
-  );
-  let prefValue = branch.getCharPref(prefName, null);
-  return prefValue ? encodeURIComponent(prefValue) : null;
+ * Represents a name/value pair for a parameter
+ * @see nsISearchEngine::addParam
+ */
+class QueryParameter {
+  /**
+   * @see nsISearchEngine::addParam
+   * @param {string} name
+   * @param {string} value
+   *  The value of the parameter. May be an empty string, must not be null or
+   *  undefined.
+   * @param {string} purpose
+   *   The search purpose for which matches when this parameter should be
+   *   applied, e.g. "searchbar", "contextmenu".
+   */
+  constructor(name, value, purpose) {
+    if (!name || value == null) {
+      SearchUtils.fail("missing name or value for QueryParameter!");
+    }
+
+    this.name = name;
+    this._value = value;
+    this.purpose = purpose;
+  }
+
+  get value() {
+    return this._value;
+  }
+
+  toJSON() {
+    const result = {
+      name: this.name,
+      value: this.value,
+    };
+    if (this.purpose) {
+      result.purpose = this.purpose;
+    }
+    return result;
+  }
 }
 
 /**
- * Simple object representing a name/value pair.
- * @see nsISearchEngine::addParam
- *
- * @param {string} name
- * @param {string} value
- * @param {string} purpose
+ * Represents a special paramater that can be set by preferences. The
+ * value is read from the 'browser.search.param.*' default preference
+ * branch.
  */
-function QueryParameter(name, value, purpose) {
-  if (!name || value == null) {
-    SearchUtils.fail("missing name or value for QueryParameter!");
+class QueryPreferenceParameter extends QueryParameter {
+  /**
+   * @param {string} name
+   *   The name of the parameter as injected into the query string.
+   * @param {string} prefName
+   *   The name of the preference to read from the branch.
+   * @param {string} purpose
+   *   The search purpose for which matches when this parameter should be
+   *   applied, e.g. `searchbar`, `contextmenu`.
+   */
+  constructor(name, prefName, purpose) {
+    super(name, prefName, purpose);
   }
 
-  this.name = name;
-  this.value = value;
-  this.purpose = purpose;
+  get value() {
+    const branch = Services.prefs.getDefaultBranch(
+      SearchUtils.BROWSER_SEARCH_PREF + "param."
+    );
+    const prefValue = branch.getCharPref(this._value, null);
+    return prefValue ? encodeURIComponent(prefValue) : null;
+  }
+
+  /**
+   * Converts the object to json. This object is converted with a mozparam flag
+   * as it gets written to the cache and hence we then know what type it is
+   * when reading it back.
+   *
+   * @returns {object}
+   */
+  toJSON() {
+    const result = {
+      condition: "pref",
+      mozparam: true,
+      name: this.name,
+      pref: this._value,
+    };
+    if (this.purpose) {
+      result.purpose = this.purpose;
+    }
+    return result;
+  }
 }
 
 /**
@@ -476,8 +530,6 @@ function EngineURL(mimeType, requestMethod, template, resultDomain) {
   this.method = method;
   this.params = [];
   this.rels = [];
-  // Don't serialize expanded mozparams
-  this.mozparams = {};
 
   var templateURI = SearchUtils.makeURI(template);
   if (!templateURI) {
@@ -512,11 +564,36 @@ EngineURL.prototype = {
     this.params.push(new QueryParameter(name, value, purpose));
   },
 
-  // Note: This method requires that aObj has a unique name or the previous MozParams entry with
-  // that name will be overwritten.
-  _addMozParam(obj) {
-    obj.mozparam = true;
-    this.mozparams[obj.name] = obj;
+  /**
+   * Adds a MozParam to the parameters list for this URL. For purpose based params
+   * these are saved as standard parameters, for preference based we save them
+   * as a special type.
+   *
+   * @param {object} param
+   * @param {string} param.name
+   *   The name of the parameter to add to the url.
+   * @param {string} [param.condition]
+   *   The type of parameter this is, e.g. "pref" for a preference parameter,
+   *   or "purpose" for a value-based parameter with a specific purpose. The
+   *   default is "purpose".
+   * @param {string} [param.value]
+   *   The value if it is a "purpose" parameter.
+   * @param {string} [param.purpose]
+   *   The purpose of the parameter for when it is applied, e.g. for `searchbar`
+   *   searches.
+   * @param {string} [param.pref]
+   *   The preference name of the parameter, that gets appended to
+   *   `browser.search.param.`.
+   */
+  _addMozParam(param) {
+    const purpose = param.purpose || undefined;
+    if (param.condition && param.condition == "pref") {
+      this.params.push(
+        new QueryPreferenceParameter(param.name, param.pref, purpose)
+      );
+    } else {
+      this.addParam(param.name, param.value || undefined, purpose);
+    }
   },
 
   getSubmission(searchTerms, engine, purpose) {
@@ -525,7 +602,10 @@ EngineURL.prototype = {
     var requestPurpose = purpose || "searchbar";
 
     // If a particular purpose isn't defined in the plugin, fallback to 'searchbar'.
-    if (!this.params.some(p => p.purpose && p.purpose == requestPurpose)) {
+    if (
+      requestPurpose != "searchbar" &&
+      !this.params.some(p => p.purpose && p.purpose == requestPurpose)
+    ) {
       requestPurpose = "searchbar";
     }
 
@@ -540,9 +620,13 @@ EngineURL.prototype = {
         continue;
       }
 
-      var value = ParamSubstitution(param.value, searchTerms, engine);
+      const paramValue = param.value;
+      // Preference MozParams might not have a preferenced saved, or a valid value.
+      if (paramValue != null) {
+        var value = ParamSubstitution(paramValue, searchTerms, engine);
 
-      dataArray.push(param.name + "=" + value);
+        dataArray.push(param.name + "=" + value);
+      }
     }
     let dataString = dataArray.join("&");
 
@@ -594,12 +678,6 @@ EngineURL.prototype = {
     for (let i = 0; i < json.params.length; ++i) {
       let param = json.params[i];
       if (param.mozparam) {
-        if (param.condition == "pref") {
-          let value = getMozParamPref(param.pref);
-          if (value) {
-            this.addParam(param.name, value);
-          }
-        }
         this._addMozParam(param);
       } else {
         this.addParam(param.name, param.value, param.purpose || undefined);
@@ -615,9 +693,10 @@ EngineURL.prototype = {
    */
   toJSON() {
     var json = {
-      template: this.template,
+      params: this.params,
       rels: this.rels,
       resultDomain: this.resultDomain,
+      template: this.template,
     };
 
     if (this.type != SearchUtils.URL_TYPE.SEARCH) {
@@ -627,11 +706,6 @@ EngineURL.prototype = {
       json.method = this.method;
     }
 
-    function collapseMozParams(param) {
-      return this.mozparams[param.name] || param;
-    }
-    json.params = this.params.map(collapseMozParams, this);
-
     return json;
   },
 };
@@ -1360,15 +1434,7 @@ SearchEngine.prototype = {
         if ((p.condition || p.purpose) && !this._isDefault) {
           continue;
         }
-        if (p.condition == "pref") {
-          let value = getMozParamPref(p.pref);
-          if (value) {
-            url.addParam(p.name, value);
-          }
-          url._addMozParam(p);
-        } else {
-          url.addParam(p.name, p.value, p.purpose || undefined);
-        }
+        url._addMozParam(p);
       }
     }
 
@@ -1502,7 +1568,6 @@ SearchEngine.prototype = {
         // We only support MozParams for default search engines
         this._isDefault
       ) {
-        var value;
         let condition = param.getAttribute("condition");
 
         // MozParams must have a condition to be valid
@@ -1518,6 +1583,10 @@ SearchEngine.prototype = {
           continue;
         }
 
+        // We can't make these both use _addMozParam due to the fallback
+        // handling - WebExtension parameters get treated as MozParams even
+        // if they are not, and hence don't have the condition parameter, so
+        // we can't warn for them.
         switch (condition) {
           case "purpose":
             url.addParam(
@@ -1525,14 +1594,8 @@ SearchEngine.prototype = {
               param.getAttribute("value"),
               param.getAttribute("purpose")
             );
-            // _addMozParam is not needed here since it can be serialized fine without. _addMozParam
-            // also requires a unique "name" which is not normally the case when @purpose is used.
             break;
           case "pref":
-            value = getMozParamPref(param.getAttribute("pref"), value);
-            if (value) {
-              url.addParam(param.getAttribute("name"), value);
-            }
             url._addMozParam({
               pref: param.getAttribute("pref"),
               name: param.getAttribute("name"),

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

@@ -6,7 +6,12 @@
 
 "use strict";
 
-function run_test() {
+const defaultBranch = Services.prefs.getDefaultBranch(
+  SearchUtils.BROWSER_SEARCH_PREF
+);
+const baseURL = "https://www.google.com/search?q=foo";
+
+add_task(async function setup() {
   // The test engines used in this test need to be recognized as 'default'
   // engines, or their MozParams will be ignored.
   let url = "resource://test/data/";
@@ -14,14 +19,9 @@ function run_test() {
     .getProtocolHandler("resource")
     .QueryInterface(Ci.nsIResProtocolHandler);
   resProt.setSubstitution("search-extensions", Services.io.newURI(url));
+});
 
-  run_next_test();
-}
-
-add_task(async function test_pref() {
-  let defaultBranch = Services.prefs.getDefaultBranch(
-    SearchUtils.BROWSER_SEARCH_PREF
-  );
+add_task(async function test_pref_initial_value() {
   defaultBranch.setCharPref("param.code", "good&id=unique");
   Services.prefs.setCharPref(
     SearchUtils.BROWSER_SEARCH_PREF + "param.code",
@@ -31,12 +31,37 @@ add_task(async function test_pref() {
   await AddonTestUtils.promiseStartupManager();
   await Services.search.init();
 
-  let engine = Services.search.getEngineByName("engine-pref");
-  let base = "https://www.google.com/search?q=foo&code=";
+  const engine = Services.search.getEngineByName("engine-pref");
+  const base = baseURL + "&code=";
   Assert.equal(
     engine.getSubmission("foo").uri.spec,
-    base + "good%26id%3Dunique"
+    base + "good%26id%3Dunique",
+    "Should have got the submission URL with the correct code"
+  );
+});
+
+add_task(async function test_pref_updated() {
+  // Update the pref without re-init nor restart.
+  defaultBranch.setCharPref("param.code", "supergood&id=unique123456");
+
+  const engine = Services.search.getEngineByName("engine-pref");
+  const base = baseURL + "&code=";
+  Assert.equal(
+    engine.getSubmission("foo").uri.spec,
+    base + "supergood%26id%3Dunique123456",
+    "Should have got the submission URL with the updated code"
+  );
+});
+
+add_task(async function test_pref_cleared() {
+  // Update the pref without re-init nor restart.
+  // Note you can't delete a preference from the default branch.
+  defaultBranch.setCharPref("param.code", "");
+
+  let engine = Services.search.getEngineByName("engine-pref");
+  Assert.equal(
+    engine.getSubmission("foo").uri.spec,
+    baseURL,
+    "Should have just the base URL after the pref was cleared"
   );
-
-  do_test_finished();
 });