Bug 1710859 - Add getVariable to FeatureAPI r=andreio

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

browser/base/content/browser.js

@@ -2187,7 +2187,7 @@ var gBrowserInit = {
       // property set to true, if yes remove focus from urlbar for about:welcome
       const aboutWelcomeSkipUrlBarFocus =
         uriToLoad == "about:welcome" &&
-        NimbusFeatures.aboutwelcome.getValue()?.skipFocus;
+        NimbusFeatures.aboutwelcome.getVariable("skipFocus");
 
       if (
         (isBlankPageURL(uriToLoad) && !aboutWelcomeSkipUrlBarFocus) ||

toolkit/components/nimbus/ExperimentAPI.jsm

@@ -28,6 +28,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   setTimeout: "resource://gre/modules/Timer.jsm",
   clearTimeout: "resource://gre/modules/Timer.jsm",
   FeatureManifest: "resource://nimbus/FeatureManifest.js",
+  AppConstants: "resource://gre/modules/AppConstants.jsm",
 });
 
 const IS_MAIN_PROCESS =
@@ -380,6 +381,10 @@ class ExperimentFeature {
     }
   }
 
+  getPreferenceName(variable) {
+    return this.manifest?.variables?.[variable]?.fallbackPref;
+  }
+
   _getUserPrefsValues() {
     let userPrefs = {};
     Object.keys(this.manifest?.variables || {}).forEach(variable => {
@@ -479,6 +484,48 @@ class ExperimentFeature {
     };
   }
 
+  getVariable(variable, { sendExposureEvent } = {}) {
+    const prefName = this.getPreferenceName(variable);
+    const prefValue = prefName ? this.prefGetters[variable] : undefined;
+
+    if (!this.manifest?.variables?.[variable]) {
+      // Only throw in nightly/tests
+      if (Cu.isInAutomation || AppConstants.NIGHTLY_BUILD) {
+        throw new Error(
+          `Nimbus: Warning - variable "${variable}" is not defined in FeatureManifest.js`
+        );
+      }
+    }
+
+    // If a user value is set for the defined preference, always return that first
+    if (prefName && Services.prefs.prefHasUserValue(prefName)) {
+      return prefValue;
+    }
+
+    // Next, check if an experiment is defined
+    const experimentValue = ExperimentAPI.activateBranch({
+      featureId: this.featureId,
+      sendExposureEvent: sendExposureEvent && this._sendExposureEventOnce,
+    })?.feature?.value?.[variable];
+
+    // Prevent future exposure events if user is enrolled in an experiment
+    if (experimentValue && sendExposureEvent) {
+      this._sendExposureEventOnce = false;
+    }
+
+    if (typeof experimentValue !== "undefined") {
+      return experimentValue;
+    }
+
+    // Next, check remote defaults
+    const remoteValue = this.getRemoteConfig()?.variables?.[variable];
+    if (typeof remoteValue !== "undefined") {
+      return remoteValue;
+    }
+    // Return the default preference value
+    return prefValue;
+  }
+
   getRemoteConfig() {
     let remoteConfig = ExperimentAPI._store.getRemoteConfig(this.featureId);
     if (!remoteConfig) {

toolkit/components/nimbus/FeatureManifest.js

@@ -79,6 +79,11 @@ const FeatureManifest = {
   },
   "password-autocomplete": {
     description: "A special autocomplete UI for password fields.",
+    variables: {
+      directMigrateSingleProfile: {
+        type: "boolean",
+      },
+    },
   },
   upgradeDialog: {
     description: "The dialog shown for major upgrades",

toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature_getVariable.js

@@ -0,0 +1,194 @@
+"use strict";
+
+const { ExperimentAPI, ExperimentFeature } = ChromeUtils.import(
+  "resource://nimbus/ExperimentAPI.jsm"
+);
+const { ExperimentFakes } = ChromeUtils.import(
+  "resource://testing-common/NimbusTestUtils.jsm"
+);
+const { TestUtils } = ChromeUtils.import(
+  "resource://testing-common/TestUtils.jsm"
+);
+
+const { XPCOMUtils } = ChromeUtils.import(
+  "resource://gre/modules/XPCOMUtils.jsm"
+);
+
+async function setupForExperimentFeature() {
+  const sandbox = sinon.createSandbox();
+  const manager = ExperimentFakes.manager();
+  await manager.onStartup();
+
+  sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
+
+  return { sandbox, manager };
+}
+
+const FEATURE_ID = "testfeature1";
+// Note: this gets deleted at the end of tests
+const TEST_PREF_BRANCH = "testfeature1.";
+const TEST_VARIABLES = {
+  enabled: {
+    type: "boolean",
+    fallbackPref: `${TEST_PREF_BRANCH}enabled`,
+  },
+  name: {
+    type: "string",
+    fallbackPref: `${TEST_PREF_BRANCH}name`,
+  },
+  count: {
+    type: "int",
+    fallbackPref: `${TEST_PREF_BRANCH}count`,
+  },
+  items: {
+    type: "json",
+    fallbackPref: `${TEST_PREF_BRANCH}items`,
+  },
+};
+
+function createInstanceWithVariables(variables) {
+  return new ExperimentFeature(FEATURE_ID, {
+    variables,
+  });
+}
+
+add_task(async function test_ExperimentFeature_getPreferenceName() {
+  const instance = createInstanceWithVariables(TEST_VARIABLES);
+
+  Assert.equal(
+    instance.getPreferenceName("enabled"),
+    "testfeature1.enabled",
+    "should return the fallback preference name"
+  );
+});
+
+add_task(async function test_ExperimentFeature_getVariable_notRegistered() {
+  const instance = createInstanceWithVariables(TEST_VARIABLES);
+
+  Assert.throws(
+    () => {
+      instance.getVariable("non_existant_variable");
+    },
+    /Nimbus: Warning - variable "non_existant_variable" is not defined in FeatureManifest\.js/,
+    "should throw in automation for variables not defined in the manifest"
+  );
+});
+
+add_task(async function test_ExperimentFeature_getVariable_noFallbackPref() {
+  const instance = createInstanceWithVariables({
+    foo: { type: "json" },
+  });
+
+  Assert.equal(
+    instance.getVariable("foo"),
+    undefined,
+    "should return undefined if no values are set and no fallback pref is defined"
+  );
+});
+
+add_task(async function test_ExperimentFeature_getVariable_precedence() {
+  const { sandbox, manager } = await setupForExperimentFeature();
+
+  const instance = createInstanceWithVariables(TEST_VARIABLES);
+  const prefName = TEST_VARIABLES.items.fallbackPref;
+
+  Services.prefs.clearUserPref(prefName);
+
+  Assert.equal(
+    instance.getVariable("items"),
+    undefined,
+    "should return undefined if the fallback pref is not set"
+  );
+
+  // Default pref values
+  Services.prefs
+    .getDefaultBranch("")
+    .setStringPref(prefName, JSON.stringify([1, 2, 3]));
+
+  Assert.deepEqual(
+    instance.getVariable("items"),
+    [1, 2, 3],
+    "should return the default pref value"
+  );
+
+  // Remote default values
+  manager.store.updateRemoteConfigs(FEATURE_ID, {
+    variables: { items: [4, 5, 6] },
+  });
+
+  Assert.deepEqual(
+    instance.getVariable("items"),
+    [4, 5, 6],
+    "should return the remote default value over the default pref value"
+  );
+
+  // Experiment values
+  const doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig(
+    {
+      featureId: FEATURE_ID,
+      value: {
+        items: [7, 8, 9],
+      },
+    },
+    { manager }
+  );
+
+  Assert.deepEqual(
+    instance.getVariable("items"),
+    [7, 8, 9],
+    "should return the experiment value over the remote value"
+  );
+
+  // User pref values
+  Services.prefs.setStringPref(prefName, JSON.stringify([10, 11, 12]));
+  Assert.deepEqual(
+    instance.getVariable("items"),
+    [10, 11, 12],
+    "should return the user branch pref value over any other value"
+  );
+
+  // Cleanup
+  Services.prefs.getDefaultBranch("").deleteBranch(TEST_PREF_BRANCH);
+  Services.prefs.deleteBranch(TEST_PREF_BRANCH);
+  await doExperimentCleanup();
+  sandbox.restore();
+});
+
+add_task(async function test_ExperimentFeature_getVariable_partial_values() {
+  const { sandbox, manager } = await setupForExperimentFeature();
+
+  const instance = createInstanceWithVariables(TEST_VARIABLES);
+
+  // Set up a pref value for .enabled,
+  // a remote value for .name,
+  // an experiment value for .items
+  Services.prefs.setBoolPref(TEST_VARIABLES.enabled.fallbackPref, true);
+  manager.store.updateRemoteConfigs(FEATURE_ID, {
+    variables: { name: "abc" },
+  });
+  const doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig(
+    {
+      featureId: FEATURE_ID,
+      value: {},
+    },
+    { manager }
+  );
+
+  Assert.equal(
+    instance.getVariable("enabled"),
+    true,
+    "should skip missing variables from remote defaults"
+  );
+
+  Assert.equal(
+    instance.getVariable("name"),
+    "abc",
+    "should skip missing variables from experiments"
+  );
+
+  // Cleanup
+  Services.prefs.getDefaultBranch("").deleteBranch(TEST_PREF_BRANCH);
+  Services.prefs.deleteBranch(TEST_PREF_BRANCH);
+  await doExperimentCleanup();
+  sandbox.restore();
+});

toolkit/components/nimbus/test/unit/xpcshell.ini

@@ -16,6 +16,7 @@ support-files =
 [test_ExperimentAPI.js]
 [test_ExperimentAPI_ExperimentFeature.js]
 [test_ExperimentAPI_ExperimentFeature_getValue.js]
+[test_ExperimentAPI_ExperimentFeature_getVariable.js]
 [test_RemoteSettingsExperimentLoader.js]
 [test_RemoteSettingsExperimentLoader_updateRecipes.js]
 [test_ExperimentAPI_NimbusFeatures.js]

toolkit/components/passwordmgr/LoginManagerParent.jsm

@@ -335,8 +335,9 @@ class LoginManagerParent extends JSWindowActorParent {
         const profiles = await migrator.getSourceProfiles();
         if (
           profiles.length == 1 &&
-          NimbusFeatures["password-autocomplete"].getValue()
-            ?.directMigrateSingleProfile
+          NimbusFeatures["password-autocomplete"].getVariable(
+            "directMigrateSingleProfile"
+          )
         ) {
           const loginAdded = new Promise(resolve => {
             const obs = (subject, topic, data) => {