Closed
Bug 981694
Opened 11 years ago
Closed 11 years ago
Show a notice to beta users when we turn telemetry on by default on the beta channel - Firefox for Android
Categories
(Firefox for Android Graveyard :: General, enhancement, P2)
Tracking
(firefox31+ fixed, fennec30+)
RESOLVED
FIXED
Firefox 31
People
(Reporter: benjamin, Assigned: liuche, NeedInfo)
References
Details
Attachments
(2 files, 2 obsolete files)
10.15 KB,
patch
|
liuche
:
review+
|
Details | Diff | Splinter Review |
8.55 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
We plan on turning Telemetry on by default on all prerelease channels. This means that telemetry will now be on by default on the beta channel when it was previously off. We will need to show some notice to users that the default has changed.
We did this once before in bug 725987 but that code has since been altered by the FHR notification. I don't know whether it would be sufficient to re-show the notification or whether we need something specifically calling out that there has been a change; Jishnu can clarify.
Flags: needinfo?(jmenon)
Reporter | ||
Updated•11 years ago
|
OS: All → Android
Summary: Show a notice to beta users when we turn telemetry on by default on the beta channel - Firefox Desktop → Show a notice to beta users when we turn telemetry on by default on the beta channel - Firefox for Android
Comment 1•11 years ago
|
||
Hi -
So here is the notice framework - we need to modify the privacy policy before landing the change:
User sees the following on first run or when we change telemetry status for current users: "%1$S automatically sends some data to %2$S so that we can improve your experience. [ Choose What I Share ]"
We need to modify the Firefox privacy policy - Ben can you confirm the accuracy? Geoff - can you please implement the change once Ben has confirmed?:
"Usage Statistics (also known as Telemetry). Beginning with version 7, Firefox includes functionality that sends Mozilla usage, performance, and responsiveness statistics about user interface features, memory, hardware configuration along with IP address.
This feature is turned off by default in general release versions of Firefox. In order to enable Nightly, Aurora and Beta testers to provide more efficient feedback, Usage Statistics are enabled by default on Nightly, Aurora and Beta. In either case, if this functionality is enabled, users can disable it in Firefox's Options/Preferences by simply deselecting the "Submit performance data" item.
Usage statistics are transmitted using SSL (a method of protecting data in transit) and help us improve future versions of Firefox. Once sent to Mozilla, usage statistics are stored in an aggregate form and made available to a broad range of developers, including both Mozilla employees and public contributors. "
Flags: needinfo?(jmenon)
Flags: needinfo?(gpiper)
Flags: needinfo?(benjamin)
Reporter | ||
Comment 2•11 years ago
|
||
> This feature is turned off by default in general release versions of
> Firefox. In order to enable Nightly, Aurora and Beta testers to provide more
> efficient feedback, Usage Statistics are enabled by default on Nightly,
> Aurora and Beta. In either case, if this functionality is enabled, users can
> disable it in Firefox's Options/Preferences by simply deselecting the
> "Submit performance data" item.
My recommendation is that instead of listing nightly/aurora/beta separately, we simply lump them together as "prerelease" users, but that's really your call.
But the current string is not "Submit performance data". There are currently two checkboxes:
"Enable Telemetry" and "Enable Nightly Health Report"
Flags: needinfo?(benjamin)
Comment 3•11 years ago
|
||
This UI isn't managed by Gecko; it'll involve a conditional change to org.mozilla.gecko.DataReportingNotification.
Component: Telemetry → General
Product: Toolkit → Firefox for Android
Comment 4•11 years ago
|
||
bsmedberg wants this to go out in 30, so there's a little bit of work prioritization that needs to happen.
tracking-fennec: --- → ?
Assignee | ||
Comment 5•11 years ago
|
||
I believe all that needs to change is that version in DataReportingNotification.java at [1] needs to be updated in the push that changes configure.in to make the beta build a non-release build.
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DataReportingNotification.java#32
Reporter | ||
Comment 6•11 years ago
|
||
Won't that cause us to reshow the notification to everyone? We should not reshow this to release users (or nightly/aurora): only beta users.
Assignee | ||
Comment 7•11 years ago
|
||
Hm, do we need to notify all pre-release users that our privacy policy has been updated? I suppose this isn't a large change, but I was under impression that's why we tracked the policy version.
If we want to just show the notification for Beta, we could add another SharedPreference that checks if Beta has been notified. I assume we would want to show this notification regardless of whether the Beta user has enabled telemetry, because it's the policy change that we want to notify about, not necessarily the enabling of telemetry?
Comment 8•11 years ago
|
||
See my note "conditional change".
If you're an Aurora or Nightly user, or a Beta or Release user with telemetry enabled -- in short, if you already have telemetry turned on -- we don't notify and just bump the version.
Otherwise, we bump the version and notify.
Updated•11 years ago
|
tracking-fennec: ? → 30+
Reporter | ||
Comment 9•11 years ago
|
||
Note: legal is currently considering re-showing the notification to all users instead of just beta users. I should know the answer on this later today or tomorrow.
Comment 10•11 years ago
|
||
Chenxia, do you have time to put this on your plate? I can review.
Reporter | ||
Comment 11•11 years ago
|
||
We are not going to re-prompt everybody. Carry on with the beta-only reprompt plan.
Assignee | ||
Comment 12•11 years ago
|
||
Ok, sounds good, I'll do that.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → liuche
Assignee | ||
Comment 13•11 years ago
|
||
I wonder if we shouldn't have a new string for the "policy changed" notification, instead of just showing the same notification again.
Attachment #8397435 -
Flags: feedback?(rnewman)
Comment 14•11 years ago
|
||
Comment on attachment 8397435 [details] [diff] [review]
Patch: Re-show data notification to beta users
Review of attachment 8397435 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/DataReportingNotification.java
@@ +100,5 @@
> }
> +
> + private static boolean shouldRenotify(SharedPreferences sharedPrefs) {
> + // Beta channel telemetry policy update for 30 (bug 981694).
> + if (TextUtils.equals(AppConstants.MOZ_UPDATE_CHANNEL, "beta") && !sharedPrefs.getBoolean(PREFS_BETA_TELEMETRY_NOTIFIED, false)) {
Here's an alternative approach, which avoids a proliferation of special-case flags.
* Bump DATA_REPORTING_VERSION to 2. After all, it's changed, and that's what the version is for.
* Have checkAndNotifyPolicy act on two numbers:
currentVersion = dataPrefs.getInt(PREFS_POLICY_VERSION, -1);
DATA_REPORTING_VERSION.
* Build your logic according to the comparisons of those numbers.
* If the currentVersion >= DATA_REPORTING_VERSION, do nothing.
* If the currentVersion is 1, and we're on the Beta channel, perform the work required by this bug:
show a change notification and bump our currentVersion. If necessary, turn on telemetry?
* If the currentVersion is 1, and we're not on the Beta channel, quietly bump our currentVersion to 2.
* If the currentVersion < 1, do first-time work: show the main notification, set prefs etc., and set our currentVersion to 2.
Note that this is identical to the common DB versioning idiom, it avoids adding another pref, and it provides us with a nice measured place to hang different behaviors for different transitions.
Attachment #8397435 -
Flags: feedback?(rnewman)
Assignee | ||
Comment 15•11 years ago
|
||
Don't automatically set the telemetry pref for beta because we can't know if the user has toggled it.
I'm assuming that the default gecko pref for telemetry in beta will be set correctly by the patch that enables telemetry.
Attachment #8397435 -
Attachment is obsolete: true
Attachment #8400305 -
Flags: review?(rnewman)
Reporter | ||
Comment 16•11 years ago
|
||
Bug 986582 has that code.
Comment 17•11 years ago
|
||
Comment on attachment 8400305 [details] [diff] [review]
Patch: Policy notification and update
Review of attachment 8400305 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/DataReportingNotification.java
@@ +48,2 @@
> }
> + } else if (currentVersion == 1) {
I'd be inclined to phrase these as
if (...) {
...
return;
}
if (...) {
...
return;
}
@@ +48,4 @@
> }
> + } else if (currentVersion == 1) {
> + // Redisplay notification only for Beta because version 2 updates Beta policy and update version.
> + if (TextUtils.equals("beta", AppConstants.MOZ_UPDATE_CHANNEL)){
Nit: space before {.
@@ +56,5 @@
> + editor.putInt(PREFS_POLICY_VERSION, DATA_REPORTING_VERSION);
> + editor.commit();
> + }
> + } else if (currentVersion >= DATA_REPORTING_VERSION) {
> + // Do nothing, we're at a current version.
Or a future version, but in that case there's nothing we can do.
@@ +63,5 @@
> }
> }
> +
> + /**
> + * Launch a notification to the user of the data policy, and record notification time and version.
s/to the user//
@@ +65,5 @@
> +
> + /**
> + * Launch a notification to the user of the data policy, and record notification time and version.
> + * @param context Context of activity
> + * @param sharedPrefs SharedPreferences instance to use for preferences
I don't think these @params add a great deal of value! :D
@@ +83,5 @@
> + String notificationSummary;
> + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) {
> + notificationSummary = context.getResources().getString(R.string.datareporting_notification_action);
> + } else {
> + // Display partial version of Big Style notification for supporting devices.
4-space indenting.
@@ +86,5 @@
> + } else {
> + // Display partial version of Big Style notification for supporting devices.
> + notificationSummary = context.getResources().getString(R.string.datareporting_notification_summary);
> + }
> + String notificationAction = context.getResources().getString(R.string.datareporting_notification_action);
Maybe worth having
final Resources res = context.getResources();
at the top of this method?
@@ +115,5 @@
> + // Record version and notification time.
> + SharedPreferences.Editor editor = sharedPrefs.edit();
> + long now = System.currentTimeMillis();
> + editor.putLong(PREFS_POLICY_NOTIFIED_TIME, now);
> + editor.putInt(PREFS_POLICY_VERSION, DATA_REPORTING_VERSION);
If we fail for some reason to display the notification, we'll never get here. But that's probably an unrecoverable error. What do you think about doing this `finally`?
Attachment #8400305 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #17)
> Comment on attachment 8400305 [details] [diff] [review]
> Patch: Policy notification and update
>
Nits addressed. Good call on my copypasta of the getResources() calls - why would anyone do that...? :P
> @@ +115,5 @@
> > + // Record version and notification time.
> > + SharedPreferences.Editor editor = sharedPrefs.edit();
> > + long now = System.currentTimeMillis();
> > + editor.putLong(PREFS_POLICY_NOTIFIED_TIME, now);
> > + editor.putInt(PREFS_POLICY_VERSION, DATA_REPORTING_VERSION);
>
> If we fail for some reason to display the notification, we'll never get
> here. But that's probably an unrecoverable error. What do you think about
> doing this `finally`?
I'd be inclined to let those values stay un-updated, because that almost certainly means that we failed to display the notification. I suppose continuing to try might be a waste of resources though, if we continue to fail for catastrophic reasons. Let me know if you feel strongly about it though.
Attachment #8400305 -
Attachment is obsolete: true
Attachment #8401016 -
Flags: review+
Comment 19•11 years ago
|
||
(In reply to Chenxia Liu [:liuche] from comment #18)
> I'd be inclined to let those values stay un-updated, because that almost
> certainly means that we failed to display the notification. I suppose
> continuing to try might be a waste of resources though, if we continue to
> fail for catastrophic reasons. Let me know if you feel strongly about it
> though.
How about a middle-ground: record success or failure (displayed/not displayed for some reason) in UI telemetry? If we see failures, particularly repeated for individual users, we can address that. And we can track that through to Beta, which should be plenty of time.
Status: NEW → ASSIGNED
Reporter | ||
Comment 20•11 years ago
|
||
Bug 986582 turned this on now, so tracking to make sure the notices happen.
tracking-firefox31:
--- → +
Assignee | ||
Comment 21•11 years ago
|
||
Only question is whether we should stick with true/false; it's *possible* we'd want to record a numerical state instead, so we're not constrained to a boolean. But whatever, we could also just update the event version we need to track some other state! \o/ versioning!
(I'll update the first patch to be a "Part 1" patch when I land everything.)
Attachment #8402135 -
Flags: review?(rnewman)
Updated•11 years ago
|
Attachment #8402135 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c029c4e35021
https://hg.mozilla.org/integration/fx-team/rev/6a1789b74601
Target Milestone: --- → Firefox 31
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c029c4e35021
https://hg.mozilla.org/mozilla-central/rev/6a1789b74601
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox31:
--- → fixed
Resolution: --- → FIXED
Comment 24•11 years ago
|
||
Can one detail the conditionals introduced here and what to expect?
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•