The string in about:preferences#privacy that allows users to opt-out of studies currently only appears in EN-US. Please share this string so that it can be localized and appear in all locales for Nightly only. Priority locales for localization are de, fr, ru, and en-gb. Here is the string in en-us: Allow Firefox to install and run studies About Firefox Studies (links to about:studies) This change only needs to happen in Nightly.

Paging :mythmon. :-)

This isn't exactly a duplicate, but is something we've been thinking about for a while. See bug 1435875 and bug 1440156. I think any blockers and higher-priority work that we've had is done now. We should be ready to work on this.

The patch attached makes Bug 1440156 a duplicate. However it is blocked by Bug 1435875.

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review247732 Note that you need an r+ from a specific set of people to land patches with .ftl (I'll review the patch once updated). https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html ::: browser/locales/en-US/browser/preferences/preferences.ftl:876 (Diff revision 1) > .label = Allow { -brand-short-name } to send technical and interaction data to { -vendor-short-name } > .accesskey = r > collection-health-report-link = Learn more > > +collection-studies = > + .label = Allow Firefox to install and run studies Replace `Firefox` with `{ -brand-short-name }` ::: browser/locales/en-US/browser/preferences/preferences.ftl:877 (Diff revision 1) > .accesskey = r > collection-health-report-link = Learn more > > +collection-studies = > + .label = Allow Firefox to install and run studies > +collection-studies-link = View Firefox Studies Same here. I also don't think links should use title case? ``` collection-studies-link = View { -brand-short-name } studies ``

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review247734 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/normandy/lib/ShieldPreferences.jsm:21 (Diff revision 1) > this, "CleanupManager", "resource://normandy/lib/CleanupManager.jsm" > ); > > var EXPORTED_SYMBOLS = ["ShieldPreferences"]; > > const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; Error: 'xul_ns' is assigned a value but never used. [eslint: no-unused-vars]

> Same here. I also don't think links should use title case? This seems to be correct, I also fixed another "Learn More" and "Privacy Notice" too

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review247736 The .ftl looks good to me. I would suggest to double check with Michelle that we're doing the right thing on those links. In particular, "Privacy Notice" seems always to be spelled like that, excluding a single occurrence in Firefox for Android https://transvision.mozfr.org/?recherche=Privacy+notice&repo=gecko_strings&sourcelocale=en-US&locale=en-US&search_type=strings_entities

Hey Michelle, In the latest patch I modified some strings: https://reviewboard.mozilla.org/r/241838/diff/2/ - "Learn More" under the network proxy section to be "Learn more" - Should "Privacy Notice" be "Privacy notice"? (I discovered I missed one however I wanted to check we are consistent here) - The first is under "Firefox Account" at the bottom of the page - The second is under this section "Nightly Data Collection And Use" Thanks

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review247928 This looks good from a Normandy point of view. I have a minor comment, but I'd be fine either way. ::: toolkit/components/normandy/lib/ShieldPreferences.jsm:93 (Diff revision 2) > - 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" }); > > + doc.getElementById("optOutStudiesBox").hidden = false; Is there a reason to have this hidden by default, and then revealed with the code here? I think the code in this file runs unconditionally, so it would be safe to change the markup to set hidden to false in all cases.

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review247928 > Is there a reason to have this hidden by default, and then revealed with the code here? I think the code in this file runs unconditionally, so it would be safe to change the markup to set hidden to false in all cases. The reason I did this was that the section is only visible when "AppConstants.MOZ_DATA_REPORTING" is true.

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review248168 ::: toolkit/components/normandy/lib/ShieldPreferences.jsm:90 (Diff revision 2) > // 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" }); Seems to me this (and all the update logic) should also move into the prefs code (privacy.js), away from normandy itself.

(In reply to Jonathan Kingston [:jkt] from comment #13) > Comment on attachment 8973430 [details] > Bug 1457620 - Expose shield preference toggle to all locales and move code > to not being injected. > > https://reviewboard.mozilla.org/r/241838/#review247928 > > > Is there a reason to have this hidden by default, and then revealed with the code here? I think the code in this file runs unconditionally, so it would be safe to change the markup to set hidden to false in all cases. > > The reason I did this was that the section is only visible when > "AppConstants.MOZ_DATA_REPORTING" is true. The section only exists if MOZ_DATA_REPORTING is defined, though - it's in the ifdef. So could just not have it be hidden?

Removing strings mentioned in Comment 11 in this patch as it's not related anyway.

For some reason reviewboard won't let me flag you again.

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review248860 r=me with all the following minor points addressed. Thanks! ::: browser/components/preferences/in-content/privacy.js:1467 (Diff revision 4) > + // Preference instances for prefs that we need to monitor while the page is open. > + Preferences.add({ id: PREF_OPT_OUT_STUDIES_ENABLED, type: "bool" }); > + > + const checkbox = document.getElementById("optOutStudiesEnabled"); > + > + // Weirdly, FHR doesn't have a Preference instance on the page, so we create it. > + const fhrPref = Preferences.add({ id: PREF_FHR_UPLOAD_ENABLED, type: "bool" }); Can you add both these pref entries at the top of the file, after the giant list, using addAll(), behind an `AppConstants.MOZ_DATA_REPORTING` check? ::: browser/components/preferences/in-content/privacy.js:1487 (Diff revision 4) > + // 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 Looks like this got renamed, but please clarify that this method is not called, and the section is not shown, if the build was compiled with MOZ_DATA_REPORTING set to false. ::: browser/components/preferences/in-content/privacy.js:1493 (Diff revision 4) > + // * the policy allows Shield > + // * the FHR pref is true > + // * Normandy is enabled > + > + const checkboxMatchesPref = ( > + AppConstants.MOZ_DATA_REPORTING && This code won't run if this is false, so you can omit this check. ::: browser/components/preferences/in-content/privacy.js:1500 (Diff revision 4) > + Services.prefs.getBoolPref(PREF_FHR_UPLOAD_ENABLED, false) && > + Services.prefs.getBoolPref(PREF_NORMANDY_ENABLED, false) > + ); > + > + if (checkboxMatchesPref) { > + if (Services.prefs.getBoolPref(PREF_OPT_OUT_STUDIES_ENABLED)) { Can you add a default value to this getter (probably default to false) ? ::: browser/components/preferences/in-content/privacy.js:1514 (Diff revision 4) > + } > + > + const isDisabled = ( > + !allowedByPolicy || > + Services.prefs.prefIsLocked(PREF_OPT_OUT_STUDIES_ENABLED) || > + !Services.prefs.getBoolPref(PREF_FHR_UPLOAD_ENABLED) Add a default value here too please. ::: browser/components/preferences/in-content/privacy.js:1527 (Diff revision 4) > + checkbox.removeAttribute("disabled"); > + } > + } > + fhrPref.on("change", updateStudyCheckboxState); > + updateStudyCheckboxState(); > + window.addEventListener("unload", () => fhrPref.off("change", updateStudyCheckboxState), { once: true }); Shouldn't need this anymore now that it all lives inside the pref page! :D ::: toolkit/components/normandy/lib/ShieldPreferences.jsm:48 (Diff revision 4) > switch (prefName) { > // 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); > + case PREF_OPT_OUT_STUDIES_ENABLED: { > + prefValue = Services.prefs.getBoolPref(PREF_OPT_OUT_STUDIES_ENABLED); > if (!prefValue) { > for (const study of await AddonStudies.getAll()) { Question for mythmon: don't we need to actually stop pref experiments as well? What about rollouts - should those be governed by the opt-out or not? It seems odd to be able to turn off addon studies but not the other things (except by flicking the telemetry switch aka nuclear option).

(In reply to :Gijs (he/him) from comment #20) > Question for mythmon: don't we need to actually stop pref experiments as > well? What about rollouts - should those be governed by the opt-out or not? > It seems odd to be able to turn off addon studies but not the other things > (except by flicking the telemetry switch aka nuclear option). Pinging :mythmon for this.

> Can you add a default value to this getter (probably default to false) ? Shouldn't the pref enabling opt-out studies have a default value of true, since they are opt-out? > Question for mythmon: don't we need to actually stop pref experiments as well? What about rollouts - should those be governed by the opt-out or not? It seems odd to be able to turn off addon studies but not the other things (except by flicking the telemetry switch aka nuclear option). Regarding pref experiments, it would be preferable to include them in this as well, but they are currently not. I'd be open to that change, but it probably isn't in-scope for this bug. This is related to (but not the same as) bug 1447499. The context here is that pref experiments were implemented first. After that we learned a lot and had higher standards for add-on studies, and implemented them with about:studies and the opt-out preference. The newer standards haven't ever been back-ported to the older preference experiments. On the other hand, preference rollouts should *not* be governed by this switch, since they are be definition not studies/experimental. They are meant more as a replacement for simple system add-ons, and so shouldn't be controlled by the same opt-out setting. There should probably be a preference to disable those as well, but it should be separate.

(In reply to Michael Cooper [:mythmon] from comment #22) > > Can you add a default value to this getter (probably default to false) ? > > Shouldn't the pref enabling opt-out studies have a default value of true, > since they are opt-out? I guess it can have the same value as whatever we ship as the default for the pref. :-) > > Question for mythmon: don't we need to actually stop pref experiments as well? What about rollouts - should those be governed by the opt-out or not? It seems odd to be able to turn off addon studies but not the other things (except by flicking the telemetry switch aka nuclear option). > > Regarding pref experiments, it would be preferable to include them in this > as well, but they are currently not. I'd be open to that change, but it > probably isn't in-scope for this bug. This is related to (but not the same > as) bug 1447499. The context here is that pref experiments were implemented > first. After that we learned a lot and had higher standards for add-on > studies, and implemented them with about:studies and the opt-out preference. > The newer standards haven't ever been back-ported to the older preference > experiments. Right. I filed bug 1460680 > On the other hand, preference rollouts should *not* be governed by this > switch, since they are be definition not studies/experimental. They are > meant more as a replacement for simple system add-ons, and so shouldn't be > controlled by the same opt-out setting. There should probably be a > preference to disable those as well, but it should be separate. Not sure we need a pref for that, tbh... if we're using it as we'd use system add-ons, I'm not sure there were prefs to turn those off, either - and in any case, there's already a general normandy pref that users could use to turn all of it off, so adding more doesn't necessarily seem like a good idea.

Comment on attachment 8973430 [details] Bug 1457620 - Expose shield preference toggle to all locales and move code to not being injected. https://reviewboard.mozilla.org/r/241838/#review248860 > Looks like this got renamed, but please clarify that this method is not called, and the section is not shown, if the build was compiled with MOZ_DATA_REPORTING set to false. This is true, the method call itself is part of an existing check in privacy.js around line 386. ``` if (AppConstants.MOZ_DATA_REPORTING) { this.initDataCollection(); if (AppConstants.NIGHTLY_BUILD) { this.initCollectBrowserErrors(); } if (AppConstants.MOZ_CRASHREPORTER) { this.initSubmitCrashes(); } this.initSubmitHealthReport(); setEventListener("submitHealthReportBox", "command", gPrivacyPane.updateSubmitHealthReport); this.initOptOutStudyCheckbox(); ``` I will file a need info with mythmon if we need that check putting back in based on the comment I just removed.

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5bbecfa4b58c Expose shield preference toggle to all locales and move code to not being injected. r=flod,Gijs,mythmon

Verified in 62.0a1 (2018-05-12) (64 bit)

In the previous code there was a reference to "MOZ_TELEMETRY_REPORTING is true" however the code didn't check that as Gijs pointed out in Comment 20. Should it be the case it's only enabled when "MOZ_DATA_REPORTING" is true or does "MOZ_TELEMETRY_REPORTING" also have to be true? If this is the case we should file a follow up bug for adding that in.

I'm actually not sure. I don't know the difference between these two constants, and I expect that I got them confused when writing the code originally. Whichever of these preferences affects the checkbox above the study checkbox should be the one that it used. That should also be the constant used in toolkit/components/normandy/lib/RecipeRunner.jsm. If any of these don't match, then that is a bug.