Closed
Bug 1457620
Opened 6 years ago
Closed 6 years ago
Expose study opt-out string to all locales in Firefox Nightly for desktop only
Categories
(Firefox :: Normandy Client, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: mheubusch, Assigned: jkt)
References
Details
Attachments
(1 file)
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.
Comment 1•6 years ago
|
||
Paging :mythmon. :-)
Component: Preferences → Normandy Client
Flags: needinfo?(mcooper)
Comment 2•6 years ago
|
||
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.
Severity: normal → major
Flags: needinfo?(mcooper)
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
The patch attached makes Bug 1440156 a duplicate. However it is blocked by Bug 1435875.
Comment 6•6 years ago
|
||
mozreview-review |
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
``
Updated•6 years ago
|
Assignee: nobody → jkt
Comment 7•6 years ago
|
||
mozreview-review |
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]
Assignee | ||
Comment 8•6 years ago
|
||
> 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 hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-review |
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
Attachment #8973430 -
Flags: review?(francesco.lodolo) → review+
Assignee | ||
Comment 11•6 years ago
|
||
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
Flags: needinfo?(mheubusch)
Comment 12•6 years ago
|
||
mozreview-review |
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.
Attachment #8973430 -
Flags: review?(mcooper) → review+
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•6 years ago
|
Attachment #8973430 -
Flags: review?(gijskruitbosch+bugs)
Comment 14•6 years ago
|
||
mozreview-review |
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.
Attachment #8973430 -
Flags: review?(gijskruitbosch+bugs) → review-
Comment 15•6 years ago
|
||
(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?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
Removing strings mentioned in Comment 11 in this patch as it's not related anyway.
Flags: needinfo?(mheubusch)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•6 years ago
|
||
For some reason reviewboard won't let me flag you again.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 20•6 years ago
|
||
mozreview-review |
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).
Attachment #8973430 -
Flags: review+
Comment 21•6 years ago
|
||
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mcooper)
Comment 22•6 years ago
|
||
> 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.
Flags: needinfo?(mcooper)
Comment 23•6 years ago
|
||
(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.
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•6 years ago
|
||
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
Comment 28•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Assignee | ||
Comment 30•6 years ago
|
||
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.
Flags: needinfo?(mcooper)
Comment 31•6 years ago
|
||
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.
Flags: needinfo?(mcooper)
You need to log in
before you can comment on or make changes to this bug.
Description
•