Telemetry reuploader follow-up: address confusion over NetworkUtils naming

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 49
All
Android
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

From bug 1243585: nit: From the method signature (of NetworkUtils.isBackgroundDataEnabled) I expect this to return false if the user has restricted background data. But it seems like this is not necessarily the case because this method also checks if I'm currently connected or connecting to a network.
Created attachment 8754535 [details]
MozReview Request: Bug 1269924 - Move telemetry upload to NetworkUtils.isConnected. r=grisha

The concept of "background data" (as it exists in the Android options menu)
doesn't exist in the Android APIs - I think it should be covered under
isConnected. Thus, I removed our `isBackgroundDataEnabled` method.

One other network consideration, however: we may want to consider stopping
uploads on roaming.

Review commit: https://reviewboard.mozilla.org/r/54004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54004/
Attachment #8754535 - Flags: review?(gkruglov)

Comment 2

2 years ago
Comment on attachment 8754535 [details]
MozReview Request: Bug 1269924 - Move telemetry upload to NetworkUtils.isConnected. r=grisha

https://reviewboard.mozilla.org/r/54004/#review51018

Agreed on removing the method. It's misnamed for one, and interplay between `isConnecting` and `isConnected` could be pretty subtle and hard to get right.

I would definitely reshuffle checks in `isReadyToUpload` as mentioned in the review comment, while you're at it.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java:183
(Diff revision 1)
>          if (!GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true)) {
>              Log.d(LOGTAG, "Telemetry upload opt-out");
>              return false;
>          }
>  
> -        if (!NetworkUtils.isBackgroundDataEnabled(context)) {
> +        if (!NetworkUtils.isConnected(context)) {

Can we move the connectivity check out of `isUploadEnabledByAppConfig`? It seems like a strange thing to do in this method.

I thing that the caller of `isUploadEnabledByAppConfig` should just explicitely check if there's data connectivity, instead of bundling the two checks into one static method.
Attachment #8754535 - Flags: review?(gkruglov)
Comment on attachment 8754535 [details]
MozReview Request: Bug 1269924 - Move telemetry upload to NetworkUtils.isConnected. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54004/diff/1-2/
Attachment #8754535 - Flags: review?(gkruglov)

Comment 4

2 years ago
Comment on attachment 8754535 [details]
MozReview Request: Bug 1269924 - Move telemetry upload to NetworkUtils.isConnected. r=grisha

https://reviewboard.mozilla.org/r/54004/#review51554

+1
Attachment #8754535 - Flags: review?(gkruglov) → review+

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87a6b7a49606
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.