Bug 1547034: Migrate PreferenceExperiments observers r=mythmon
authorEthan Glasser-Camp <ethan@betacantrips.com>
Thu, 16 May 2019 15:04:18 +0000
changeset 474246 da4380fe5ce1ea4d7dba885f1127fac61860552e
parent 474245 137380671f01c6185f61a04b3fd55aac9617da4a
child 474247 454e86015872654981d491d0867f91e4c6685e6f
push id36027
push usershindli@mozilla.com
push dateFri, 17 May 2019 16:24:38 +0000
treeherdermozilla-central@c94c54aff466 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmythmon
bugs1547034
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1547034: Migrate PreferenceExperiments observers r=mythmon Move startObserver to take a preferences object and register its observer for each preference in that object. While we're here, move to the canonical observer interface according to nsIPrefBranch.idl, with an `observe` method instead of just passing a function. Also perform some drive-by cleanups in the tests: add a name to one test, drop some unused arguments to some other tests. Depends on D29293 Differential Revision: https://phabricator.services.mozilla.com/D29871
toolkit/components/normandy/lib/PreferenceExperiments.jsm
toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
--- a/toolkit/components/normandy/lib/PreferenceExperiments.jsm
+++ b/toolkit/components/normandy/lib/PreferenceExperiments.jsm
@@ -263,20 +263,18 @@ var PreferenceExperiments = {
 
       // Notify Telemetry of experiments we're running, since they don't persist between restarts
       TelemetryEnvironment.setExperimentActive(
         experiment.name,
         experiment.branch,
         {type: EXPERIMENT_TYPE_PREFIX + experiment.experimentType}
       );
 
-      for (const [prefName, prefInfo] of Object.entries(experiment.preferences)) {
-        // Watch for changes to the experiment's preference
-        this.startObserver(experiment.name, prefName, prefInfo.preferenceType, prefInfo.preferenceValue);
-      }
+      // Watch for changes to the experiment's preference
+      this.startObserver(experiment.name, experiment.preferences);
     }
   },
 
   /**
    * Save in-progress, default-branch preference experiments in a sub-branch of
    * the normandy preferences. On startup, we read these to set the
    * experimental values.
    *
@@ -451,17 +449,17 @@ var PreferenceExperiments = {
           preferenceType,
           previousPreferenceValue: getPref(preferences, preferenceName, preferenceType),
         },
       },
       experimentType,
     };
 
     setPref(preferences, preferenceName, preferenceType, preferenceValue);
-    PreferenceExperiments.startObserver(name, preferenceName, preferenceType, preferenceValue);
+    PreferenceExperiments.startObserver(name, {[preferenceName]: {preferenceType, preferenceValue}});
     store.data.experiments[name] = experiment;
     store.saveSoon();
 
     TelemetryEnvironment.setExperimentActive(name, branch, {type: EXPERIMENT_TYPE_PREFIX + experimentType});
     TelemetryEvents.sendEvent("enroll", "preference_study", name, {experimentType, branch});
     await this.saveStartupPrefs();
   },
 
@@ -469,39 +467,42 @@ var PreferenceExperiments = {
    * Register a preference observer that stops an experiment when the user
    * modifies the preference.
    * @param {string} experimentName
    * @param {string} preferenceName
    * @param {string|integer|boolean} preferenceValue
    * @throws {Error}
    *   If an observer for the named experiment is already active.
    */
-  startObserver(experimentName, preferenceName, preferenceType, preferenceValue) {
+  startObserver(experimentName, preferences) {
     log.debug(`PreferenceExperiments.startObserver(${experimentName})`);
 
     if (experimentObservers.has(experimentName)) {
       throw new Error(
         `An observer for the preference experiment ${experimentName} is already active.`
       );
     }
 
     const observerInfo = {
-      preferenceName,
-      observer() {
+      preferences,
+      observe(aSubject, aTopic, preferenceName) {
+        const {preferenceValue, preferenceType} = preferences[preferenceName];
         const newValue = getPref(UserPreferences, preferenceName, preferenceType);
         if (newValue !== preferenceValue) {
           PreferenceExperiments.stop(experimentName, {
             resetValue: false,
             reason: "user-preference-changed",
           }).catch(Cu.reportError);
         }
       },
     };
     experimentObservers.set(experimentName, observerInfo);
-    Services.prefs.addObserver(preferenceName, observerInfo.observer);
+    for (const preferenceName of Object.keys(preferences)) {
+      Services.prefs.addObserver(preferenceName, observerInfo);
+    }
   },
 
   /**
    * Check if a preference observer is active for an experiment.
    * @param {string} experimentName
    * @return {Boolean}
    */
   hasObserver(experimentName) {
@@ -517,28 +518,32 @@ var PreferenceExperiments = {
    */
   stopObserver(experimentName) {
     log.debug(`PreferenceExperiments.stopObserver(${experimentName})`);
 
     if (!experimentObservers.has(experimentName)) {
       throw new Error(`No observer for the preference experiment ${experimentName} found.`);
     }
 
-    const {preferenceName, observer} = experimentObservers.get(experimentName);
-    Services.prefs.removeObserver(preferenceName, observer);
+    const observer = experimentObservers.get(experimentName);
+    for (const preferenceName of Object.keys(observer.preferences)) {
+      Services.prefs.removeObserver(preferenceName, observer);
+    }
     experimentObservers.delete(experimentName);
   },
 
   /**
    * Disable all currently-active preference observers for experiments.
    */
   stopAllObservers() {
     log.debug("PreferenceExperiments.stopAllObservers()");
-    for (const {preferenceName, observer} of experimentObservers.values()) {
-      Services.prefs.removeObserver(preferenceName, observer);
+    for (const observer of experimentObservers.values()) {
+      for (const preferenceName of Object.keys(observer.preferences)) {
+        Services.prefs.removeObserver(preferenceName, observer);
+      }
     }
     experimentObservers.clear();
   },
 
   /**
    * Update the timestamp storing when Normandy last sent a recipe for the named
    * experiment.
    * @param {string} experimentName
--- a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
+++ b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js
@@ -221,17 +221,17 @@ decorate_task(
       branch: "branch",
       preferenceName: "fake.preference",
       preferenceValue: "newvalue",
       preferenceBranchType: "default",
       preferenceType: "string",
     });
     ok(await PreferenceExperiments.get("test"), "start saved the experiment");
     ok(
-      startObserverStub.calledWith("test", "fake.preference", "string", "newvalue"),
+      startObserverStub.calledWith("test", {"fake.preference": {preferenceType: "string", preferenceValue: "newvalue"}}),
       "start registered an observer",
     );
 
     const expectedExperiment = {
       name: "test",
       branch: "branch",
       expired: false,
       preferences: {
@@ -279,17 +279,17 @@ decorate_task(
       name: "test",
       branch: "branch",
       preferenceName: "fake.preference",
       preferenceValue: "newvalue",
       preferenceType: "string",
       preferenceBranchType: "user",
     });
     ok(
-      startObserver.calledWith("test", "fake.preference", "string", "newvalue"),
+      startObserver.calledWith("test", {"fake.preference": {preferenceType: "string", preferenceValue: "newvalue"}}),
       "start registered an observer",
     );
 
     const expectedExperiment = {
       name: "test",
       branch: "branch",
       expired: false,
       preferences: {
@@ -342,106 +342,141 @@ decorate_task(
   }
 );
 
 // startObserver should throw if an observer for the experiment is already
 // active.
 decorate_task(
   withMockExperiments(),
   async function() {
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "newvalue");
+    PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "newvalue"}});
     Assert.throws(
-      () => PreferenceExperiments.startObserver("test", "another.fake", "string", "othervalue"),
+      () => PreferenceExperiments.startObserver("test", {"another.fake": {preferenceType: "string", preferenceValue: "othervalue"}}),
       /observer.*is already active/i,
-      "startObserver threw due to a conflicting active observer",
+      "startObservers threw due to a conflicting active observer",
     );
     PreferenceExperiments.stopAllObservers();
   }
 );
 
-// startObserver should register an observer that calls stop when a preference
+// startObserver should register an observer that calls stop when *any* preference
 // changes from its experimental value.
 decorate_task(
   withMockExperiments(),
   withMockPreferences,
-  async function(mockExperiments, mockPreferences) {
-    const tests = [
-      ["string", "startvalue", "experimentvalue", "newvalue"],
-      ["boolean", false, true, false],
-      ["integer", 1, 2, 42],
-    ];
+  async function testObserversCanObserveChanges(mockExperiments, mockPreferences) {
+    const preferences = {
+      "fake.preferencestring": {
+        preferenceType: "string",
+        previousPreferenceValue: "startvalue",
+        preferenceValue: "experimentvalue",
+      },
+      // "newvalue",
+      "fake.preferenceboolean": {
+        preferenceType: "boolean",
+        previousPreferenceValue: false,
+        preferenceValue: true,
+      }, // false
+      "fake.preferenceinteger": {
+        preferenceType: "integer",
+        previousPreferenceValue: 1,
+        preferenceValue: 2,
+      }, // 42
+    };
+    const newValues = {
+      "fake.preferencestring": "newvalue",
+      "fake.preferenceboolean": false,
+      "fake.preferenceinteger": 42,
+    };
 
-    for (const [type, startvalue, experimentvalue, newvalue] of tests) {
+    for (const [testPref, newValue] of Object.entries(newValues)) {
       const stop = sinon.stub(PreferenceExperiments, "stop");
-      mockPreferences.set("fake.preference" + type, startvalue);
+      for (const [prefName, prefInfo] of Object.entries(preferences)) {
+        mockPreferences.set(prefName, prefInfo.previousPreferenceValue);
+      }
 
       // NOTE: startObserver does not modify the pref
-      PreferenceExperiments.startObserver("test" + type, "fake.preference" + type, type, experimentvalue);
+      PreferenceExperiments.startObserver("test" + testPref, preferences);
 
       // Setting it to the experimental value should not trigger the call.
-      mockPreferences.set("fake.preference" + type, experimentvalue);
-      ok(!stop.called, "Changing to the experimental pref value did not trigger the observer");
+      for (const [prefName, prefInfo] of Object.entries(preferences)) {
+        mockPreferences.set(prefName, prefInfo.preferenceValue);
+        ok(!stop.called, "Changing to the experimental pref value did not trigger the observer");
+      }
 
       // Setting it to something different should trigger the call.
-      mockPreferences.set("fake.preference" + type, newvalue);
+      mockPreferences.set(testPref, newValue);
       ok(stop.called, "Changing to a different value triggered the observer");
 
       PreferenceExperiments.stopAllObservers();
       stop.restore();
     }
   }
 );
 
 decorate_task(
   withMockExperiments(),
   async function testHasObserver() {
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentValue");
+    PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "experimentValue"}});
 
     ok(await PreferenceExperiments.hasObserver("test"), "hasObserver should detect active observers");
     ok(
       !(await PreferenceExperiments.hasObserver("missing")),
       "hasObserver shouldn't detect inactive observers",
     );
 
     PreferenceExperiments.stopAllObservers();
   }
 );
 
 // stopObserver should throw if there is no observer active for it to stop.
 decorate_task(
   withMockExperiments(),
   async function() {
     Assert.throws(
-      () => PreferenceExperiments.stopObserver("neveractive", "another.fake", "othervalue"),
+      () => PreferenceExperiments.stopObserver("neveractive"),
       /no observer.*found/i,
       "stopObserver threw because there was not matching active observer",
     );
   }
 );
 
-// stopObserver should cancel an active observer.
+// stopObserver should cancel an active observers.
 decorate_task(
   withMockExperiments(),
   withMockPreferences,
   async function(mockExperiments, mockPreferences) {
+    const preferenceInfo = {
+      "fake.preferencestring": {
+        preferenceType: "string",
+        preferenceValue: "experimentvalue",
+      },
+      "fake.preferenceinteger": {
+        preferenceType: "integer",
+        preferenceValue: 2,
+      },
+    };
     const stop = sinon.stub(PreferenceExperiments, "stop");
     mockPreferences.set("fake.preference", "startvalue");
 
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
+    PreferenceExperiments.startObserver("test", preferenceInfo);
     PreferenceExperiments.stopObserver("test");
 
     // Setting the preference now that the observer is stopped should not call
     // stop.
-    mockPreferences.set("fake.preference", "newvalue");
-    ok(!stop.called, "stopObserver successfully removed the observer");
+    mockPreferences.set("fake.preferencestring", "newvalue");
+    ok(!stop.called, "stopObserver successfully removed the observer for string");
+
+    mockPreferences.set("fake.preferenceinteger", 42);
+    ok(!stop.called, "stopObserver successfully removed the observer for integer");
 
     // Now that the observer is stopped, start should be able to start a new one
     // without throwing.
     try {
-      PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
+      PreferenceExperiments.startObserver("test", preferenceInfo);
     } catch (err) {
       ok(false, "startObserver did not throw an error for an observer that was already stopped");
     }
 
     PreferenceExperiments.stopAllObservers();
     stop.restore();
   }
 );
@@ -450,31 +485,31 @@ decorate_task(
 decorate_task(
   withMockExperiments(),
   withMockPreferences,
   async function(mockExperiments, mockPreferences) {
     const stop = sinon.stub(PreferenceExperiments, "stop");
     mockPreferences.set("fake.preference", "startvalue");
     mockPreferences.set("other.fake.preference", "startvalue");
 
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
-    PreferenceExperiments.startObserver("test2", "other.fake.preference", "string", "experimentvalue");
+    PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
+    PreferenceExperiments.startObserver("test2", {"other.fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
     PreferenceExperiments.stopAllObservers();
 
     // Setting the preference now that the observers are stopped should not call
     // stop.
     mockPreferences.set("fake.preference", "newvalue");
     mockPreferences.set("other.fake.preference", "newvalue");
     ok(!stop.called, "stopAllObservers successfully removed all observers");
 
     // Now that the observers are stopped, start should be able to start new
     // observers without throwing.
     try {
-      PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
-      PreferenceExperiments.startObserver("test2", "other.fake.preference", "string", "experimentvalue");
+      PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
+      PreferenceExperiments.startObserver("test2", {"other.fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
     } catch (err) {
       ok(false, "startObserver did not throw an error for an observer that was already stopped");
     }
 
     PreferenceExperiments.stopAllObservers();
     stop.restore();
   }
 );
@@ -562,17 +597,17 @@ decorate_task(
   withSendEventStub,
   async function testStop(experiments, mockPreferences, stopObserverSpy, sendEventStub) {
     // this assertion is mostly useful for --verify test runs, to make
     // sure that tests clean up correctly.
     is(Preferences.get("fake.preference"), null, "preference should start unset");
 
     mockPreferences.set(`${startupPrefs}.fake.preference`, "experimentvalue", "user");
     mockPreferences.set("fake.preference", "experimentvalue", "default");
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
+    PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
 
     await PreferenceExperiments.stop("test", {reason: "test-reason"});
     ok(stopObserverSpy.calledWith("test"), "stop removed an observer");
     const experiment = await PreferenceExperiments.get("test");
     is(experiment.expired, true, "stop marked the experiment as expired");
     is(
       DefaultPreferences.get("fake.preference"),
       "oldvalue",
@@ -611,17 +646,17 @@ decorate_task(
   })]),
   withMockPreferences,
   withStub(PreferenceExperiments, "stopObserver"),
   withStub(PreferenceExperiments, "hasObserver"),
   async function testStopUserPrefs(experiments, mockPreferences, stopObserver, hasObserver) {
     hasObserver.returns(true);
 
     mockPreferences.set("fake.preference", "experimentvalue", "user");
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
+    PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
 
     await PreferenceExperiments.stop("test");
     ok(stopObserver.calledWith("test"), "stop removed an observer");
     const experiment = await PreferenceExperiments.get("test");
     is(experiment.expired, true, "stop marked the experiment as expired");
     is(
       Preferences.get("fake.preference"),
       "oldvalue",
@@ -975,17 +1010,22 @@ decorate_task(
     stop.throws("Stop should not be called");
     mockPreferences.set("fake.preference", "experiment value", "default");
     is(Preferences.get("fake.preference"), "experiment value", "pref shouldn't have a user value");
     await PreferenceExperiments.init();
 
     ok(startObserver.calledOnce, "init should register an observer");
     Assert.deepEqual(
       startObserver.getCall(0).args,
-      ["test", "fake.preference", "string", "experiment value"],
+      ["test", {"fake.preference": {
+        preferenceType: "string",
+        preferenceValue: "experiment value",
+        previousPreferenceValue: "oldfakevalue",
+        preferenceBranchType: "default",
+      }}],
       "init should register an observer with the right args",
     );
   }
 );
 
 // saveStartupPrefs
 decorate_task(
   withMockExperiments([
@@ -1107,17 +1147,17 @@ decorate_task(
 );
 
 // test that default branch prefs restore to the right value if the default pref changes
 decorate_task(
   withMockExperiments(),
   withMockPreferences,
   withStub(PreferenceExperiments, "startObserver"),
   withStub(PreferenceExperiments, "stopObserver"),
-  async function testDefaultBranchStop(mockExperiments, mockPreferences, stopObserverStub) {
+  async function testDefaultBranchStop(mockExperiments, mockPreferences) {
     const prefName = "fake.preference";
     mockPreferences.set(prefName, "old version's value", "default");
 
     // start an experiment
     await PreferenceExperiments.start({
       name: "test",
       branch: "branch",
       preferenceName: prefName,
@@ -1155,17 +1195,17 @@ decorate_task(
 );
 
 // test that default branch prefs restore to the right value if the preference is removed
 decorate_task(
   withMockExperiments(),
   withMockPreferences,
   withStub(PreferenceExperiments, "startObserver"),
   withStub(PreferenceExperiments, "stopObserver"),
-  async function testDefaultBranchStop(mockExperiments, mockPreferences, stopObserverStub) {
+  async function testDefaultBranchStop(mockExperiments, mockPreferences) {
     const prefName = "fake.preference";
     mockPreferences.set(prefName, "old version's value", "default");
 
     // start an experiment
     await PreferenceExperiments.start({
       name: "test",
       branch: "branch",
       preferenceName: prefName,
@@ -1264,17 +1304,17 @@ decorate_task(
         previousPreferenceValue: "oldvalue",
         preferenceBranchType: "default",
       },
     },
   })]),
   async function testPrefChangeEventTelemetry(mockPreferences, sendEventStub, mockExperiments) {
     is(Preferences.get("fake.preference"), null, "preference should start unset");
     mockPreferences.set("fake.preference", "oldvalue", "default");
-    PreferenceExperiments.startObserver("test", "fake.preference", "string", "experimentvalue");
+    PreferenceExperiments.startObserver("test", {"fake.preference": {preferenceType: "string", preferenceValue: "experimentvalue"}});
 
     // setting the preference on the user branch should trigger the observer to stop the experiment
     mockPreferences.set("fake.preference", "uservalue", "user");
 
     // let the event loop tick to run the observer
     await Promise.resolve();
 
     sendEventStub.assertEvents(