Bug 1447226 - Don't show shield study opt-in as checked when studies won't run r=Gijs draft
authorMike Cooper <mcooper@mozilla.com>
Thu, 29 Mar 2018 16:23:21 -0700
changeset 785087 71bf4e8f1d53f5de6ca69a45737ae1a45b5ee590
parent 784946 8ed49dd81059dfdd876cf62ad5def1cfa56ffbbf
push id107132
push userbmo:mcooper@mozilla.com
push dateThu, 19 Apr 2018 16:33:28 +0000
reviewersGijs
bugs1447226
milestone61.0a1
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);
   },
 };