From fe6fdaf91ce575a26dd0662c425619f3d2c5c12c Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Fri, 8 Nov 2019 12:04:02 +0000 Subject: [PATCH] Bug 1583214 - Support ESR settings in the modern search configuration. r=mikedeboer Differential Revision: https://phabricator.services.mozilla.com/D52041 --HG-- extra : moz-landing-system : lando --- .../components/search/extensions/engines.json | 52 ++++++++ .../test/browser/browser_google_behavior.js | 5 + .../search/SearchEngineSelector.jsm | 22 ++-- toolkit/components/search/SearchService.jsm | 6 +- .../search/docs/SearchConfigurationSchema.rst | 55 ++++++++ .../schema/search-engine-config-schema.json | 9 +- .../searchconfigs/head_searchconfig.js | 6 +- .../xpcshell/searchconfigs/test_google.js | 4 - .../test_engine_selector_application.js | 118 ++++++++++++++++++ .../xpcshell/test_engine_selector_order.js | 3 +- .../search/tests/xpcshell/xpcshell-common.ini | 1 + 11 files changed, 261 insertions(+), 20 deletions(-) create mode 100644 toolkit/components/search/tests/xpcshell/test_engine_selector_application.js diff --git a/browser/components/search/extensions/engines.json b/browser/components/search/extensions/engines.json index fbb77004c698..8c422bdb0fc9 100644 --- a/browser/components/search/extensions/engines.json +++ b/browser/components/search/extensions/engines.json @@ -14,6 +14,16 @@ "appliesTo": [{ "included": { "everywhere": true }, "default": "yes" + }, { + "included": { "everywhere": true }, + "application": { + "channel": ["esr"] + }, + "searchUrlGetParams": { + "client": "firefox-b-e", + "q": "{searchTerms}" + }, + "telemetryId": "google-b-e" }, { "included": { "regions": ["ru", "tr", "by", "kz"], @@ -24,6 +34,23 @@ "telemetryId": "google" }, "default": "no" + }, { + "included": { + "regions": ["ru", "tr", "by", "kz"], + "locales": { + "matches": ["ru", "tr", "be", "kk"], + "startsWith": ["en"] + }, + "telemetryId": "google" + }, + "application": { + "channel": ["esr"] + }, + "searchUrlGetParams": { + "client": "firefox-b-e", + "q": "{searchTerms}" + }, + "default": "no" }, { "included": { "regions": ["cn"], @@ -32,6 +59,21 @@ } }, "default": "no" + }, { + "included": { + "regions": ["cn"], + "locales": { + "matches": ["zh-CN"] + } + }, + "application": { + "channel": ["esr"] + }, + "searchUrlGetParams": { + "client": "firefox-b-e", + "q": "{searchTerms}" + }, + "default": "no" }, { "included": { "regions": ["us"] }, "searchUrlGetParams": { @@ -39,6 +81,16 @@ "q": "{searchTerms}" }, "telemetryId": "google-b-1-d" + }, { + "included": { "regions": ["us"] }, + "application": { + "channel": ["esr"] + }, + "searchUrlGetParams": { + "client": "firefox-b-1-e", + "q": "{searchTerms}" + }, + "telemetryId": "google-b-1-e" }] }, { diff --git a/browser/components/search/test/browser/browser_google_behavior.js b/browser/components/search/test/browser/browser_google_behavior.js index e38fb9567a5d..6c5241134a47 100644 --- a/browser/components/search/test/browser/browser_google_behavior.js +++ b/browser/components/search/test/browser/browser_google_behavior.js @@ -5,6 +5,11 @@ * Test Google search plugin URLs * TODO: This test is a near duplicate of browser_searchEngine_behaviors.js but * specific to Google. This is required due to bug 1315953. + * + * Note: Although we have tests for codes in + * toolkit/components/tests/xpcshell/searchconfigs, we also need this test as an + * integration test to check the search service to selector integration is + * working correctly (especially the ESR codes). */ "use strict"; diff --git a/toolkit/components/search/SearchEngineSelector.jsm b/toolkit/components/search/SearchEngineSelector.jsm index ef5c61d9ce71..449c8f4c712c 100644 --- a/toolkit/components/search/SearchEngineSelector.jsm +++ b/toolkit/components/search/SearchEngineSelector.jsm @@ -49,13 +49,14 @@ class SearchEngineSelector { /** * @param {string} locale - Users locale. * @param {string} region - Users region. + * @param {string} channel - The update channel the application is running on. * @returns {object} * An object with "engines" field, a sorted list of engines and - * optionally "privateDefault" which is an object continaing the engine - * details for the engine which should be the default in private mode. + * optionally "privateDefault" which is an object containing the engine + * details for the engine which should be the default in Private Browsing mode. */ - fetchEngineConfiguration(locale, region = "default") { - log(`fetchEngineConfiguration ${region}:${locale}`); + fetchEngineConfiguration(locale, region, channel) { + log(`fetchEngineConfiguration ${region}:${locale}:${channel}`); let cohort = Services.prefs.getCharPref("browser.search.cohort", null); let engines = []; const lcLocale = locale.toLowerCase(); @@ -63,15 +64,22 @@ class SearchEngineSelector { for (let config of this.configuration) { const appliesTo = config.appliesTo || []; const applies = appliesTo.filter(section => { + if ("cohort" in section && cohort != section.cohort) { + return false; + } + if ( + "application" in section && + "channel" in section.application && + !section.application.channel.includes(channel) + ) { + return false; + } let included = "included" in section && this._isInSection(lcRegion, lcLocale, section.included); let excluded = "excluded" in section && this._isInSection(lcRegion, lcLocale, section.excluded); - if ("cohort" in section && cohort != section.cohort) { - return false; - } return included && !excluded; }); diff --git a/toolkit/components/search/SearchService.jsm b/toolkit/components/search/SearchService.jsm index c13ae930687c..9c80bb0fb424 100644 --- a/toolkit/components/search/SearchService.jsm +++ b/toolkit/components/search/SearchService.jsm @@ -1768,9 +1768,13 @@ SearchService.prototype = { let region = Services.prefs.getCharPref("browser.search.region", "default"); await engineSelector.init(); + let channel = AppConstants.MOZ_APP_VERSION_DISPLAY.endsWith("esr") + ? "esr" + : AppConstants.MOZ_UPDATE_CHANNEL; let { engines, privateDefault } = engineSelector.fetchEngineConfiguration( locale, - region + region, + channel ); const defaultEngine = engines[0]; diff --git a/toolkit/components/search/docs/SearchConfigurationSchema.rst b/toolkit/components/search/docs/SearchConfigurationSchema.rst index 258a29f82580..3be24aac1dd6 100644 --- a/toolkit/components/search/docs/SearchConfigurationSchema.rst +++ b/toolkit/components/search/docs/SearchConfigurationSchema.rst @@ -128,6 +128,61 @@ depending on the user's locale. You can specify ``"default"`` as a region in the configuration if the engine is to be included when no region is specified. +Application Scoping +=================== + +An engine configuration may be scoped to a particular application. + +Channel +======= + +One or more channels may be specified in an array to restrict a configuration +to just those channels. The current known channels are: + + - default: Self-builds of Firefox, or possibly some self-distributed versions. + - nightly: Firefox Nightly builds. + - aurora: Firefox Developer Edition + - beta: Firefox Beta + - release: The main Firefox release channel. + - esr: The ESR Channel. This will also match versions of Firefox where the + displayed version number includes ``esr``. We do this to include Linux + distributions and other manual builds of ESR. + +In the following example, ``web@ext`` would be set as default on the default +channel only, whereas ``web1@ext`` would be set as default on release and esr +channels. + +.. code-block:: js + + { + "webExtension": { + "id": "web@ext" + }, + "appliesTo": [{ + "included": { + "everywhere": true + "default": "yes", + "application": { + "channel": ["default"] + } + } + ]} + }, + { + "webExtension": { + "id": "web1@ext" + }, + "appliesTo": [{ + "included": { + "everywhere": true + "default": "yes", + "application": { + "channel": ["release", "esr"] + } + } + ]} + } + Experiments =========== diff --git a/toolkit/components/search/schema/search-engine-config-schema.json b/toolkit/components/search/schema/search-engine-config-schema.json index 1c2bda4e8aae..d853d62839c9 100644 --- a/toolkit/components/search/schema/search-engine-config-schema.json +++ b/toolkit/components/search/schema/search-engine-config-schema.json @@ -10,9 +10,6 @@ "defaultPrivate": { "$ref": "#/definitions/defaultPrivate" }, - "application": { - "$ref": "#/definitions/application" - }, "orderHint": { "$ref": "#/definitions/orderHint" }, @@ -51,10 +48,10 @@ "android" ] }, - "branches": { + "channel": { "type": "array", - "title": "Branches", - "description": "Which branches this belongs to (not set = everywhere)", + "title": "Channel", + "description": "Which channel this belongs to (not set = everywhere). For ESR this is also keyed from the display version.", "items": { "type": "string", "enum": [ diff --git a/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js b/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js index a30056e6aac2..ee08448c30e0 100644 --- a/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js +++ b/toolkit/components/search/tests/xpcshell/searchconfigs/head_searchconfig.js @@ -16,6 +16,7 @@ const { XPCOMUtils } = ChromeUtils.import( XPCOMUtils.defineLazyModuleGetters(this, { AddonManager: "resource://gre/modules/AddonManager.jsm", AddonTestUtils: "resource://testing-common/AddonTestUtils.jsm", + AppConstants: "resource://gre/modules/AppConstants.jsm", ObjectUtils: "resource://gre/modules/ObjectUtils.jsm", OS: "resource://gre/modules/osfile.jsm", SearchEngine: "resource://gre/modules/SearchEngine.jsm", @@ -168,7 +169,10 @@ class SearchConfigTest { let engines = []; let configs = await engineSelector.fetchEngineConfiguration( locale, - region + region, + AppConstants.MOZ_APP_VERSION_DISPLAY.endsWith("esr") + ? "esr" + : AppConstants.MOZ_UPDATE_CHANNEL ); for (let config of configs.engines) { let engine = await Services.search.makeEnginesFromConfig(config); diff --git a/toolkit/components/search/tests/xpcshell/searchconfigs/test_google.js b/toolkit/components/search/tests/xpcshell/searchconfigs/test_google.js index f078e9a48410..f893abc07747 100644 --- a/toolkit/components/search/tests/xpcshell/searchconfigs/test_google.js +++ b/toolkit/components/search/tests/xpcshell/searchconfigs/test_google.js @@ -3,10 +3,6 @@ "use strict"; -const { AppConstants } = ChromeUtils.import( - "resource://gre/modules/AppConstants.jsm" -); - const test = new SearchConfigTest({ identifier: "google", aliases: ["@google"], diff --git a/toolkit/components/search/tests/xpcshell/test_engine_selector_application.js b/toolkit/components/search/tests/xpcshell/test_engine_selector_application.js new file mode 100644 index 000000000000..a1da4b6a5dcd --- /dev/null +++ b/toolkit/components/search/tests/xpcshell/test_engine_selector_application.js @@ -0,0 +1,118 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +XPCOMUtils.defineLazyModuleGetters(this, { + SearchEngineSelector: "resource://gre/modules/SearchEngineSelector.jsm", +}); + +const CONFIG_URL = + "data:application/json," + + JSON.stringify({ + data: [ + { + webExtension: { + id: "aol@example.com", + }, + appliesTo: [ + { + included: { everywhere: true }, + }, + ], + default: "yes-if-no-other", + }, + { + webExtension: { + id: "lycos@example.com", + }, + appliesTo: [ + { + included: { everywhere: true }, + application: { + channel: ["nightly"], + }, + }, + ], + default: "yes", + }, + { + webExtension: { + id: "altavista@example.com", + }, + appliesTo: [ + { + included: { everywhere: true }, + application: { + channel: ["nightly", "esr"], + }, + }, + ], + }, + { + webExtension: { + id: "excite@example.com", + }, + appliesTo: [ + { + included: { everywhere: true }, + }, + { + included: { everywhere: true }, + application: { + channel: ["release"], + }, + default: "yes", + }, + ], + }, + ], + }); + +const expectedEnginesPerChannel = { + default: ["aol@example.com", "excite@example.com"], + nightly: [ + "lycos@example.com", + "aol@example.com", + "altavista@example.com", + "excite@example.com", + ], + beta: ["aol@example.com", "excite@example.com"], + release: ["excite@example.com", "aol@example.com"], + esr: ["aol@example.com", "altavista@example.com", "excite@example.com"], +}; + +const expectedDefaultEngine = { + default: "aol@example.com", + nightly: "lycos@example.com", + beta: "aol@example.com", + release: "excite@example.com", + esr: "aol@example.com", +}; + +const engineSelector = new SearchEngineSelector(); + +add_task(async function test_engine_selector_channels() { + await engineSelector.init(CONFIG_URL); + + for (let [channel, expected] of Object.entries(expectedEnginesPerChannel)) { + const { engines } = engineSelector.fetchEngineConfiguration( + "en-US", + "us", + channel + ); + + const engineIds = engines.map(obj => obj.webExtension.id); + Assert.deepEqual( + engineIds, + expected, + `Should have the expected engines for channel "${channel}"` + ); + + Assert.equal( + engineIds[0], + expectedDefaultEngine[channel], + `Should have the correct default for channel "${channel}"` + ); + } +}); diff --git a/toolkit/components/search/tests/xpcshell/test_engine_selector_order.js b/toolkit/components/search/tests/xpcshell/test_engine_selector_order.js index 1dd33c1a17f8..54e9d1d3be16 100644 --- a/toolkit/components/search/tests/xpcshell/test_engine_selector_order.js +++ b/toolkit/components/search/tests/xpcshell/test_engine_selector_order.js @@ -116,7 +116,8 @@ add_task(async function() { const { engines, privateDefault } = engineSelector.fetchEngineConfiguration( "us", - "en-US" + "en-US", + "default" ); let names = engines.map(obj => obj.engineName); Assert.deepEqual( diff --git a/toolkit/components/search/tests/xpcshell/xpcshell-common.ini b/toolkit/components/search/tests/xpcshell/xpcshell-common.ini index 0d37c9bf766c..7b5d8a92120e 100644 --- a/toolkit/components/search/tests/xpcshell/xpcshell-common.ini +++ b/toolkit/components/search/tests/xpcshell/xpcshell-common.ini @@ -15,6 +15,7 @@ skip-if = true # Is confusing [test_defaultEngine_fallback.js] [test_defaultEngine.js] [test_defaultPrivateEngine.js] +[test_engine_selector_application.js] [test_engine_selector_order.js] [test_engine_selector.js] [test_engine_set_alias.js]