diff --git a/browser/modules/BrowserUsageTelemetry.jsm b/browser/modules/BrowserUsageTelemetry.jsm index 8316875a0061..78c26835f10a 100644 --- a/browser/modules/BrowserUsageTelemetry.jsm +++ b/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, }); }, diff --git a/toolkit/components/search/SearchEngine.jsm b/toolkit/components/search/SearchEngine.jsm index 0f94ffb2fd8a..26bbeba671e3 100644 --- a/toolkit/components/search/SearchEngine.jsm +++ b/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-: 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. * diff --git a/toolkit/components/search/SearchService.jsm b/toolkit/components/search/SearchService.jsm index ff2e84e37882..fbef50fd7b43 100644 --- a/toolkit/components/search/SearchService.jsm +++ b/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() { diff --git a/toolkit/components/search/nsISearchService.idl b/toolkit/components/search/nsISearchService.idl index 272fccb50a74..2adb75126938 100644 --- a/toolkit/components/search/nsISearchService.idl +++ b/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. diff --git a/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js b/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js index bfab6abc6f65..9db9bb1f28da 100644 --- a/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js +++ b/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}.` ); } } diff --git a/toolkit/components/search/tests/xpcshell/test_identifiers.js b/toolkit/components/search/tests/xpcshell/test_identifiers.js index 8e74c9d4053b..4d0428809b4c 100644 --- a/toolkit/components/search/tests/xpcshell/test_identifiers.js +++ b/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"); }); diff --git a/toolkit/components/search/tests/xpcshell/test_json_cache.js b/toolkit/components/search/tests/xpcshell/test_json_cache.js index 85dddde5bd7b..7cfd1b408b55 100644 --- a/toolkit/components/search/tests/xpcshell/test_json_cache.js +++ b/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); } diff --git a/toolkit/components/search/tests/xpcshell/test_json_cache_broken.js b/toolkit/components/search/tests/xpcshell/test_json_cache_broken.js index 32d1f83248ee..4f93271f512f 100644 --- a/toolkit/components/search/tests/xpcshell/test_json_cache_broken.js +++ b/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",