Bug 1605164 - Create nsISearchEngine.telemetryId and use that for telemetry rather than the identifier/other-name combo. r=daleharvey

This also bumps the cache version numbers, because we currently need to cache the telemetry id as part of the engine info.

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

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

browser/modules/BrowserUsageTelemetry.jsm

@@ -145,18 +145,6 @@ function getPinnedTabsCount() {
   return pinnedTabs;
 }
 
-function getSearchEngineId(engine) {
-  if (engine) {
-    if (engine.identifier) {
-      return engine.identifier;
-    }
-    if (engine.name) {
-      return "other-" + engine.name;
-    }
-  }
-  return "other";
-}
-
 function shouldRecordSearchCount(tabbrowser) {
   return (
     !PrivateBrowsingUtils.isWindowPrivate(tabbrowser.ownerGlobal) ||
@@ -444,7 +432,7 @@ let BrowserUsageTelemetry = {
       return;
     }
 
-    const countIdPrefix = getSearchEngineId(engine) + ".";
+    const countIdPrefix = `${engine.telemetryId}.`;
     const countIdSource = countIdPrefix + source;
     let histogram = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS");
 
@@ -489,7 +477,7 @@ let BrowserUsageTelemetry = {
       1
     );
     Services.telemetry.recordEvent("navigation", "search", source, action, {
-      engine: getSearchEngineId(engine),
+      engine: engine.telemetryId,
     });
   },
 

toolkit/components/search/SearchEngine.jsm

@@ -920,6 +920,8 @@ SearchEngine.prototype = {
   _isBuiltin: false,
   // The order hint from the configuration (if any).
   _orderHint: null,
+  // The telemetry id from the configuration (if any).
+  _telemetryId: null,
 
   /**
    * Retrieves the data from the engine's file asynchronously.
@@ -1498,6 +1500,7 @@ SearchEngine.prototype = {
     this._locale = params.locale;
     this._isBuiltin = !!params.isBuiltin;
     this._orderHint = params.orderHint;
+    this._telemetryId = params.telemetryId;
 
     this._initEngineURLFromMetaData(SearchUtils.URL_TYPE.SEARCH, {
       method: (params.searchPostParams && "POST") || params.method || "GET",
@@ -1770,6 +1773,7 @@ SearchEngine.prototype = {
     this._metaData = json._metaData || {};
     this._isBuiltin = json._isBuiltin;
     this._orderHint = json._orderHint || null;
+    this._telemetryId = json._telemetryId || null;
     if (json.filePath) {
       this._filePath = json.filePath;
     }
@@ -1810,6 +1814,7 @@ SearchEngine.prototype = {
       _urls: this._urls,
       _isBuiltin: this._isBuiltin,
       _orderHint: this._orderHint,
+      _telemetryId: this._telemetryId,
     };
 
     if (this._updateInterval) {
@@ -1863,6 +1868,21 @@ SearchEngine.prototype = {
     SearchUtils.notifyAction(this, SearchUtils.MODIFIED_TYPE.CHANGED);
   },
 
+  /**
+   * Returns the appropriate identifier to use for telemetry. It is based on
+   * the following order:
+   *
+   * - telemetryId: The telemetry id from the configuration.
+   * - identifier: The built-in identifier of app-provided engines.
+   * - other-<name>: The engine name prefixed by `other-` for non-app-provided
+   *                 engines.
+   *
+   * @returns {string}
+   */
+  get telemetryId() {
+    return this._telemetryId || this.identifier || `other-${this.name}`;
+  },
+
   /**
    * Return the built-in identifier of app-provided engines.
    *

toolkit/components/search/SearchService.jsm

@@ -529,7 +529,7 @@ SearchService.prototype = {
   // Current cache version. This should be incremented if the format of the cache
   // file is modified.
   get CACHE_VERSION() {
-    return gModernConfig ? 2 : 1;
+    return gModernConfig ? 4 : 3;
   },
 
   // The current status of initialization. Note that it does not determine if
@@ -2681,6 +2681,9 @@ SearchService.prototype = {
     if (locale != SearchUtils.DEFAULT_TAG) {
       shortName += "-" + locale;
     }
+    // TODO: Bug 1619656. We should no longer need to maintain the short name as
+    // the telemetry id. However, we need to check that this doesn't adversely
+    // affect settings or caches.
     if ("telemetryId" in engineParams && engineParams.telemetryId) {
       shortName = engineParams.telemetryId;
     }
@@ -2726,6 +2729,7 @@ SearchService.prototype = {
       suggestPostParams: suggestUrlPostParams,
       queryCharset: searchProvider.encoding || "UTF-8",
       mozParams,
+      telemetryId: engineParams.telemetryId,
       initEngine: engineParams.initEngine || false,
     };
 
@@ -3158,15 +3162,6 @@ SearchService.prototype = {
       name: engine.name ? engine.name : "",
     };
 
-    let shortName;
-    if (engine.identifier) {
-      shortName = engine.identifier;
-    } else if (engine.name) {
-      shortName = "other-" + engine.name;
-    } else {
-      shortName = "UNDEFINED";
-    }
-
     if (engine._isDefault) {
       engineData.origin = "default";
     } else {
@@ -3252,7 +3247,7 @@ SearchService.prototype = {
       engineData.submissionURL = uri.spec;
     }
 
-    return [shortName, engineData];
+    return [engine.telemetryId, engineData];
   },
 
   async getDefaultEngineInfo() {

toolkit/components/search/nsISearchService.idl

@@ -147,6 +147,11 @@ interface nsISearchEngine : nsISupports
    */
   readonly attribute AString searchForm;
 
+  /**
+   * The identifier to use for this engine when submitting to telemetry.
+   */
+  readonly attribute AString telemetryId;
+
   /**
    * An optional unique identifier for this search engine within the context of
    * the distribution, as provided by the distributing entity.

toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js

@@ -497,9 +497,9 @@ class SearchConfigTest {
       }
       if (rule.telemetryId) {
         this.assertEqual(
-          engine._shortName,
+          engine.telemetryId,
           rule.telemetryId,
-          `Should have the correct shortName ${location}.`
+          `Should have the correct telemetryId ${location}.`
         );
       }
     }

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

@@ -22,33 +22,43 @@ add_task(async function setup() {
   await installTestEngine();
 });
 
-function checkIdentifier(engineName, expectedIdentifier) {
-  let profileEngine = Services.search.getEngineByName(engineName);
+function checkIdentifier(engineName, expectedIdentifier, expectedTelemetryId) {
+  const engine = Services.search.getEngineByName(engineName);
   Assert.ok(
-    profileEngine instanceof Ci.nsISearchEngine,
+    engine instanceof Ci.nsISearchEngine,
     "Should be derived from nsISearchEngine"
   );
 
   Assert.equal(
-    profileEngine.identifier,
+    engine.identifier,
     expectedIdentifier,
     "Should have the correct identifier"
   );
+
+  Assert.equal(
+    engine.telemetryId,
+    expectedTelemetryId,
+    "Should have the correct telemetry Id"
+  );
 }
 
-add_task(async function test_identifier_from_profile() {
+add_task(async function test_from_profile() {
   // An engine loaded from the profile directory won't have an identifier,
   // because it's not built-in.
-  checkIdentifier(kTestEngineName, null);
+  checkIdentifier(kTestEngineName, null, `other-${kTestEngineName}`);
 });
 
-add_task(async function test_identifier_from_webextension_id() {
+add_task(async function test_from_telemetry_id() {
   // The telemetryId check isn't applicable to the legacy config.
-  checkIdentifier("basic", gModernConfig ? "telemetry" : "basic");
+  if (gModernConfig) {
+    checkIdentifier("basic", "telemetry", "telemetry");
+  } else {
+    checkIdentifier("basic", "basic", "basic");
+  }
 });
 
-add_task(async function test_identifier_from_telemetry_id() {
+add_task(async function test_from_webextension_id() {
   // If not specified, the telemetry Id is derived from the WebExtension prefix,
   // it should not use the WebExtension display name.
-  checkIdentifier("Simple Engine", "simple");
+  checkIdentifier("Simple Engine", "simple", "simple");
 });

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

@@ -40,9 +40,10 @@ add_task(async function setup() {
   engineTemplateFile.copyTo(engineFile.parent, "test-search-engine.xml");
 
   if (gModernConfig) {
-    cacheTemplate.version = 2;
+    cacheTemplate.version = 4;
     delete cacheTemplate.visibleDefaultEngines;
   } else {
+    cacheTemplate.version = 3;
     // The list of visibleDefaultEngines needs to match or the cache will be ignored.
     cacheTemplate.visibleDefaultEngines = getDefaultEngineList(false);
   }

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

@@ -23,7 +23,7 @@ const { getVerificationHash } = ChromeUtils.import(
 var cacheTemplate, appPluginsPath, profPlugins;
 
 const enginesCache = {
-  version: gModernConfig ? 2 : 1,
+  version: gModernConfig ? 4 : 3,
   buildID: "TBD",
   appVersion: "TBD",
   locale: "en-US",