Bug 1447226 - Don't show shield study opt-in as checked when studies won't run r=Gijs
authorMike Cooper <mcooper@mozilla.com>
Thu, 29 Mar 2018 16:23:21 -0700
changeset 468022 f7f47849e748a4509be6005d060be7e09f537495
parent 468021 130ce494ee0e3d54c7926aec8988b115f9b1dad9
child 468023 64ec147eb854e6beefd2efae5e465de99bbfe97d
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1447226
milestone61.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 1447226 - Don't show shield study opt-in as checked when studies won't run r=Gijs MozReview-Commit-ID: 51TycE6mLcq
toolkit/components/normandy/lib/ShieldPreferences.jsm
--- a/toolkit/components/normandy/lib/ShieldPreferences.jsm
+++ b/toolkit/components/normandy/lib/ShieldPreferences.jsm
@@ -17,27 +17,23 @@ ChromeUtils.defineModuleGetter(
 );
 
 var EXPORTED_SYMBOLS = ["ShieldPreferences"];
 
 const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
 const NS_PREFBRANCH_PREFCHANGE_TOPIC_ID = "nsPref:changed"; // from modules/libpref/nsIPrefBranch.idl
 const FHR_UPLOAD_ENABLED_PREF = "datareporting.healthreport.uploadEnabled";
 const OPT_OUT_STUDIES_ENABLED_PREF = "app.shield.optoutstudies.enabled";
+const NORMANDY_ENABLED_PREF = "app.normandy.enabled";
 
 /**
  * Handles Shield-specific preferences, including their UI.
  */
 var ShieldPreferences = {
   init() {
-    // If the FHR pref was disabled since our last run, disable opt-out as well.
-    if (!Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF)) {
-      Services.prefs.setBoolPref(OPT_OUT_STUDIES_ENABLED_PREF, false);
-    }
-
     // Watch for changes to the FHR pref
     Services.prefs.addObserver(FHR_UPLOAD_ENABLED_PREF, this);
     CleanupManager.addCleanupHandler(() => {
       Services.prefs.removeObserver(FHR_UPLOAD_ENABLED_PREF, this);
     });
 
     // Watch for changes to the Opt-out pref
     Services.prefs.addObserver(OPT_OUT_STUDIES_ENABLED_PREF, this);
@@ -66,23 +62,16 @@ var ShieldPreferences = {
         this.observePrefChange(data);
         break;
     }
   },
 
   async observePrefChange(prefName) {
     let prefValue;
     switch (prefName) {
-      // If the FHR pref changes, set the opt-out-study pref to the value it is changing to.
-      case FHR_UPLOAD_ENABLED_PREF: {
-        prefValue = Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF);
-        Services.prefs.setBoolPref(OPT_OUT_STUDIES_ENABLED_PREF, prefValue);
-        break;
-      }
-
       // If the opt-out pref changes to be false, disable all current studies.
       case OPT_OUT_STUDIES_ENABLED_PREF: {
         prefValue = Services.prefs.getBoolPref(OPT_OUT_STUDIES_ENABLED_PREF);
         if (!prefValue) {
           for (const study of await AddonStudies.getAll()) {
             if (study.active) {
               await AddonStudies.stop(study.recipeId, "general-opt-out");
             }
@@ -93,65 +82,96 @@ var ShieldPreferences = {
     }
   },
 
   /**
    * Injects the opt-out-study preference checkbox into about:preferences and
    * handles events coming from the UI for it.
    */
   injectOptOutStudyCheckbox(doc) {
+    const allowedByPolicy = Services.policies.isAllowed("Shield");
+
     const container = doc.createElementNS(XUL_NS, "vbox");
     container.classList.add("indent");
 
     const hContainer = doc.createElementNS(XUL_NS, "hbox");
     hContainer.setAttribute("align", "center");
     container.appendChild(hContainer);
 
     const checkbox = doc.createElementNS(XUL_NS, "checkbox");
     checkbox.setAttribute("id", "optOutStudiesEnabled");
     checkbox.setAttribute("class", "tail-with-learn-more");
     checkbox.setAttribute("label", "Allow Firefox to install and run studies");
-
-    let allowedByPolicy = Services.policies.isAllowed("Shield");
-    if (allowedByPolicy) {
-      // If Shield is not allowed by policy, don't tie this checkbox to the preference,
-      // so that the checkbox remains unchecked.
-      // Otherwise, it would be grayed out but still checked, which looks confusing
-      // because it appears it's enabled with no way to disable it.
-      checkbox.setAttribute("preference", OPT_OUT_STUDIES_ENABLED_PREF);
-    }
     hContainer.appendChild(checkbox);
 
     const viewStudies = doc.createElementNS(XUL_NS, "label");
     viewStudies.setAttribute("id", "viewShieldStudies");
     viewStudies.setAttribute("href", "about:studies");
     viewStudies.setAttribute("useoriginprincipal", true);
     viewStudies.textContent = "View Firefox Studies";
     viewStudies.classList.add("learnMore", "text-link");
     hContainer.appendChild(viewStudies);
 
     // Preference instances for prefs that we need to monitor while the page is open.
     doc.defaultView.Preferences.add({ id: OPT_OUT_STUDIES_ENABLED_PREF, type: "bool" });
 
     // Weirdly, FHR doesn't have a Preference instance on the page, so we create it.
     const fhrPref = doc.defaultView.Preferences.add({ id: FHR_UPLOAD_ENABLED_PREF, type: "bool" });
-    function onChangeFHRPref() {
-      let isDisabled = Services.prefs.prefIsLocked(FHR_UPLOAD_ENABLED_PREF) ||
-                       !AppConstants.MOZ_TELEMETRY_REPORTING ||
-                       !Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF) ||
-                       !allowedByPolicy;
+    function updateStudyCheckboxState() {
+      // The checkbox should be disabled if any of the below are true. This
+      // prevents the user from changing the value in the box.
+      //
+      // * the policy forbids shield
+      // * the Shield Study preference is locked
+      // * the FHR pref is false
+      //
+      // The checkbox should match the value of the preference only if all of
+      // these are true. Otherwise, the checkbox should remain unchecked. This
+      // is because in these situations, Shield studies are always disabled, and
+      // so showing a checkbox would be confusing.
+      //
+      // * MOZ_TELEMETRY_REPORTING is true
+      // * the policy allows Shield
+      // * the FHR pref is true
+      // * Normandy is enabled
+
+      const checkboxMatchesPref = (
+        AppConstants.MOZ_DATA_REPORTING &&
+        allowedByPolicy &&
+        Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF, false) &&
+        Services.prefs.getBoolPref(NORMANDY_ENABLED_PREF, false)
+      );
+
+      if (checkboxMatchesPref) {
+        if (Services.prefs.getBoolPref(OPT_OUT_STUDIES_ENABLED_PREF)) {
+          checkbox.setAttribute("checked", "checked");
+        } else {
+          checkbox.removeAttribute("checked");
+        }
+        checkbox.setAttribute("preference", OPT_OUT_STUDIES_ENABLED_PREF);
+      } else {
+        checkbox.removeAttribute("preference");
+        checkbox.removeAttribute("checked");
+      }
+
+      const isDisabled = (
+        !allowedByPolicy ||
+        Services.prefs.prefIsLocked(OPT_OUT_STUDIES_ENABLED_PREF) ||
+        !Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF)
+      );
+
       // We can't use checkbox.disabled here because the XBL binding may not be present,
       // in which case setting the property won't work properly.
       if (isDisabled) {
         checkbox.setAttribute("disabled", "true");
       } else {
         checkbox.removeAttribute("disabled");
       }
     }
-    fhrPref.on("change", onChangeFHRPref);
-    onChangeFHRPref();
-    doc.defaultView.addEventListener("unload", () => fhrPref.off("change", onChangeFHRPref), { once: true });
+    fhrPref.on("change", updateStudyCheckboxState);
+    updateStudyCheckboxState();
+    doc.defaultView.addEventListener("unload", () => fhrPref.off("change", updateStudyCheckboxState), { once: true });
 
     // Actually inject the elements we've created.
     const parent = doc.getElementById("submitHealthReportBox").closest("description");
     parent.appendChild(container);
   },
 };