I'd like to understand how much of MOZ_TELEMETRY_REPORTING is still needed in frontend code. We have only a few points in browser code using it, and that may cause more problems than benefits imo, because the code that we (developers) run differs from the code that we ship. For example this caused bug 1306639, where I thought we regressed telemetry, but in reality I always tested with my own build where MOZ_TELEMETRY_REPORTING was not defined, and thus it was missing a notification observer. Having sliced out code in developer builds is scary, it may either hide bugs or create them. This is the points where this is defined: http://searchfox.org/mozilla-central/search?q=MOZ_TELEMETRY_REPORTING&path= I think at least some of the uses from browser/ should go away, do we plan to ship browser without telemetry? Is this for non official builds? If so, I think it would be ok if we only hide the UI parts, but we should not remove code like the one in nsBrowserGlue that is already excluded by the telemetry backend. I'm not sure if there's any other point that we could remove or cleanup, and that's why I'm filing this under telemetry. Could you please audit the callpoints and tell what can go away?

(In reply to Marco Bonardo [::mak] from comment #0) > I think at least some of the uses from browser/ should go away, do we plan > to ship browser without telemetry? Is this for non official builds? > If so, I think it would be ok if we only hide the UI parts, but we should > not remove code like the one in nsBrowserGlue that is already excluded by > the telemetry backend. In bug 1200164, we made MOZ_TELEMETRY_REPORTING control the upload of the Telemetry pings. Long story short, without this definition telemetry should still be gathered (e.g. for dev builds) but must not be sent. With that context, I think that what you found is a bug: we should not be defending the call sites in nsBrowserGlue.js with MOZ_TELEMETRY_REPORTING. > I'm not sure if there's any other point that we could remove or cleanup, and > that's why I'm filing this under telemetry. > Could you please audit the callpoints and tell what can go away? The rest of the call sites are for the Preferences UI. I think these should be changed as well, as we would always want to show the UI if Telemetry is recording. Otherwise, we end up with bugs like bug 1284010. Georg, any opinion on the points above? [1] - http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/toolkit/components/telemetry/Telemetry.cpp#1898

After discussing this a bit, I've attempted to get rid of all the references to MOZ_TELEMETRY_REPORTING in the front end code. As mentioned in comment 1, MOZ_TELEMETRY_REPORTING controls if pings get uploaded to our servers, not the recording itself. For this reason, it should not be used to enable or disable UI controls. This patch does 2 things: - Removes the MOZ_TELEMETRY_REPORTING wrapping for the keyword-search feature in nsBrowserGlue.js - Always enables the Preferences entries for Telemetry, but makes them "disabled" if MOZ_TELEMETRY_REPORTING is not defined, as the preferences explicitly mention "sharing" and "sending", so they refer to ping uploads. Georg, do these changes make sense for you? We should probably also require somebody else to review the code changes, especially the ones for the Preferences panel.

Mh, I wonder if we could simply use |telemetry.isOfficialTelemetry| for enabling/disabling the UI parts instead of relying solely on MOZ_TELEMETRY_REPORTING. We're relying in the former for sending [1]. [1] - https://dxr.mozilla.org/mozilla-central/rev/79feeed4293336089590320a9f30a813fade8e3c/toolkit/components/telemetry/TelemetrySend.jsm#1031

Comment on attachment 8811183 [details] [diff] [review] bug1312182.patch Jared, can you also take a look over the advanced.js changes? I think we should probably have a localized message there when the data settings are disabled, e.g. "Data reporting is not enabled for this build configuration." Otherwise it is a bit confusing as to why this is greyed out / disabled.

Comment on attachment 8811183 [details] [diff] [review] bug1312182.patch Review of attachment 8811183 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/advanced.js @@ +246,5 @@ > { > + this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore"); > + // If we're not sending any Telemetry, disble The telemetry one as well. > + if (!AppConstants.MOZ_TELEMETRY_REPORTING) { > + document.getElementById("submitTelemetryBox").setAttribute("disabled", "true"); I agree with Georg, we should have a localizable message here that is displayed when these controls are disabled.

@gfritzsche: I've added a localized string as requested by you and jaws. Do we need to bring someone from the UX on board for checking that? I've attached a screenshot to see how it looks like. @flod: would you kindly check the localization bits? This patch adds a new string that is only shown on builds that are not sending any Telemetry.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > Comment on attachment 8811183 [details] [diff] [review] > bug1312182.patch > > Review of attachment 8811183 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/preferences/in-content/advanced.js > @@ +246,5 @@ > > { > > + this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore"); > > + // If we're not sending any Telemetry, disble The telemetry one as well. > > + if (!AppConstants.MOZ_TELEMETRY_REPORTING) { > > + document.getElementById("submitTelemetryBox").setAttribute("disabled", "true"); > > I agree with Georg, we should have a localizable message here that is > displayed when these controls are disabled. Thanks :jaws!

Comment on attachment 8818535 [details] [diff] [review] bug1312182.patch Review of attachment 8818535 [details] [diff] [review]: ----------------------------------------------------------------- The l10n bits look good (I don't see a screenshot though). ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd @@ +29,5 @@ > <!ENTITY checkSpelling.accesskey "t"> > > <!ENTITY dataChoicesTab.label "Data Choices"> > > +<!ENTITY healthReportingDisabled.label "Data reporting is not enabled for this build configuration"> I wonder if "available" is more suitable for this case?

Telemetry supported vs Telemetry unsupported screenshot.

(In reply to Francesco Lodolo [:flod] (mostly out of office until Dec 19) from comment #8) > Comment on attachment 8818535 [details] [diff] [review] > bug1312182.patch > > Review of attachment 8818535 [details] [diff] [review]: > ----------------------------------------------------------------- > > The l10n bits look good (I don't see a screenshot though). > > ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd > @@ +29,5 @@ > > <!ENTITY checkSpelling.accesskey "t"> > > > > <!ENTITY dataChoicesTab.label "Data Choices"> > > > > +<!ENTITY healthReportingDisabled.label "Data reporting is not enabled for this build configuration"> > > I wonder if "available" is more suitable for this case? Ouch, I forgot to attach the screenshot. Thank you for catching that. I'll update the message once I get the review from Georg as well!

Side question (coffee didn't fully kick in yet…): is this easily reproducible on a standard build? My guess would be not, in which case maybe we should add a note. Example: <!-- LOCALIZATION NOTE (healthReportingDisabled.label): This message is displayed above disabled data sharing options in special builds. -->

(In reply to Francesco Lodolo [:flod] (mostly out of office until Dec 19) from comment #11) > Side question (coffee didn't fully kick in yet…): is this easily > reproducible on a standard build? If you clone m-c and build without providing MOZ_TELEMETRY_REPORTING in the .mozconfig file, you'd see the message. This is also the case for third party packaged builds with no Telemetry support (Fedora, Debian, archlinux, etc.). Basically, you should be able to see this message unless it's an official build by us. But I think it makes sense to leave a note there, I'll do that.

Comment on attachment 8818580 [details] datareporting.png :phlsa, flagging you on this one as I'm not sure who's the best person to ask :) Background: since builds compiled directly from mozilla-central and third party builds (Firefox packed in Fedora, ArchLinux, etc.) cannot send Telemetry, we're disabling the options in the Preferences and showing a clear message explaining why. From an UX perspective, does the new look (below, in the screenshot) look reasonable?

Comment on attachment 8818535 [details] [diff] [review] bug1312182.patch Review of attachment 8818535 [details] [diff] [review]: ----------------------------------------------------------------- Generally this looks fine to me, i left some comments to follow up on below. ::: browser/base/content/baseMenuOverlay.xul @@ -60,5 @@ > oncommand="openHelpLink('keyboard-shortcuts')" > onclick="checkForMiddleClick(this, event);" > label="&helpKeyboardShortcuts.label;" > accesskey="&helpKeyboardShortcuts.accesskey;"/> > -#ifdef MOZ_TELEMETRY_REPORTING We should not remove this, but change it to MOZ_SERVICES_HEALTHREPORT, as that is the flag that controls whether we have about:healthreport: https://dxr.mozilla.org/mozilla-central/rev/2785aaf276ba29fb2e1f5607d90d441fee42efb4/browser/base/jar.mn#49 ::: browser/components/preferences/in-content/advanced.js @@ +252,5 @@ > */ > initTelemetry: function() > { > + this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore"); > + // If we're not sending any Telemetry, disble The telemetry one as well. "disable the Telemetry" Also, do you mean the "Telemetry upload checkbox" or so? "the Telemetry one" is vague. @@ +254,5 @@ > { > + this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore"); > + // If we're not sending any Telemetry, disble The telemetry one as well. > + if (!AppConstants.MOZ_TELEMETRY_REPORTING) { > + document.getElementById("submitTelemetryBox").setAttribute("disabled", "true"); When, using a dev build with toolkit.telemetry.enabled;true, i open preferences and this triggers... Will this trigger setTelemetrySectionEnabled() and disable my extended Telemetry recording? Ditto with all the checkbox setting below.

(In reply to Alessio Placitelli [:Dexter] from comment #13) > Comment on attachment 8818580 [details] > datareporting.png > > :phlsa, flagging you on this one as I'm not sure who's the best person to > ask :) > > Background: since builds compiled directly from mozilla-central and third > party builds (Firefox packed in Fedora, ArchLinux, etc.) cannot send > Telemetry, we're disabling the options in the Preferences and showing a > clear message explaining why. > > From an UX perspective, does the new look (below, in the screenshot) look > reasonable? Sorry, took me forever to respond... Yes, looks reasonable!

Comment on attachment 8826139 [details] [diff] [review] bug1312182.patch (In reply to Georg Fritzsche [:gfritzsche] from comment #14) > @@ +254,5 @@ > > { > > + this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore"); > > + // If we're not sending any Telemetry, disble The telemetry one as well. > > + if (!AppConstants.MOZ_TELEMETRY_REPORTING) { > > + document.getElementById("submitTelemetryBox").setAttribute("disabled", "true"); > > When, using a dev build with toolkit.telemetry.enabled;true, i open > preferences and this triggers... > Will this trigger setTelemetrySectionEnabled() and disable my extended > Telemetry recording? > > Ditto with all the checkbox setting below. I've updated the patch with the changes suggested by :Flod. I've also manually checked that the prefs behave consistently in developer builds, as follows: - I've manually set the uploadEnabled and telemetry.enabled prefs to true. - Opened the about:preferences - Navigated to the data choices section (it showed the first checkbox as locked & untick, the share additional data locked & ticked) - Closed the page - The value in the prefs did not change. That's because we're calling |initSubmitHealthReport| after checking if the pref is locked or MOZ_TELEMETRY_REPORTING is not set. But we return if that's the case. This leaves us with an inconsistent pref synchronization between UI and pref value: we're manually unticking the first checkbox (even if the pref is true), but leaving the second untouched (and so ticked if the pref is true). Should we care about that? Maybe a better option would be to just lock the options without actually ticking/unticking them if telemetry is not available.

Mass wontfix for bugs affecting firefox 52.

Comment on attachment 8826139 [details] [diff] [review] bug1312182.patch Cancelling the review: I'll rebase this and break it into UI and Telemetry specific changes to ease reviewing.

I've un-bitrot the patch and pushed it back on MozReview. The behaviour from comment 17 is preserved, there's only one major change: while the Telemetry prefs are now in privacy.xul, the localization strings are still in advanced.dtd, so I've added the new one there as well.

Comment on attachment 8872555 [details] Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. https://reviewboard.mozilla.org/r/144078/#review147798 Strings-wise it looks good. With prefs reorg, strings have been moved all over the place, so it's expected to find strings for privacy still in advanced.dtd, moving them around would create a lot of churn for localizers. Talking about prefs reorg: do you need this also in preferences-old, since the reorg is not moving to Beta with 55?

Comment on attachment 8872555 [details] Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. https://reviewboard.mozilla.org/r/144078/#review147798 Mh, I don't think that's needed: this change is only helpful for developers who build from mozilla-central.

Comment on attachment 8872555 [details] Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. https://reviewboard.mozilla.org/r/144078/#review149622 Thanks, this looks good. Can we get a safety QA pass scheduled on this for the "official" build configuration? ::: browser/components/preferences/in-content/privacy.xul:763 (Diff revision 2) > -#ifdef MOZ_TELEMETRY_REPORTING > -<groupbox id="historyGroup" data-category="panePrivacy" data-subcategory="reports" hidden="true"> > +#ifdef MOZ_DATA_REPORTING > +<groupbox id="telemetryGroup" data-category="panePrivacy" data-subcategory="reports" hidden="true"> Nit: Keep the empty line, it seems to match the `#endif` style. ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:34 (Diff revision 2) > <!ENTITY dataChoicesTab.label "Data Choices"> > > +<!-- LOCALIZATION NOTE (healthReportingDisabled.label): This message is displayed above > +disabled data sharing options in developer builds or builds with no Telemetry support > +available. --> > +<!ENTITY healthReportingDisabled.label "Data reporting is not available for this build configuration"> Maybe "Data reporting is disabled for ..."?

Ouch, had to do some fun rebasing due to 1363850 which renames some directories :)

Comment on attachment 8872555 [details] Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. https://reviewboard.mozilla.org/r/144078/#review149734 Please update browser/components/preferences/in-content/\* as well. ::: browser/components/preferences/in-content-new/privacy.js:1462 (Diff revision 4) > > let checkbox = document.getElementById("submitHealthReportBox"); > > - if (Services.prefs.prefIsLocked(PREF_UPLOAD_ENABLED)) { > + // Telemetry is only sending data if MOZ_TELEMETRY_REPORTING is defined. > + // We still want to display the preferences panel if that's not the case, but > + // we want it to be disabled and unckecked. spelling error, "unchecked" instead of "unckecked"

Comment on attachment 8872555 [details] Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. https://reviewboard.mozilla.org/r/144078/#review153038 Thanks!

Hi Madalin! Would you kindly do a QA pass on this before we land it? The builds ara available here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45bba2ca0841ab5d45fbb1678cbba5c609eb4559&selectedJob=106941703 We want to make sure that: - Developer builds (no MOZ_TELEMETRY_REPORTING defined when building) still show the Telemetry options in about:preferences (currently they don't), but that they look as in the attached screenshot; - The state of the options in about:preferences reflect the state of the telemetry prefs, toolkit.telemetry.enabled and datareporting.healthreport.uploadEnabled; - There is no regression on non-developer/official builds: the UI prefs are still shown and control the preferences. Please let me know if you need more input on this.

Mh, you know what? let's land this anyway, it shouldn't bite us on official builds.

Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7f1e5af6138b Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. r=flod,gfritzsche,jaws

Backed out for failing e.g. browser-chrome's browser_storagePressure_notification.js because of entity parsing error for healthReportingDisabled.label: https://hg.mozilla.org/integration/autoland/rev/ffe216fab0e7a8449b328b181d8523aaabd91dfe Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7f1e5af6138bbcbd7c3ef73d37e50b91c52e18e1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log browser_storagePressure_notification.js: https://treeherder.mozilla.org/logviewer.html#?job_id=107058097&repo=autoland [task 2017-06-14T15:26:23.727717Z] 15:26:23 INFO - TEST-PASS | browser/base/content/test/general/browser_storagePressure_notification.js | Should display storage pressure notification - [task 2017-06-14T15:26:23.729694Z] 15:26:23 INFO - Console message: [JavaScript Error: "Not a number"] [task 2017-06-14T15:26:23.733059Z] 15:26:23 INFO - Console message: [JavaScript Error: "XML Parsing Error: undefined entity [task 2017-06-14T15:26:23.736104Z] 15:26:23 INFO - Location: about:preferences#advanced [task 2017-06-14T15:26:23.738308Z] 15:26:23 INFO - Line Number 1099, Column 20:" {file: "about:preferences#advanced" line: 1099 column: 20 source: " <description>&healthReportingDisabled.label;</description>"}] [task 2017-06-14T15:26:23.743106Z] 15:26:23 INFO - Buffered messages finished [task 2017-06-14T15:26:23.748203Z] 15:26:23 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_storagePressure_notification.js | Test timed out - More browser-chrome tests ran and failed for https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4180fb6074b11fa29119cdef6a90432be01f0036&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

> [task 2017-06-14T15:26:23.738308Z] 15:26:23 INFO - Line Number 1099, > Column 20:" {file: "about:preferences#advanced" line: 1099 column: 20 > source: " <description>&healthReportingDisabled.label;</description>"}] I'm failing to spot anything that justifies this one.

The failure is because the string was only added to browser/locales/en-US/chrome/browser/preferences/advanced.dtd but should also be added to browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd too. We forked the preference strings along with the fork of the preferences.

Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c74bea253277 Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. r=flod,gfritzsche,jaws

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #38) > The failure is because the string was only added to > browser/locales/en-US/chrome/browser/preferences/advanced.dtd but should > also be added to > browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd too. We > forked the preference strings along with the fork of the preferences. Thanks Jared (& flod too)!

I have created a set of tests to validate this. They can be found in here: https://docs.google.com/document/d/17vRzqXk2ZdlKL6wsMaekQGCakEfppIAAeSPE_ND58Tg/edit Also I have done some exploratory checking around those scenario. The testing was don in latest nightly and a developer build provided by Alessio. Closing this bug as verified fixed.

(In reply to Madalin Cotetiu from comment #43) > I have created a set of tests to validate this. They can be found in here: > https://docs.google.com/document/d/ > 17vRzqXk2ZdlKL6wsMaekQGCakEfppIAAeSPE_ND58Tg/edit > Also I have done some exploratory checking around those scenario. > The testing was don in latest nightly and a developer build provided by > Alessio. > Closing this bug as verified fixed. Great job, thanks Madalin!