Closed
Bug 1093401
Opened 10 years ago
Closed 10 years ago
MOZ_DATA_REPORTING is mis-commented or wrong
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(fennec35+)
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
fennec | 35+ | --- |
People
(Reporter: rnewman, Assigned: nalexander)
Details
Attachments
(1 file)
1.58 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
We do a lot of data reporting -- health report, telemetry, crashes, etc. This flag controls whether we show the data reporting notification (which we must always show if we're doing any of those things), and whether we include data reporting prefs in Settings. As far as I can tell, everything about this is wrong: # Wifi-AP/cell tower data reporting is enabled on non-release builds. if test ! "$RELEASE_BUILD"; then MOZ_DATA_REPORTING=1 fi For, e.g., this reason: // Display notification for Mozilla data reporting, if data should be collected. if (AppConstants.MOZ_DATA_REPORTING) { DataReportingNotification.checkAndNotifyPolicy(GeckoAppShell.getContext()); } Nick, could you double-check my reasoning?
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
Assignee | ||
Comment 1•10 years ago
|
||
The snippet rnewman shows is from mobile/android/confvars.sh and I agree it looks wrong. It was "regressed" -- changed, at least -- by a few different patches, most recently blassey. It sure looks like the original commit [1] adding the comment was incorrect, since MOZ_DATA_REPORTING has always meant more than just Wifi/AP info. So yeah, this is busted. [1] http://hg.mozilla.org/mozilla-central/rev/2ff16cbb2830
Flags: needinfo?(nalexander)
Comment 2•10 years ago
|
||
Yes, MOZ_DATA_REPORTING should reflect whether any data is being uploaded by default - this should depend on FHR, telemetry, crashreporter, and geo, correct? I don't think that's something that we should be setting independently - aren't we controlling that in configure.in [1]? AFAICT, we don't have a flag for WiFi/AP, and we should. The original comment for MOZ_DATA_REPORTING was incomplete I guess, and should have included "...so MOZ_DATA_REPORTING is always enabled" - it implicitly folded control of WiFi/AP into that flag with the assumption it would always be enabled. Correct me if I'm wrong, but can we just rely on configure.in? Though it should be updated to include the STUMBLER flag, and also the (new) WiFi/AP flag. [1] http://mxr.mozilla.org/mozilla-central/source/configure.in#8759
Assignee | ||
Comment 3•10 years ago
|
||
I think liuche is right, we should modify the configure.in check and get rid of our local confvars.sh. I'll own this.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
Glancing through the bug chain, I remember why we didn't have a separate pref for WiFi/AP - there's no build flag for it, so this was a mobile/android only pref to reflect the build state for WiFi/AP. See bug 884499.
Updated•10 years ago
|
Flags: needinfo?(mark.finkle)
Updated•10 years ago
|
tracking-fennec: ? → 35+
Assignee | ||
Comment 5•10 years ago
|
||
For all applications, MOZ_DATA_REPORTING is set in configure if any of MOZ_TELEMETRY_REPORTING, MOZ_SERVICES_HEALTHREPORT, or MOZ_CRASHREPORTER is set. For mobile/android, we *also* set MOZ_DATA_REPORTING when we're not in a release (Beta/Release) build. Geo/stumbler data is build-time enabled by MOZ_ANDROID_MLS_STUMBLER but does not automatically upload data: the user must manually enable uploading geo/stumbler data. That is, this is an explicit opt-in rather than an explicit opt-out; and geo/stumbler data should not be covered by the data reporting notification at this time. In the past, I believe that geo/stumbler data was uploaded based on the feature being build time enabled, which corresponded to !RELEASE_BUILD, so the logic being removed was reasonable. rnewman: do you agree?
Attachment #8521787 -
Flags: review?(rnewman)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8521787 [details] [diff] [review] Only set MOZ_DATA_REPORTING in configure. Review of attachment 8521787 [details] [diff] [review]: ----------------------------------------------------------------- If location service is off by default, then sgtm.
Attachment #8521787 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/474d4e4f6567
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/474d4e4f6567
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 36 → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•