From eca22ee4e97d11a1d7536d07b0cde63f4cab2c8c Mon Sep 17 00:00:00 2001 From: Beth Rennie Date: Thu, 13 Mar 2025 15:52:44 +0000 Subject: [PATCH] Bug 1943119 - Remove ExperimentAPI._store r=nalexander,nimbus-reviewers,emcminn ExperimentAPI._store used to create a new, separate ExperimentStore in the child process until bug 1941948 (which was unnecessary) and now only exists for vestigial reasons. Use ExperimentAPI._manager.store everywhere. Differential Revision: https://phabricator.services.mozilla.com/D240414 --- .../shell/test/browser_doesAppNeedPin.js | 7 --- .../shell/test/browser_setDefaultBrowser.js | 3 -- .../components/nimbus/ExperimentAPI.sys.mjs | 48 ++++++++++--------- .../nimbus/test/NimbusTestUtils.sys.mjs | 2 +- ...rowser_remotesettings_experiment_enroll.js | 2 +- ...ettingsexperimentloader_remote_defaults.js | 26 +++++----- .../nimbus/test/unit/test_ExperimentAPI.js | 22 +++++---- 7 files changed, 55 insertions(+), 55 deletions(-) diff --git a/browser/components/shell/test/browser_doesAppNeedPin.js b/browser/components/shell/test/browser_doesAppNeedPin.js index 3d6ab6393d6a..f6e4ee26c69f 100644 --- a/browser/components/shell/test/browser_doesAppNeedPin.js +++ b/browser/components/shell/test/browser_doesAppNeedPin.js @@ -2,15 +2,10 @@ * http://creativecommons.org/publicdomain/zero/1.0/ */ ChromeUtils.defineESModuleGetters(this, { - ExperimentAPI: "resource://nimbus/ExperimentAPI.sys.mjs", ExperimentFakes: "resource://testing-common/NimbusTestUtils.sys.mjs", NimbusFeatures: "resource://nimbus/ExperimentAPI.sys.mjs", }); -registerCleanupFunction(() => { - ExperimentAPI._store._deleteForTests("shellService"); -}); - let defaultValue; add_task(async function default_need() { defaultValue = await ShellService.doesAppNeedPin(); @@ -47,8 +42,6 @@ add_task(async function restore_default() { return; } - ExperimentAPI._store._deleteForTests("shellService"); - Assert.equal( await ShellService.doesAppNeedPin(), defaultValue, diff --git a/browser/components/shell/test/browser_setDefaultBrowser.js b/browser/components/shell/test/browser_setDefaultBrowser.js index f22cae9bb328..12b670ca7af5 100644 --- a/browser/components/shell/test/browser_setDefaultBrowser.js +++ b/browser/components/shell/test/browser_setDefaultBrowser.js @@ -33,8 +33,6 @@ const sendTriggerStub = sinon.stub(ASRouter, "sendTriggerMessage"); registerCleanupFunction(() => { sinon.restore(); - - ExperimentAPI._store._deleteForTests("shellService"); }); let defaultUserChoice; @@ -91,7 +89,6 @@ add_task(async function restore_default() { userChoiceStub.resetHistory(); setDefaultStub.resetHistory(); - ExperimentAPI._store._deleteForTests("shellService"); await ShellService.setDefaultBrowser(); diff --git a/toolkit/components/nimbus/ExperimentAPI.sys.mjs b/toolkit/components/nimbus/ExperimentAPI.sys.mjs index bee4ac05a735..5c6d2eace08b 100644 --- a/toolkit/components/nimbus/ExperimentAPI.sys.mjs +++ b/toolkit/components/nimbus/ExperimentAPI.sys.mjs @@ -143,7 +143,7 @@ export const ExperimentAPI = { * store */ async ready() { - return this._store.ready(); + return this._manager.store.ready(); }, async _annotateCrashReport() { @@ -181,9 +181,9 @@ export const ExperimentAPI = { let experimentData; try { if (slug) { - experimentData = this._store.get(slug); + experimentData = this._manager.store.get(slug); } else if (featureId) { - experimentData = this._store.getExperimentForFeature(featureId); + experimentData = this._manager.store.getExperimentForFeature(featureId); } } catch (e) { console.error(e); @@ -216,12 +216,13 @@ export const ExperimentAPI = { let experimentData; try { if (slug) { - experimentData = this._store.get(slug); + experimentData = this._manager.store.get(slug); } else if (featureId) { if (isRollout) { - experimentData = this._store.getRolloutForFeature(featureId); + experimentData = this._manager.store.getRolloutForFeature(featureId); } else { - experimentData = this._store.getExperimentForFeature(featureId); + experimentData = + this._manager.store.getExperimentForFeature(featureId); } } } catch (e) { @@ -263,9 +264,9 @@ export const ExperimentAPI = { let experiment = null; try { if (slug) { - experiment = this._store.get(slug); + experiment = this._manager.store.get(slug); } else if (featureId) { - experiment = this._store.getExperimentForFeature(featureId); + experiment = this._manager.store.getExperimentForFeature(featureId); } } catch (e) { console.error(e); @@ -286,7 +287,7 @@ export const ExperimentAPI = { * This should noly be called from the main process. * * Note that the recipe is directly fetched from RemoteSettings, which has - * all the recipe metadata available without relying on the `this._store`. + * all the recipe metadata available without relying on the `this._manager.store`. * Therefore, calling this function does not require to call `this.ready()` first. * * @param slug {String} An experiment identifier @@ -387,7 +388,7 @@ export class _ExperimentFeature { fallbackPref, null, () => { - ExperimentAPI._store._emitFeatureUpdate( + ExperimentAPI._manager.store._emitFeatureUpdate( this.featureId, "pref-updated" ); @@ -428,7 +429,9 @@ export class _ExperimentFeature { getAllVariables({ defaultValues = null } = {}) { let enrollment = null; try { - enrollment = ExperimentAPI._store.getExperimentForFeature(this.featureId); + enrollment = ExperimentAPI._manager.store.getExperimentForFeature( + this.featureId + ); } catch (e) { console.error(e); } @@ -436,7 +439,9 @@ export class _ExperimentFeature { if (typeof featureValue === "undefined") { try { - enrollment = ExperimentAPI._store.getRolloutForFeature(this.featureId); + enrollment = ExperimentAPI._manager.store.getRolloutForFeature( + this.featureId + ); } catch (e) { console.error(e); } @@ -463,7 +468,9 @@ export class _ExperimentFeature { // Next, check if an experiment is defined let enrollment = null; try { - enrollment = ExperimentAPI._store.getExperimentForFeature(this.featureId); + enrollment = ExperimentAPI._manager.store.getExperimentForFeature( + this.featureId + ); } catch (e) { console.error(e); } @@ -474,7 +481,9 @@ export class _ExperimentFeature { // Next, check for a rollout. try { - enrollment = ExperimentAPI._store.getRolloutForFeature(this.featureId); + enrollment = ExperimentAPI._manager.store.getRolloutForFeature( + this.featureId + ); } catch (e) { console.error(e); } @@ -489,7 +498,7 @@ export class _ExperimentFeature { } getRollout() { - let remoteConfig = ExperimentAPI._store.getRolloutForFeature( + let remoteConfig = ExperimentAPI._manager.store.getRolloutForFeature( this.featureId ); if (!remoteConfig) { @@ -536,11 +545,11 @@ export class _ExperimentFeature { } onUpdate(callback) { - ExperimentAPI._store._onFeatureUpdate(this.featureId, callback); + ExperimentAPI._manager.store._onFeatureUpdate(this.featureId, callback); } offUpdate(callback) { - ExperimentAPI._store._offFeatureUpdate(this.featureId, callback); + ExperimentAPI._manager.store._offFeatureUpdate(this.featureId, callback); } /** @@ -731,11 +740,6 @@ ChromeUtils.defineLazyGetter(ExperimentAPI, "_manager", function () { return lazy.ExperimentManager; }); -Object.defineProperty(ExperimentAPI, "_store", { - configurable: true, - get: () => ExperimentAPI._manager.store, -}); - ChromeUtils.defineLazyGetter(ExperimentAPI, "_rsLoader", function () { return lazy.RemoteSettingsExperimentLoader; }); diff --git a/toolkit/components/nimbus/test/NimbusTestUtils.sys.mjs b/toolkit/components/nimbus/test/NimbusTestUtils.sys.mjs index 3a6ca6e968b2..d3b1839a9b86 100644 --- a/toolkit/components/nimbus/test/NimbusTestUtils.sys.mjs +++ b/toolkit/components/nimbus/test/NimbusTestUtils.sys.mjs @@ -189,7 +189,7 @@ export const ExperimentFakes = { }, waitForExperimentUpdate(ExperimentAPI, slug) { return new Promise(resolve => - ExperimentAPI._store.once(`update:${slug}`, resolve) + ExperimentAPI._manager.store.once(`update:${slug}`, resolve) ); }, /** diff --git a/toolkit/components/nimbus/test/browser/browser_remotesettings_experiment_enroll.js b/toolkit/components/nimbus/test/browser/browser_remotesettings_experiment_enroll.js index a83c374ec7ec..ab5f2d4c14f3 100644 --- a/toolkit/components/nimbus/test/browser/browser_remotesettings_experiment_enroll.js +++ b/toolkit/components/nimbus/test/browser/browser_remotesettings_experiment_enroll.js @@ -71,7 +71,7 @@ add_task(async function test_experimentEnrollment() { }); Assert.ok(!experiment.active, "Experiment is no longer active"); - ExperimentAPI._store._deleteForTests(recipe.slug); + ExperimentAPI._manager.store._deleteForTests(recipe.slug); }); add_task(async function test_experimentEnrollment_startup() { diff --git a/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js b/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js index f4e43d60e715..5fd9753c2801 100644 --- a/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js +++ b/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js @@ -252,10 +252,10 @@ add_task(async function test_remote_fetch_and_ready() { ); Assert.ok(!barInstance.getVariable("remoteValue"), "Should be missing"); - ExperimentAPI._store._deleteForTests("foo"); - ExperimentAPI._store._deleteForTests("bar"); - ExperimentAPI._store._deleteForTests(REMOTE_CONFIGURATION_FOO.slug); - ExperimentAPI._store._deleteForTests(REMOTE_CONFIGURATION_BAR.slug); + ExperimentAPI._manager.store._deleteForTests("foo"); + ExperimentAPI._manager.store._deleteForTests("bar"); + ExperimentAPI._manager.store._deleteForTests(REMOTE_CONFIGURATION_FOO.slug); + ExperimentAPI._manager.store._deleteForTests(REMOTE_CONFIGURATION_BAR.slug); sandbox.restore(); cleanupTestFeatures(); @@ -413,8 +413,8 @@ add_task(async function test_finalizeRemoteConfigs_cleanup() { // This will also remove the inactive recipe from the store // the previous update (from recipe not seen code path) // only sets the recipe as inactive - ExperimentAPI._store._deleteForTests("bar-rollout"); - ExperimentAPI._store._deleteForTests("foo-rollout"); + ExperimentAPI._manager.store._deleteForTests("bar-rollout"); + ExperimentAPI._manager.store._deleteForTests("foo-rollout"); cleanupTestFeatures(); cleanup(); @@ -424,7 +424,7 @@ add_task(async function test_finalizeRemoteConfigs_cleanup() { // this test should not throw add_task(async function remote_defaults_no_mutation() { let sandbox = sinon.createSandbox(); - sandbox.stub(ExperimentAPI._store, "getRolloutForFeature").returns( + sandbox.stub(ExperimentAPI._manager.store, "getRolloutForFeature").returns( Cu.cloneInto( { featureIds: ["foo"], @@ -446,8 +446,8 @@ add_task(async function remote_defaults_no_mutation() { }); add_task(async function remote_defaults_active_remote_defaults() { - ExperimentAPI._store._deleteForTests("foo"); - ExperimentAPI._store._deleteForTests("bar"); + ExperimentAPI._manager.store._deleteForTests("foo"); + ExperimentAPI._manager.store._deleteForTests("bar"); let barFeature = new ExperimentFeature("bar", { description: "mochitest", variables: { enabled: { type: "boolean" } }, @@ -514,8 +514,8 @@ add_task(async function remote_defaults_active_remote_defaults() { await featureUpdate; Assert.ok(fooFeature.getVariable("enabled"), "Targeting should match"); - ExperimentAPI._store._deleteForTests("foo"); - ExperimentAPI._store._deleteForTests("bar"); + ExperimentAPI._manager.store._deleteForTests("foo"); + ExperimentAPI._manager.store._deleteForTests("bar"); cleanup(); cleanupTestFeatures(); @@ -586,6 +586,6 @@ add_task(async function remote_defaults_variables_storage() { "Variable pref is cleared" ); Assert.ok(!barFeature.getVariable("string"), "Variable is no longer defined"); - ExperimentAPI._store._deleteForTests("bar"); - ExperimentAPI._store._deleteForTests("bar-rollout"); + ExperimentAPI._manager.store._deleteForTests("bar"); + ExperimentAPI._manager.store._deleteForTests("bar-rollout"); }); diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js b/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js index 42338c54dd2b..e0f61a1da62a 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js +++ b/toolkit/components/nimbus/test/unit/test_ExperimentAPI.js @@ -107,8 +107,10 @@ add_task(function test_getExperimentMetaData_safe() { const sandbox = sinon.createSandbox(); let exposureStub = sandbox.stub(ExperimentAPI, "recordExposureEvent"); - sandbox.stub(ExperimentAPI._store, "get").throws(); - sandbox.stub(ExperimentAPI._store, "getExperimentForFeature").throws(); + sandbox.stub(ExperimentAPI._manager.store, "get").throws(); + sandbox + .stub(ExperimentAPI._manager.store, "getExperimentForFeature") + .throws(); try { let metadata = ExperimentAPI.getExperimentMetaData({ slug: "foo" }); @@ -117,7 +119,7 @@ add_task(function test_getExperimentMetaData_safe() { Assert.ok(false, "Error should be caught in ExperimentAPI"); } - Assert.ok(ExperimentAPI._store.get.calledOnce, "Sanity check"); + Assert.ok(ExperimentAPI._manager.store.get.calledOnce, "Sanity check"); try { let metadata = ExperimentAPI.getExperimentMetaData({ featureId: "foo" }); @@ -127,7 +129,7 @@ add_task(function test_getExperimentMetaData_safe() { } Assert.ok( - ExperimentAPI._store.getExperimentForFeature.calledOnce, + ExperimentAPI._manager.store.getExperimentForFeature.calledOnce, "Sanity check" ); @@ -138,7 +140,9 @@ add_task(function test_getExperimentMetaData_safe() { add_task(async function test_getExperiment_safe() { const sandbox = sinon.createSandbox(); - sandbox.stub(ExperimentAPI._store, "getExperimentForFeature").throws(); + sandbox + .stub(ExperimentAPI._manager.store, "getExperimentForFeature") + .throws(); try { Assert.equal( @@ -163,7 +167,7 @@ add_task(async function test_getExperiment_featureAccess() { }, }); const stub = sandbox - .stub(ExperimentAPI._store, "getExperimentForFeature") + .stub(ExperimentAPI._manager.store, "getExperimentForFeature") .returns(expected); let { branch } = ExperimentAPI.getExperiment({ featureId: "cfr" }); @@ -185,7 +189,7 @@ add_task(async function test_getExperiment_featureAccess_backwardsCompat() { }, }); const stub = sandbox - .stub(ExperimentAPI._store, "getExperimentForFeature") + .stub(ExperimentAPI._manager.store, "getExperimentForFeature") .returns(expected); let { branch } = ExperimentAPI.getExperiment({ featureId: "cfr" }); @@ -477,7 +481,9 @@ add_task(async function test_getActiveBranch() { add_task(async function test_getActiveBranch_safe() { const sandbox = sinon.createSandbox(); - sandbox.stub(ExperimentAPI._store, "getAllActiveExperiments").throws(); + sandbox + .stub(ExperimentAPI._manager.store, "getAllActiveExperiments") + .throws(); try { Assert.equal(