Closed Bug 1093401 Opened 5 years ago Closed 5 years ago

MOZ_DATA_REPORTING is mis-commented or wrong


(Firefox Build System :: Android Studio and Gradle Integration, defect)

Not set



Tracking Status
fennec 35+ ---


(Reporter: rnewman, Assigned: nalexander)



(1 file)

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

For, e.g., this reason:

                // Display notification for Mozilla data reporting, if data should be collected.
                if (AppConstants.MOZ_DATA_REPORTING) {

Nick, could you double-check my reasoning?
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
The snippet rnewman shows is from mobile/android/ 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.

Flags: needinfo?(nalexander)
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 [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 " 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 Though it should be updated to include the STUMBLER flag, and also the (new) WiFi/AP flag.

I think liuche is right, we should modify the check and get rid of our local  I'll own this.
Assignee: nobody → nalexander
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.
Flags: needinfo?(mark.finkle)
tracking-fennec: ? → 35+
For all applications, MOZ_DATA_REPORTING is set in configure if any of
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)
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+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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.