833625 - Android UI for notification of data collection/upload (Telemetry, FHR)

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect, P1)

Description

[Chenxia Liu [:liuche]]

Attachment: Android data collection UI (notifications and management)

The notification UI for data collection needs to be unified in Firefox for Android. At the moment, Telemetry displays a doorhanger specifically for itself, but this should be replaced by a single notification covering all user data collection.

UI mock by :ibarlow attached.

Comment 1

[Gregory Szorc [:gps]]

This is a beta blocker? We're uplifting FHR on Android to 20?

Comment 2

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #1)
> This is a beta blocker? We're uplifting FHR on Android to 20?

I don't know, are we? :D

I would make a case for it if it were done in time.

And I'd rather flag it and then have this marked as not blocking than not flag it.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Part 1: WIP Data reporting doorhanger notification

WIP for data reporting doorhanger notification. Removed Telemetry prompting and replaced with policy-driven notification.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Part 1: WIP Data reporting doorhanger notification

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Part 0: enabling and packaging data reporting

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Part 1: WIP Data reporting doorhanger notification

Working doorhanger-popup version now that packaging fixed.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 708255 [details] [diff] [review]
Part 0: enabling and packaging data reporting

Patch to enable FHR on Android has been moved to Bug 840129, and that must be applied before patches for this bug.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting doorhanger notification v2

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Part 2: Abstract out preferences screen v1

Abstracts out the Gecko preferences screen that future preferences screens can subclass.

(Unfortunately, simply nesting a preference screen inside preferences.xml.in won't work because the back behavior in the actionbar fails, which is a bug in Android r14. Thus the workaround of an explicit activity for the preference screen.)

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Part 3: WIP nested preferences screen for data choices v1

Creates and integrates the activity into the Gecko settings flow.

Currently trying to figure out why healthreport prefs are not accessible.

Comment 11

[Chenxia Liu [:liuche]]

This is coming along - adding access to crashreporter "pref," which is not in actuality a pref, but must be fetched from the xpcom service at runtime.

Also discovered that the first patch has bit-rotted, and the doorhanger is no longer being displayed, so fixing that as well.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Part 3: WIP crashreporter pref fetching v2

This does not seem to be the correct way to access Crash Reporter state, because code hangs on the fetch of the submitReports boolean. I've enabled crashreporting (MOZ_CRASHREPORTING) in my .mozconfig - this should be enough, so I'm stumped on what else might be failing.

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting doorhanger notification v3

Notifications appearing once again, typo derp.

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Part 3: WIP crashreporter pref fetching v3

Cleaning up a few loose ends. Crashreporter problem still present.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v1

Add another two messages between Gecko and Java: persisting some prefs to SharedPreferences, and launching an activity from Gecko.

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v2

Earlier patch was borked on apply and merge, fixed now.

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 3: WIP crashreporter pref fetching v4

Added r14 specific UI (SwitchPreference) to data choice preferences xml file.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 716888 [details]
Part 4: Messaging between Gecko and Java v2

This looks good to me. I want Brian to take a look too.

The "Activity" message reminded me that Fabrice, a while ago, had started building a system for JS to access Android intents. This approach is much simpler, but might be a bit too open since add-ons could access it too.

I'm not blocking on that though, just wondering if we could make something a bit more generic, maybe adding to the NativeWindow API. Something to think about.

I wonder if we could use our JS<->JNI code to better wrap the Intent system? Adding Wes to think about that too.

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting doorhanger notification v4

Remove more (obsolete) Telemetry notification prefs references.

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v2

Updated patch to move setting of SharedPreferences to background thread - fixes StrictMode warnings.

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 718168 [details] [diff] [review]
Part 4: Messaging between Gecko and Java v2

Review of attachment 718168 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +524,5 @@
> +                    }
> +                }
> +                // Persist value to SharedPreferences. Use a background thread.
> +                final SharedPreferences.Editor threadEditor = editor;
> +                GeckoBackgroundThread.post(new Runnable() {

handleMessage() is already called off of the UI thread, so you shouldn't need to create another Runnable here if you're doing this to avoid StrictMode warnings.

::: mobile/android/base/GeckoDataPreferences.java
@@ +31,3 @@
>          if (!TextUtils.isEmpty(prefName)) {
>            PrefsHelper.setPref(prefName, newValue);
> +          if (!(newValue instanceof Boolean)) {

Nit: Use 4-space indentation in Java

@@ +38,5 @@
> +          // Do this on a background thread.
> +          final Boolean threadValue = (Boolean) newValue;
> +          GeckoBackgroundThread.post(new Runnable() {
> +            @Override
> +            public synchronized void run() {

Why synchronized?

Comment 22

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v3

Thanks for the feedback, Brian. Took out the synchronized, it's totally unnecessary, and fixed indenting/extraneous-background nits.

Comment 23

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting doorhanger notification v5

Removed more unnecessary Telemetry prefs.

Comment 24

[Chenxia Liu [:liuche]]

Attachment: Part 2: Abstract out preferences screen v2

Added compatibility for pre/post ICS CheckBoxPreference and SwitchPreference.

Comment 25

[Chenxia Liu [:liuche]]

Attachment: Part 3: Additional preference screen for Data Choices v5

Support pre/post-ICS UI for preferences.

Comment 26

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v4

Fixed bug in crashreporter messaging.

Comment 27

[Chenxia Liu [:liuche]]

Attachment: Optional: Part 3b - Handle TwoState/CheckBox preference versioning properly

This is the "proper" way of handling different versions of the Preference, and should be applied on top of Part 3. We encapsulate the Preference handling in classes which aren't loaded unless we know the Android version can handle it.

It's uglier than the Part 3 patch, which works fine except for a dalvik error:
02-27 11:05:25.918: E/dalvikvm(26535): Could not find class 'android.preference.TwoStatePreference', referenced from method org.mozilla.gecko.GeckoPreferencesActivity$2.prefValue
02-27 11:05:25.918: W/dalvikvm(26535): VFY: unable to resolve instanceof 204 (Landroid/preference/TwoStatePreference;) in Lorg/mozilla/gecko/GeckoPreferencesActivity$2;
02-27 11:05:25.918: D/dalvikvm(26535): VFY: replacing opcode 0x20 at 0x0008
02-27 11:05:25.918: D/dalvikvm(26535): VFY: dead code 0x000a-000b in Lorg/mozilla/gecko/GeckoPreferencesActivity$2;.prefValue (Ljava/lang/String;Z)V

The clean-but-non-kosher version doesn't throw on the r10 device I tested - just the dalvik complaint.

Richard, any suggestion on this?

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 718809 [details] [diff] [review]
Part 1: Data reporting doorhanger notification v5

Review of attachment 718809 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +7481,5 @@
> +  onNotifyDataPolicy: function(request) {
> +    try {
> +      this._displayDataPolicyNotification(request);
> +    } catch (ex) {
> +      request.onUserNotifyFailed(ex);

I'd be inclined to handle the case where request is null…

@@ +7489,5 @@
> +  _clearPolicyNotification: function() {
> +    NativeWindow.doorhanger.hide();
> +  },
> +
> +  // Display policy if requesting upload and no notification has been displayed. 

Trailing whitespace.

@@ +7515,5 @@
> +        label: Strings.browser.GetStringFromName("datareporting.notification.choose"),
> +        callback: function () {
> +          // Policy is accepted.
> +          request.onUserAccept("info-bar-button-pressed");
> +          // TODO: open settings.

*ahem*

@@ +7521,5 @@
> +      }
> +    ];
> +
> +    // Does this throw? Not sure from docs.
> +    NativeWindow.doorhanger.show(message, "data-reporting-prompt", buttons, BrowserApp.selectedTab.id, { persistence: -1 });

It does not throw, as far as I can tell.

show passes through to nsIAndroidBridge::HandleGeckoMessage, which wraps JNI exceptions and returns.

@@ -7601,5 @@
> -        }
> -      }
> -    ];
> -#endif
> -    let learnMoreLabel = Strings.browser.GetStringFromName("telemetry.optin.learnMore");

It seems like you should be cleaning up these telemetry strings, no?

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +74,5 @@
>  
> +# Data Reporting
> +# %1$S = product name (Firefox); %2$S = server owner (Mozilla)
> +datareporting.notification.message=%1$S automatically sends some data to %2$S so that we can improve your experience
> +datareporting.notification.choose=Choose what I share

Make the content of these match desktop exactly (chrome/browser/browser.properties), and refer to that file in the localization hint:

dataReportingNotification.message       = %1$S automatically sends some data to %2$S so that we can improve your 
experience.
dataReportingNotification.button.label  = Choose What I Share

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 718810 [details] [diff] [review]
Part 2: Abstract out preferences screen v2

Review of attachment 718810 [details] [diff] [review]:
-----------------------------------------------------------------

I trust that your code relocation doesn't also include landmines :)

::: mobile/android/base/GeckoPreferences.java
@@ +39,5 @@
>      public static String PREFS_UPDATER_AUTODOWNLOAD  = "app.update.autodownload";
>  
>      @Override
> +    protected int getPreferencesResource() {
> +      return R.xml.preferences;

Be consistent with your spacing. This should be 4.

Comment 30

[Richard Newman [:rnewman]]

Comment on attachment 718811 [details] [diff] [review]
Part 3: Additional preference screen for Data Choices v5

Review of attachment 718811 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoPreferencesActivity.java
@@ +213,3 @@
>              @Override public void prefValue(String prefName, final boolean value) {
>                  final Preference pref = getField(prefName);
> +                if (pref instanceof CheckBoxPreference || pref instanceof TwoStatePreference) {

Does this work on < 14?!

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +123,5 @@
> +<!ENTITY datachoices_telemetry_summary "Shares performance, usage, hardware and customization data about your browser with Mozilla to help us make &brandShortName; better">
> +<!ENTITY datachoices_fhr_title "Firefox Health Report">
> +<!ENTITY datachoices_fhr_summary "Helps you understand your browser performance and shares data with Mozilla about your browser health">
> +<!ENTITY datachoices_crashreporter_title "Firefox Crash Reporter">
> +<!ENTITY datachoices_crashreporter_summary "&brandShortName; submits crash reports to help Mozilla make your browser more stable and secure">

Make sure that these align with the strings we use on desktop, and again, point there for localization notes.

Comment 31

[Chenxia Liu [:liuche]]

> ::: mobile/android/base/GeckoPreferencesActivity.java
> @@ +213,3 @@
> >              @Override public void prefValue(String prefName, final boolean value) {
> >                  final Preference pref = getField(prefName);
> > +                if (pref instanceof CheckBoxPreference || pref instanceof TwoStatePreference) {
> 
> Does this work on < 14?!
> 

Quite, except for some dalvik complaints. See comment 27.

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 718812 [details] [diff] [review]
Part 4: Messaging between Gecko and Java v4

Review of attachment 718812 [details] [diff] [review]:
-----------------------------------------------------------------

A few niggles with this one.

::: mobile/android/base/BrowserApp.java
@@ +502,5 @@
> +                    editor = getSharedPreferences(message.getString("branch"), Activity.MODE_PRIVATE).edit();
> +                } else {
> +                    editor = getPreferences(Activity.MODE_PRIVATE).edit();
> +                }
> +                JSONArray jsonPrefs = message.getJSONArray("preferences");

I'd like to see a try block around all of the handling of jsonPrefs.

::: mobile/android/base/GeckoDataPreferences.java
@@ +31,2 @@
>          if (!TextUtils.isEmpty(prefName)) {
> +            PrefsHelper.setPref(prefName, newValue);

This should really be setGeckoPref. Oh well.

@@ +31,4 @@
>          if (!TextUtils.isEmpty(prefName)) {
> +            PrefsHelper.setPref(prefName, newValue);
> +            if (!(newValue instanceof Boolean)) {
> +                Log.e(LOGTAG, "Pref persisted is not a boolean.");

Why do you care?

::: mobile/android/chrome/content/browser.js
@@ +7540,5 @@
> +          sendMessageToJava({
> +            type: "Activity:Launch",
> +            classname: "org.mozilla.gecko.GeckoDataPreferences",
> +          });
> +          // Mirror prefs to SharedPreferences in Java for upload service.

Don't you want to mirror these prefs *before* launching the activity?

@@ +7552,5 @@
> +          let healthreportEnabled = Services.prefs.getBoolPref(this.PREF_HEALTHREPORT_ENABLED);
> +          prefs.push({
> +            name: this.PREF_HEALTHREPORT_ENABLED,
> +            type: "bool",
> +            value: healthreportEnabled

Why is this a proper boolean, and the previous example uses the string "true"?

Comment 33

[Richard Newman [:rnewman]]

Comment on attachment 719133 [details] [diff] [review]
Optional: Part 3b - Handle TwoState/CheckBox preference versioning properly

Review of attachment 719133 [details] [diff] [review]:
-----------------------------------------------------------------

Strictly speaking you don't need three classes. Checkbox can be your base class.

But this is a horrible way to have to solve the problem :/

::: mobile/android/base/GeckoPreferencesActivity.java
@@ +203,5 @@
> +
> +            class CheckBoxPrefSetter extends BackwardsCompatibleBooleanPrefSetter {
> +                @Override
> +                public void setBooleanPref(Preference preference, boolean value) {
> +                    

Whitespace!

@@ +220,5 @@
>              }
>  
>              @Override public void prefValue(String prefName, final boolean value) {
>                  final Preference pref = getField(prefName);
> +                BackwardsCompatibleBooleanPrefSetter prefSetter;

Just make this final and get rid of `finalPrefSetter`. Java is smart enough to know that it'll only be set once.

@@ +229,4 @@
>                  }
> +                final BackwardsCompatibleBooleanPrefSetter finalPrefSetter = prefSetter;
> +                GeckoAppShell.getMainHandler().post(new Runnable() {
> +                    public void run() {

@Override.

Comment 34

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting doorhanger notification v6

Fixed nits, removed telemetry strings, and added localization notes to reference desktop strings.

Comment 35

[Chenxia Liu [:liuche]]

Attachment: Part 2: Abstract out preferences screen v3

Fixed spacing.

Comment 36

[Chenxia Liu [:liuche]]

Attachment: Part 3: Additional preference screen for Data Choices v6

Merged in Part 3b to handle older Android versions correctly, and cleaned up subclass/superclass messiness.

Comment 37

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v4

> I'd like to see a try block around all of the handling of jsonPrefs.

There is actually a try block around all the message handling. I added the try around the type handling for more specificity, but wrapping the jsonPrefs handling in a try does pretty much what the outer try is covering. So, removed it.

> @@ +31,4 @@
> >          if (!TextUtils.isEmpty(prefName)) {
> > +            PrefsHelper.setPref(prefName, newValue);
> > +            if (!(newValue instanceof Boolean)) {
> > +                Log.e(LOGTAG, "Pref persisted is not a boolean.");
> 
> Why do you care?

Setting prefs in SharedPreferences has to be done by type (Boolean, Int, Long, Float, String, etc) so this was kind of an ugly hack to get by handling all the types since the Data Choices xml only contains boolean prefs. Changed it so the check is implicit, and commented.

> Don't you want to mirror these prefs *before* launching the activity?

I don't think it actually matters because the DataPreferences activity fetches prefs from Gecko (because that is what the GeckoPreferencesActivity does). It *might* be a problem if the user is able to set a pref on the preferences screen (manually) faster than the Gecko -> Android message. So, no harm in swapping the two calls, but there shouldn't be a dependency.

> Why is this a proper boolean, and the previous example uses the string
> "true"?

Good catch, fixed now.

Comment 38

[Chenxia Liu [:liuche]]

Sorry this is taking so long, writing tests for this now, and figuring out (yet again) why some Gecko prefs are not accessible.

Comment 39

[Chenxia Liu [:liuche]]

Attachment: Part 3: Additional preference screen for Data Choices v7

Indenting typo.

Comment 40

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v4

Access Gecko prefs from DataNotification callback properly.

Comment 41

[Richard Newman [:rnewman]]

Comment on attachment 720583 [details] [diff] [review]
Part 4: Messaging between Gecko and Java v4

Review of attachment 720583 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoDataPreferences.java
@@ +34,5 @@
> +
> +            // Currently, Data Choices only contains Booleans.
> +            final Object threadValue = newValue;
> +            if (threadValue instanceof Boolean) {
> +                // Set pref value in SharedPreferences for Java background services.

So call me crazy, but isn't an OnPreferenceChangeListener called to allow you to do things like *avoid* setting into SharedPreferences (by returning false), and otherwise the Preference's `key` attribute is used to persist the value into SharedPreferences?

"This class contains a key that will be used as the key into the SharedPreferences. It is up to the subclass to decide how to store the value."

http://developer.android.com/reference/android/preference/Preference.html

"This preference will store a boolean into the SharedPreferences."

http://developer.android.com/reference/android/preference/SwitchPreference.html

That is, why do you need to change this method? Just ensure (as you have) that the pref XML has

+    <SwitchPreference android:key="datareporting.healthreport.uploadEnabled"

and write into Gecko as this method always has. What am I missing?

Comment 42

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v5

No, you are totally correct about that - uploaded a fixed patch.

Comment 43

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting doorhanger notification v7

Comment 44

[Chenxia Liu [:liuche]]

Attachment: Part 2: Abstract out preferences screen v4

Comment 45

[Chenxia Liu [:liuche]]

Attachment: Part 3: Additional preference screen for Data Choices v8

Comment 46

[Chenxia Liu [:liuche]]

Attachment: Part 4: Messaging between Gecko and Java v5

Comment 47

[Chenxia Liu [:liuche]]

Unbit-rotted to apply on current services-central.

Comment 48

[Chenxia Liu [:liuche]]

Patches applied to github clone of services repo. Commits at updated url.

Unbitrotted conflicts with bug 802130.

Comment 49

[Chenxia Liu [:liuche]]

Additional Telemetry behavior QA:

One of the last things we want to do with this new unification of data reporting notifications is to break Telemetry reporting on Android. Telemetry uses a pref to determine whether to upload, and the default state of this pref differs between testing channels (Nightly, Aurora) and release-ish(?) channels (Beta, GA), so QA should verify that this pref is being set as expected.

The following cases should be verified with the new notifications/preferences code:
- Nightly/Aurora (These are builds with the flag MOZ_TELEMETRY_ENABLED_BY_DEFAULT): Telemetry is opt-out.
	a. On first startup, the pref toolkit.telemetry.enabledPreRelease should be true. This pref should be controllable via Settings > Data Choices > Telemetry.
	b. After the data reporting policy notification pops up*, tapping "Choose what I share" should redirect the user to the Data Choice preference screen, where the state of the Telemetry pref should be displayed as the state the user has explicitly set in Settings, or if no explicit choice has been made yet, default to checked/on.

- Beta/Release: Telemetry is opt-in.
	a. On first startup, the pref toolkit.telemetry.enabled should be false. This pref should be controllable via Settings > Data Choices > Telemetry.
	b. After the data reporting policy notification pops up*, tapping "Choose what I share" should redirect the user to the Data Choice preference screen, where the state of the Telemetry pref should be displayed as the state the user has explicitly set in Settings, or if no explicit choice has been made yet, default to unchecked/off.

* To launch notification early, modify the datareporting.policy.firstRunTime value by subtracting 3 from the third leftmost digit (e.g., 1365612974269 becomes 1335612974269).

Comment 50

[Chenxia Liu [:liuche]]

Just a quick verification from you, Ian: the patches for the Data Choices preference screen are in the process of being reviewed, and I just wanted to double check with you that the strings in the mock are intended to omit the periods. (This is string style for Android, I assume?)

Comment 51

[Chenxia Liu [:liuche]]

SCOPE AND SPEC CHANGE:

Instead of using a doorhanger to notify policy (due to limitations in its dismiss behavior), we will be using an Android system notification instead (ok-ed by ibarlow).

Preferences screen for different datareporting selections will proceed as planned.

Comment 52

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #50)
> Just a quick verification from you, Ian: the patches for the Data Choices
> preference screen are in the process of being reviewed, and I just wanted to
> double check with you that the strings in the mock are intended to omit the
> periods. (This is string style for Android, I assume?)

Yep! :)

Comment 53

[Chenxia Liu [:liuche]]

First pass hooking in Android system notifications. Functional, but needs strings, icons, etc, and some strict mode debugging.

Comment 54

[Chenxia Liu [:liuche]]

Attachment: Screenshot: tablet UI settings nesting

More spec changes! Tablet-mode for settings has landed in the tree, so the UI needs to be adjusted accordingly.

Ian, in the new tablet view, would you want the Data choices to be nested under the "Privacy & Security" header (see screenshot), or should it be it's own separate header? Or something else entirely?

(The check-boxes in the screenshot are a quick mock-up - I can still do the two state sliders if you want them.)

Comment 55

[Ian Barlow (:ibarlow)]

This looks good to me. We can leave the checkmarks for now, instead of the state sliders. This way is more consistent with the rest of our settings.

Comment 56

[Richard Newman [:rnewman]]

SHIP IT SHIP IT

Comment 57

[Chenxia Liu [:liuche]]

Comment on attachment 732521 [details] [diff] [review]
Part 1: Data reporting doorhanger notification v7

Obsoleting Gecko-based fhr patches.

Comment 58

[Chenxia Liu [:liuche]]

Attachment: Part 0: Fix strict mode in titlebar pref

Comment 59

[Chenxia Liu [:liuche]]

Attachment: Part 1: Preferences screen for datareporting v1

Comment 60

[Chenxia Liu [:liuche]]

Attachment: Part 2: Android notifications for datareporting

Comment 61

[Chenxia Liu [:liuche]]

Github pull request for other reviewing alternatives (based on the git inbound): https://github.com/mozilla/mozilla-central/pull/14

Comment 62

[Richard Newman [:rnewman]]

Comment on attachment 747901 [details] [diff] [review]
Part 1: Preferences screen for datareporting v1

Review of attachment 747901 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget patch headers:

  Bug 833625 - Part 1: update GeckoPreferences, implement data reporting preferences. r=rnewman

I think this is fine, but I'm going to build and test...

::: mobile/android/base/AppConstants.java.in
@@ +56,5 @@
> +#ifdef MOZ_SERVICES_HEALTHREPORT
> +    true;
> +#else
> +    false;
> +#endif

Remember to update android-sync's copy of AppConstants, and preprocessor.ini, to match.

::: mobile/android/base/GeckoPreferences.java
@@ +69,5 @@
>  
>      @Override
>      protected void onCreate(Bundle savedInstanceState) {
> +        // For fragment-capable devices, display the default fragment if no explicit fragment to show.
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB &&

Let's add DeviceUtils or AppConstants definition for fragment capability, rather than using a magic version number.

@@ +78,3 @@
>          super.onCreate(savedInstanceState);
>  
> +        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB) {

Same.

@@ +79,5 @@
>  
> +        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB) {
> +            Bundle intentExtras = getIntent().getExtras();
> +            if (intentExtras != null && intentExtras.containsKey(INTENT_EXTRA_RESOURCES)) {
> +                String resourceName = intentExtras.getString(INTENT_EXTRA_RESOURCES);

You repeat this stanza twice, at least. Let's lift it out.

Comment 63

[Richard Newman [:rnewman]]

Comment on attachment 747902 [details] [diff] [review]
Part 2: Android notifications for datareporting

Review of attachment 747902 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Makefile.in
@@ +77,4 @@
>    db/BrowserDB.java \
>    db/LocalBrowserDB.java \
>    db/DBUtils.java \
> +  DataReportingNotification.java \

Did you forget to `hg add` this file?

Comment 64

[Richard Newman [:rnewman]]

Try build for this plus other FHR stuff:

https://tbpl.mozilla.org/?tree=Try&rev=613beacb0448

Comment 65

[Chenxia Liu [:liuche]]

Attachment: Part 1.5: Adding missing res files.

Adding missing res files for part 1.

Comment 66

[Chenxia Liu [:liuche]]

Attachment: Part 2.5: Adding missing java file + icons

Adding missing DataReportingNotification.java and big notification icons.

Comment 67

[Chenxia Liu [:liuche]]

Attachment: Part 2.5: Adding missing java file + icons

ACTUALLY add the icons.

Comment 68

[Richard Newman [:rnewman]]

Attachment: Part 0. v2

Comment 69

[Richard Newman [:rnewman]]

Attachment: Part 1. v2

Comment 70

[Richard Newman [:rnewman]]

Attachment: Part 2. v2

Comment 71

[Chenxia Liu [:liuche]]

Attachment: Part 2. v3

Updated with shorter strings from bug 862116. Does not include dynamic string selection based on notification space available.

Comment 72

[Chenxia Liu [:liuche]]

Attachment: Part 1. v3

Tiny fix in confvars.sh that didn't apply cleanly on m-i.

Comment 73

[Richard Newman [:rnewman]]

Error reading pref [datareporting.crashreporter.submitEnabled]: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult:
                    "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/browser.js :: getPreferences :: line 1021"  data: no]

Comment 74

[Richard Newman [:rnewman]]

The UI doesn't degrade to not show the crashreporter checkbox if built without crashreporter.

Comment 75

[Richard Newman [:rnewman]]

Attachment: Part 1b: hide crash reporter. v1

Comment 76

[Richard Newman [:rnewman]]

Attachment: Part 1b: hide crash reporter. v2

MOZ_CRASHREPORTER is either "" or "1", so using #if instead of #ifdef.

Comment 77

[Chenxia Liu [:liuche]]

Attachment: Part 1c: hide data choices v1

Remove data choices menu item if MOZ_DATAREPORTING is false, or if none of the data collection is present (telemetry, crashreporter, fhr).

(Currently testing a clobber build for displaying this.)

Comment 78

[Chenxia Liu [:liuche]]

Attachment: Part 2b: Don't notify if not data collection

Don't send notification if data collection isn't being built.

Comment 79

[Chenxia Liu [:liuche]]

Attachment: Part 1a. v1: add switch preference in datareporting UI

Comment 80

[Chenxia Liu [:liuche]]

Attachment: Part 1c: hide data choices v2

Fixed wrong build flag.

Comment 81

[Chenxia Liu [:liuche]]

Attachment: Part 2b: Don't notify if not collecting data v2

Fixed wrong build flag.

Comment 82

[Chenxia Liu [:liuche]]

Attachment: Part 2c: Set healthreport shared pref v1

Set the default value of healthreport upload enabled pref upon notification, for about:hr to use.

Comment 83

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting UI v1

Folded patch for data reporting UI.

Comment 84

[Chenxia Liu [:liuche]]

Attachment: Part 2: Android notification for datareporting v1

Folded patch for Android notification of datareporting, including setting FHR SharedPreference for about:hr

Comment 85

[Chenxia Liu [:liuche]]

Attachment: Part 1: Data reporting UI v2

whitespace changes

Comment 86

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/bfe5a09bd4db
https://hg.mozilla.org/services/services-central/rev/42adc74b087d
https://hg.mozilla.org/services/services-central/rev/5b345b6dc928

Comment 87

[Chenxia Liu [:liuche]]

Telemetry QA [UPDATED]:

One of the last things we want to do with this new unification of data reporting notifications is to break Telemetry reporting on Android. Telemetry uses a pref to determine whether to upload, and the default state of this pref differs between testing channels (Nightly, Aurora) and release channels (Beta, GA), so QA should verify that this pref is being set as expected.

The behavior to verify is as follows:
1. The default for the telemetry pref should be true for nightly and aurora, and false for beta and release.

2. Toggling the telemetry pref in Settings > Data Choices > Telemetry should mirror correctly to about:telemetry and the about:config telemetry pref (toolkit.telemetry.enabled in release builds and toolkit.telemetry in nightly and aurora builds), and vice versa.

Comment 88

[Richard Newman [:rnewman]]

Taras, this is the Android side of the data reporting UI change that hit desktop a while back. CCing you (please 302 if necessary!) so you can keep an eye open for any regressions in reporting as this progresses into m-c and along the trains.

Comment 89

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/bfe5a09bd4db
https://hg.mozilla.org/mozilla-central/rev/42adc74b087d
https://hg.mozilla.org/mozilla-central/rev/5b345b6dc928

Comment 90

[Aaron Train [:aaronmt]]

Please add MozTrap test-cases (there's no appropriate flag in this component).

Comment 91

[Teodora Vermesan (:TeoVermesan)]

Build: Firefox for Android 23.0a1(2013-05-14) 
Device: Asus Transformer TF 101
OS: Android 4.0.3

Based on comment #49 and comment #87 I've verified the followings:
- The default for the telemetry preference in Settings > Data Choices > Telemetry is ON
- the preference is controllable via Settings > Data Choices > Telemetry
- In about:config, toolkit.telemetry.enabledPreRelease = true
- Changing the telemetry preference in Settings > Data Choices > Telemetry changes correctly in about:telemetry and in about:config.
- After the data reporting policy notification pops up, tapping "Choose what I share" redirects to the Data Choice preference screen

Comment 92

[Richard Newman [:rnewman]]

(In reply to Aaron Train [:aaronmt] from comment #90)
> Please add MozTrap test-cases (there's no appropriate flag in this
> component).

Can we use in-testsuite? for this? If not, I'll get a BZ admin to fix this.

Comment 93

[Teodora Vermesan (:TeoVermesan)]

Test Cases added in moztrap for Telemetry:
https://moztrap.mozilla.org/manage/case/8128/
https://moztrap.mozilla.org/manage/case/8129/
https://moztrap.mozilla.org/manage/case/8130/
https://moztrap.mozilla.org/manage/case/8131/
https://moztrap.mozilla.org/manage/case/8132/

Comment 94

[Lukas Blakk [:lsblakk] use ?needinfo]

Adding Delphine to this bug to ensure the new strings in this feature are verified localized in time for shipping.

Comment 95

[Delphine Lebédel [:delphine - Use needinfo]]

Sorry I QA localization on B2G, won't be able to help in this case!