Bug 1622864 - Avoid sanitizing a name for app-provided engines, and clarify the WebExtension id in the search engine schema. r=daleharvey
authorMark Banner <standard8@mozilla.com>
Thu, 26 Mar 2020 16:53:49 +0000
changeset 520606 69266001850728ca05430fce0cc06ce42c19507c
parent 520605 9de66850f37d43bb53046f1c17d63f871fb3a23f
child 520607 08ae58fc28daafa6b06368040866b736efea029b
push id37254
push usernerli@mozilla.com
push dateFri, 27 Mar 2020 04:48:07 +0000
treeherdermozilla-central@2d758b42bd73 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdaleharvey
bugs1622864
milestone76.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 1622864 - Avoid sanitizing a name for app-provided engines, and clarify the WebExtension id in the search engine schema. r=daleharvey makeEngineFromConfig currently initialises SearchEngine with the full name which is then sanitized, only for it to be overriden when calling _initFromMetadata. Being able to pass shortName directly, makes it clearer as to what is happening. By being explicit about if we're passing shortName or the display name, we can avoid the awkward sanitizeName option. Also updating the schema to provide validation of the WebExtension Id so that we have the format we expect there (which shortName is based on). Differential Revision: https://phabricator.services.mozilla.com/D68229
toolkit/components/search/SearchEngine.jsm
toolkit/components/search/SearchService.jsm
toolkit/components/search/schema/search-engine-config-schema.json
--- a/toolkit/components/search/SearchEngine.jsm
+++ b/toolkit/components/search/SearchEngine.jsm
@@ -742,43 +742,42 @@ EngineURL.prototype = {
 
 /**
  * nsISearchEngine constructor.
  *
  * @param {object} options
  *   The options for this search engine. At least one of options.name,
  *   options.fileURI or options.uri are required.
  * @param {string} [options.name]
- *   The short name to use for the search engine.
+ *   The name to base the short name of the engine on. This is typically the
+ *   display name where a pre-defined/sanitized short name is not available.
+ * @param {string} [options.shortName]
+ *   The short name to use for the engine. This should be known to match
+ *   the basic requirements in sanitizeName for a short name.
  * @param {nsIFile} [options.fileURI]
  *   The file URI that points to the search engine data.
  * @param {nsIURI|string} [options.uri]
  *   Represents the location of the search engine data file.
- * @param {boolean} [options.sanitizeName]
- *   Only applies when options.name is specified, will santize the name so
- *   it can be used as a file name, defaults to false.
  * @param {boolean} options.isBuiltin
  *   Indicates whether the engine is a app-provided or not. If it is, it will
  *   be treated as read-only.
  */
 function SearchEngine(options = {}) {
   if (!("isBuiltin" in options)) {
     throw new Error("isBuiltin missing from options.");
   }
   this._isBuiltin = options.isBuiltin;
   this._urls = [];
   this._metaData = {};
 
   let file, uri;
   if ("name" in options) {
-    if ("sanitizeName" in options && options.sanitizeName) {
-      this._shortName = sanitizeName(options.name);
-    } else {
-      this._shortName = options.name;
-    }
+    this._shortName = sanitizeName(options.name);
+  } else if ("shortName" in options) {
+    this._shortName = options.shortName;
   } else if ("fileURI" in options && options.fileURI instanceof Ci.nsIFile) {
     file = options.fileURI;
   } else if ("uri" in options) {
     let optionsURI = options.uri;
     if (typeof optionsURI == "string") {
       optionsURI = SearchUtils.makeURI(optionsURI);
     }
     // makeURI can return null if the URI is invalid.
--- a/toolkit/components/search/SearchService.jsm
+++ b/toolkit/components/search/SearchService.jsm
@@ -1789,17 +1789,17 @@ SearchService.prototype = {
           " built-in engines."
       );
     }
   },
 
   _loadEngineFromCache(json) {
     try {
       let engine = new SearchEngine({
-        name: json._shortName,
+        shortName: json._shortName,
         isBuiltin: !!json._isBuiltin,
       });
       engine._initWithJSON(json);
       this._addEngineToStore(engine);
     } catch (ex) {
       SearchUtils.log("Failed to load " + json._name + " from cache: " + ex);
       SearchUtils.log("Engine JSON: " + json.toSource());
     }
@@ -2516,17 +2516,16 @@ SearchService.prototype = {
           Cr.NS_ERROR_FILE_ALREADY_EXISTS
         );
       }
     }
 
     let newEngine = new SearchEngine({
       name,
       isBuiltin,
-      sanitizeName: true,
     });
     newEngine._initFromMetadata(name, params);
     newEngine._loadPath = "[other]addEngineWithDetails";
     if (params.extensionID) {
       newEngine._loadPath += ":" + params.extensionID;
     }
     if (isReload && this._engines.has(newEngine.name)) {
       newEngine._engineToUpdate = this._engines.get(newEngine.name);
@@ -2617,19 +2616,20 @@ SearchService.prototype = {
     let engineParams = await Services.search.getEngineParams(
       policy.extension,
       manifest,
       locale,
       params
     );
 
     let engine = new SearchEngine({
-      name: engineParams.name,
+      // No need to sanitize the name, as shortName uses the WebExtension id
+      // which should already be sanitized.
+      shortName: engineParams.shortName,
       isBuiltin: engineParams.isBuiltin,
-      sanitizeName: true,
     });
     engine._initFromMetadata(engineParams.name, engineParams);
     engine._loadPath = "[other]addEngineWithDetails";
     if (engineParams.extensionID) {
       engine._loadPath += ":" + engineParams.extensionID;
     }
     if (isReload && this._engines.has(engine.name)) {
       engine._engineToUpdate = this._engines.get(engine.name);
--- a/toolkit/components/search/schema/search-engine-config-schema.json
+++ b/toolkit/components/search/schema/search-engine-config-schema.json
@@ -262,17 +262,18 @@
     },
     "webExtension": {
       "type": "object",
       "title": "WebExtension",
       "properties": {
         "id": {
           "type": "string",
           "title": "WebExtension Id",
-          "description": "The identifier (local part) of the associated WebExtension"
+          "description": "The identifier (local part) of the associated WebExtension should be of the format example@search.mozilla.org",
+          "pattern": "^[a-z0-9-._]*@search.mozilla.org$"
         },
         "locale": {
           "type": "string",
           "title": "WebExtension Locale",
           "description": "Overrides the WebExtension locales and specifies to use a particular one. Ideally this should only be used when really necessary, otherwise considered deprecated."
         }
       }
     },