Bug 1875404 - Require branch for setPref variables r=chumphreys

The setPref variable annotation now requires both the pref to set and the
branch that should be used. Previously this was determined by the
isEarlyStartup feature annotation: true meant that prefs would be set on the
user branch and false meant that they would be set on the default branch. All
setPref annotations have been updated to stay consistent with that model.

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

toolkit/components/nimbus/ExperimentAPI.sys.mjs

@@ -338,6 +338,12 @@ export class _ExperimentFeature {
   }
 
   getSetPrefName(variable) {
+    const setPref = this.manifest?.variables?.[variable]?.setPref;
+
+    return setPref?.pref ?? setPref ?? undefined;
+  }
+
+  getSetPref(variable) {
     return this.manifest?.variables?.[variable]?.setPref;
   }
 

toolkit/components/nimbus/generate/generate_feature_manifest.py

@@ -39,6 +39,10 @@ def validate_feature_manifest(schema_path, manifest_path, manifest):
 
             for variable, variable_def in feature_def.get("variables", {}).items():
                 set_pref = variable_def.get("setPref")
+
+                if isinstance(set_pref, dict):
+                    set_pref = set_pref.get("pref")
+
                 if set_pref is not None:
                     if set_pref in set_prefs:
                         other_feature = set_prefs[set_pref][0]

toolkit/components/nimbus/lib/ExperimentManager.sys.mjs

@@ -896,13 +896,12 @@ export class _ExperimentManager {
       // need to check if we have another enrollment for the same feature.
       const conflictingEnrollment = getConflictingEnrollment(featureId);
 
-      const prefBranch =
-        feature.manifest.isEarlyStartup ?? false ? "user" : "default";
-
       for (const [variable, value] of Object.entries(featureValue)) {
-        const prefName = feature.getSetPrefName(variable);
+        const setPref = feature.getSetPref(variable);
+
+        if (setPref) {
+          const { pref: prefName, branch: prefBranch } = setPref;
 
-        if (prefName) {
           let originalValue;
           const conflictingPref = conflictingEnrollment?.prefs?.find(
             p => p.name === prefName
@@ -935,7 +934,11 @@ export class _ExperimentManager {
 
           // An experiment takes precedence if there is already a pref set.
           if (!isRollout || !conflictingPref) {
-            prefsToSet.push({ name: prefName, value, prefBranch });
+            prefsToSet.push({
+              name: prefName,
+              value,
+              prefBranch,
+            });
           }
         }
       }
@@ -1134,7 +1137,12 @@ export class _ExperimentManager {
       }
 
       // If the variable is setting a different preference, unenroll.
-      if (variableDef.setPref !== name) {
+      const prefName =
+        typeof variableDef.setPref === "object"
+          ? variableDef.setPref.pref
+          : variableDef.setPref;
+
+      if (prefName !== name) {
         this._unenroll(enrollment, {
           reason: "pref-variable-changed",
           duringRestore: true,

toolkit/components/nimbus/schemas/ExperimentFeatureManifest.schema.json

@@ -63,8 +63,21 @@
               "description": "A pref that provides the default value for a feature when none is present"
             },
             "setPref": {
-              "type": "string",
-              "description": "A pref that should be set to the value of this variable when enrolling in experiments."
+              "description": "A pref that should be set to the value of this variable when enrolling in experiments.",
+              "type": "object",
+              "properties": {
+                "branch": {
+                  "type": "string",
+                  "enum": ["default", "user"],
+                  "description": "The branch the pref will be set on."
+                },
+                "pref": {
+                  "type": "string",
+                  "description": "The name of the pref."
+                }
+              },
+              "required": ["branch", "pref"],
+              "additionalProperties": false
             },
             "enum": {
               "description": "Validate feature value using a list of possible options (for string only values)."

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

@@ -33,12 +33,14 @@ const PREF_FEATURES = [
     description: "Test feature that sets a pref on the default branch.",
     owner: "test@test.test",
     hasExposure: false,
-    isEarlyStartup: false,
     variables: {
       foo: {
         type: "string",
         description: "Test variable",
-        setPref: "nimbus.test-only.foo",
+        setPref: {
+          branch: "default",
+          pref: "nimbus.test-only.foo",
+        },
       },
     },
   }),
@@ -51,7 +53,10 @@ const PREF_FEATURES = [
       bar: {
         type: "string",
         description: "Test variable",
-        setPref: "nimbus.test-only.bar",
+        setPref: {
+          branch: "user",
+          pref: "nimbus.test-only.bar",
+        },
       },
     },
   }),
@@ -2596,17 +2601,23 @@ add_task(async function test_prefChanged_noPrefSet() {
   const featureId = "test-set-pref-2";
   const pref = "nimbus.test-only.baz";
 
-  function featureFactory(isEarlyStartup) {
+  function featureFactory(prefBranch) {
+    if (![USER, DEFAULT].includes(prefBranch)) {
+      Assert.ok(false, `invalid branch ${prefBranch}`);
+    }
+
     return new ExperimentFeature(featureId, {
       description: "Test feature that sets a pref",
       owner: "test@test.test",
       hasExposure: false,
-      isEarlyStartup,
       variables: {
         baz: {
           type: "string",
           description: "Test variable",
-          setPref: pref,
+          setPref: {
+            branch: prefBranch,
+            pref,
+          },
         },
         qux: {
           type: "string",
@@ -2623,8 +2634,8 @@ add_task(async function test_prefChanged_noPrefSet() {
     },
   };
 
-  for (const isEarlyStartup of [true, false]) {
-    const feature = featureFactory(isEarlyStartup);
+  for (const prefBranch of [USER, DEFAULT]) {
+    const feature = featureFactory(prefBranch);
     const cleanupFeature = ExperimentTestUtils.addTestFeatures(feature);
 
     const store = ExperimentFakes.store();
@@ -2737,17 +2748,23 @@ add_task(async function test_restorePrefs_manifestChanged() {
   const pref = "nimbus.test-only.baz";
 
   // Return a new object so we can modified the returned value.
-  function featureFactory(isEarlyStartup) {
+  function featureFactory(prefBranch) {
+    if (![USER, DEFAULT].includes(prefBranch)) {
+      Assert.ok(false, `invalid branch ${prefBranch}`);
+    }
+
     return new ExperimentFeature(featureId, {
       description: "Test feature that sets a pref on the default branch.",
       owner: "test@test.test",
       hasExposure: false,
-      isEarlyStartup,
       variables: {
         baz: {
           type: "string",
           description: "Test variable",
-          setPref: pref,
+          setPref: {
+            branch: prefBranch,
+            pref,
+          },
         },
         qux: {
           type: "string",
@@ -2820,7 +2837,7 @@ add_task(async function test_restorePrefs_manifestChanged() {
       /* clear = */ true
     );
 
-    const feature = featureFactory(branch === USER);
+    const feature = featureFactory(branch);
     const cleanupFeatures = ExperimentTestUtils.addTestFeatures(feature);
 
     setPrefs(pref, { defaultBranchValue, userBranchValue });
@@ -3178,17 +3195,22 @@ add_task(async function test_nested_prefs_enroll_both() {
     description: "Nested prefs",
     owner: "test@test.test",
     hasExposure: false,
-    isEarlyStartup: false,
     variables: {
       enabled: {
         type: "boolean",
         description: "enable this feature",
-        setPref: "nimbus.test-only.nested",
+        setPref: {
+          branch: "default",
+          pref: "nimbus.test-only.nested",
+        },
       },
       setting: {
         type: "string",
         description: "a nested setting",
-        setPref: "nimbus.test-only.nested.setting",
+        setPref: {
+          branch: "default",
+          pref: "nimbus.test-only.nested.setting",
+        },
       },
     },
   });