Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-27 06:43:32 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Backed out changeset 55c366ee4422 (bug 1876178) for causing documentation failures. CLOSED TREE

Browse Source
This commit is contained in:
Cosmin Sabou 2024-11-20 20:33:29 +02:00
parent c310fcedba
commit a98472cdeb
5 changed files with 19 additions and 217 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

98
toolkit/components/search/AppProvidedSearchEngine.sys.mjs
View File

@ -18,7 +18,6 @@ const lazy = {};
ChromeUtils.defineESModuleGetters(lazy, {
NimbusFeatures: "resource://nimbus/ExperimentAPI.sys.mjs",
ObjectUtils: "resource://gre/modules/ObjectUtils.sys.mjs",
RemoteSettings: "resource://services-settings/remote-settings.sys.mjs",
SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs",
});
@ -397,15 +396,6 @@ export class AppProvidedSearchEngine extends SearchEngine {
*/
#isGeneralPurposeSearchEngine = false;
/**
* Stores certain initial info about an engine. Used to verify whether we've
* actually changed the engine, so that we don't record default engine
* changed telemetry unnecessarily.
*
* @type {Map}
*/
#prevEngineInfo = null;
/**
* @param {object} options
* The options for this search engine.
@ -425,13 +415,6 @@ export class AppProvidedSearchEngine extends SearchEngine {
this.#init(config);
this._loadSettings(settings);
this.#prevEngineInfo = new Map([
["name", this.name],
["_loadPath", this._loadPath],
["submissionURL", this.getSubmission("foo").uri.spec],
["aliases", this._definedAliases],
]);
}
/**
@ -458,26 +441,7 @@ export class AppProvidedSearchEngine extends SearchEngine {
update({ configuration }) {
this._urls = [];
this.#init(configuration);
let needToSendUpdate = this.#hasBeenModified(this, this.#prevEngineInfo, [
"name",
"_loadPath",
"aliases",
]);
// We only send a notification if critical fields have changed, e.g., ones
// that may affect the UI or telemetry. If we want to add more fields here
// in the future, we need to ensure we don't send unnecessary
// `engine-update` telemetry. Therefore we may need additional notification
// types or to implement an alternative.
if (needToSendUpdate) {
lazy.SearchUtils.notifyAction(
this,
lazy.SearchUtils.MODIFIED_TYPE.CHANGED
);
this._resetPrevEngineInfo();
}
lazy.SearchUtils.notifyAction(this, lazy.SearchUtils.MODIFIED_TYPE.CHANGED);
}
/**
@ -517,17 +481,6 @@ export class AppProvidedSearchEngine extends SearchEngine {
return this.#isGeneralPurposeSearchEngine;
}
/**
* Stores certain initial info about an engine. Used to verify whether we've
* actually changed the engine, so that we don't record default engine
* changed telemetry unnecessarily.
*
* @type {Map}
*/
get prevEngineInfo() {
return this.#prevEngineInfo;
}
/**
* Returns the icon URL for the search engine closest to the preferred width.
*
@ -687,53 +640,4 @@ export class AppProvidedSearchEngine extends SearchEngine {
this._urls.push(engineURL);
}
/**
* Determines whether the specified engine properties differ between their
* current and initial values.
*
* @param {Engine} currentEngine
* The current engine.
* @param {Map} initialValues
* The initial values stored for the currentEngine.
* @param {Array<string>} targetKeys
* The relevant keys to compare (current value for the engine vs. initial
* value).
* @returns {boolean}
* Returns true if any of the properties relevant to default engine changed
* telemetry was changed.
*/
#hasBeenModified(currentEngine, initialValues, targetKeys) {
for (let i = 0; i < targetKeys.length; i++) {
let key = targetKeys[i];
if (
!lazy.ObjectUtils.deepEqual(currentEngine[key], initialValues.get(key))
) {
return true;
}
let currentEngineSubmissionURL =
currentEngine.getSubmission("foo").uri.spec;
if (currentEngineSubmissionURL != initialValues.get("submissionURL")) {
return true;
}
}
return false;
}
// Not #-prefixed private because it's spied upon in a test.
_resetPrevEngineInfo() {
this.#prevEngineInfo.forEach((_value, key) => {
let newValue;
if (key == "submissionURL") {
newValue = this.submissionURL ?? this.getSubmission("foo").uri.spec;
} else {
newValue = this[key];
}
this.#prevEngineInfo.set(key, newValue);
});
}
}

7
toolkit/components/search/SearchService.sys.mjs
View File

@ -1969,9 +1969,8 @@ export class SearchService {
engine.pendingRemoval = true;
continue;
} else {
// This is an existing engine that we should update. (However
// notification will happen only if the configuration for this engine
// has changed).
// This is an existing engine that we should update (we don't know if
// the configuration for this engine has changed or not).
await engine.update({
configuration: configEngines[index],
});
@ -2918,7 +2917,7 @@ export class SearchService {
* check the "separatePrivateDefault" preference - that is up to the caller.
* @param {nsISearchEngine} newEngine
* The search engine to select
* @param {REASON_CHANGE_MAP} changeSource
* @param {SearchUtils.REASON_CHANGE_MAP} changeSource
* The source of the change of engine.
*/
#setEngineDefault(privateMode, newEngine, changeSource) {

2
toolkit/components/search/tests/xpcshell/test_reload_engines.js
View File

@ -222,7 +222,7 @@ add_task(async function test_config_updated_engine_changes() {
Assert.deepEqual(
enginesModified.sort(),
["hasParamsInFRRegion"],
["appDefault", "defaultInFRRegion", "hasParamsInFRRegion"],
"Should have modified the expected engines"
);

2
toolkit/components/search/tests/xpcshell/test_reload_engines_experiment.js
View File

@ -98,7 +98,7 @@ add_task(async function test_config_updated_engine_changes() {
Assert.deepEqual(
enginesModified.sort(),
[],
["appDefault", "non-experiment"],
"Should have modified the expected engines"
);

127
toolkit/components/search/tests/xpcshell/test_telemetry_event_default.js
View File

@ -10,8 +10,6 @@
"use strict";
ChromeUtils.defineESModuleGetters(this, {
AppProvidedSearchEngine:
"resource://gre/modules/AppProvidedSearchEngine.sys.mjs",
NimbusFeatures: "resource://nimbus/ExperimentAPI.sys.mjs",
});
@ -130,38 +128,6 @@ const MAIN_CONFIG = [
},
];
const CONFIG_WITH_MODIFIED_CLASSIFICATION = [
{
identifier: "originalDefault",
base: {
name: "Original Default",
urls: {
search: {
base: "https://example.com/search",
searchTermParamName: "q",
},
},
classification: "unknown",
},
},
];
const CONFIG_WITH_MODIFIED_NAME = [
{
identifier: "originalDefault",
base: {
name: "Modified Engine Name",
urls: {
search: {
base: "https://example.com/search",
searchTermParamName: "q",
},
},
classification: "general",
},
},
];
const testSearchEngine = {
id: "originalDefault",
name: "Original Default",
@ -205,6 +171,13 @@ async function checkTelemetry(
checkPrivate = false,
additionalEventsExpected = false
) {
// TODO Bug 1876178 - Improve engine change telemetry.
// When we reload engines due to a config change, we update the engines as
// they may have changed, we don't track if any attribute has actually changed
// from previous, and so we send out an update regardless. This is why in
// this test we test for the additional `engine-update` event that's recorded.
// In future, we should be more specific about when to record the event and
// so only one event is captured and not two.
let snapshot;
if (checkPrivate) {
snapshot = await Glean.searchEnginePrivate.changed.testGetValue();
@ -212,8 +185,6 @@ async function checkTelemetry(
snapshot = await Glean.searchEngineDefault.changed.testGetValue();
}
// additionalEventsExpected should be true whenever we expect something
// stored in AppProvidedSearchEngine.#prevEngineInfo to have changed.
if (additionalEventsExpected) {
delete snapshot[0].timestamp;
Assert.deepEqual(
@ -315,7 +286,8 @@ add_task(async function test_experiment_changes_default() {
"experiment",
testNewDefaultEngine,
testDefaultForExperiment,
false
false,
true
);
// Reset the stub so that we are no longer in an experiment.
@ -334,7 +306,8 @@ add_task(async function test_locale_changes_default() {
"locale",
testDefaultForExperiment,
testDefaultInLocaleFRNotRegionDEEngine,
false
false,
true
);
});
@ -350,7 +323,8 @@ add_task(async function test_region_changes_default() {
"region",
testDefaultInLocaleFRNotRegionDEEngine,
testPrefEngine,
false
false,
true
);
});
@ -471,78 +445,3 @@ add_task(async function test_default_engine_update() {
await checkTelemetry("engine-update", defaultEngineData, defaultEngineData);
await extension.unload();
});
add_task(async function test_only_notify_on_relevant_engine_property_change() {
clearTelemetry();
await SearchTestUtils.updateRemoteSettingsConfig(BASE_CONFIG);
// Since SearchUtils.notifyAction can be called for multiple different search
// engine topics, `resetPrevEngineInfo` is a better way to track
// notifications in this case.
let notificationSpy = sinon.spy(
AppProvidedSearchEngine.prototype,
"_resetPrevEngineInfo"
);
// Change an engine property that is not stored in
// AppProvidedSearchEngine.#prevEngineInfo.
let reloadObserved =
SearchTestUtils.promiseSearchNotification("engines-reloaded");
await SearchTestUtils.updateRemoteSettingsConfig(
CONFIG_WITH_MODIFIED_CLASSIFICATION
);
await reloadObserved;
Assert.equal(
notificationSpy.callCount,
0,
"Should not have sent a notification"
);
notificationSpy.restore();
});
add_task(
async function test_multiple_updates_only_notify_on_relevant_engine_property_change() {
clearTelemetry();
await SearchTestUtils.updateRemoteSettingsConfig(BASE_CONFIG);
// Since SearchUtils.notifyAction can be called for multiple different search
// engine topics, `resetPrevEngineInfo` is a better way to track
// notifications in this case.
let notificationSpy = sinon.spy(
AppProvidedSearchEngine.prototype,
"_resetPrevEngineInfo"
);
// Change an engine property that is not stored in
// AppProvidedSearchEngine.#prevEngineInfo.
let reloadObserved1 =
SearchTestUtils.promiseSearchNotification("engines-reloaded");
await SearchTestUtils.updateRemoteSettingsConfig(
CONFIG_WITH_MODIFIED_CLASSIFICATION
);
await reloadObserved1;
Assert.equal(
notificationSpy.callCount,
0,
"Should not have sent a notification"
);
// Now change an engine property that is stored in
// AppProvidedSearchEngine.#prevEngineInfo.
let reloadObserved2 =
SearchTestUtils.promiseSearchNotification("engines-reloaded");
await SearchTestUtils.updateRemoteSettingsConfig(CONFIG_WITH_MODIFIED_NAME);
await reloadObserved2;
Assert.equal(
notificationSpy.callCount,
1,
"Should have sent a notification"
);
notificationSpy.restore();
}
);