Closed Bug 833625 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

(firefox23 fixed)

RESOLVED FIXED
Firefox 23
Tracking Status
firefox23 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

(Blocks 2 open bugs, )

Details

Attachments

(4 files, 52 obsolete files)

73.11 KB, image/png
Details
1.56 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
17.62 KB, patch
Details | Diff | Splinter Review
62.38 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → liuche
OS: Mac OS X → Android
Hardware: x86 → ARM
Blocks: 829887
Status: NEW → ASSIGNED
This is a beta blocker? We're uplifting FHR on Android to 20?
(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.
WIP for data reporting doorhanger notification. Removed Telemetry prompting and replaced with policy-driven notification.
Attachment #706235 - Attachment is obsolete: true
Working doorhanger-popup version now that packaging fixed.
Attachment #706246 - Attachment is obsolete: true
Depends on: 838877
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.
Attachment #708255 - Attachment is obsolete: true
Attachment #708283 - Attachment is obsolete: true
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.)
Creates and integrates the activity into the Gecko settings flow.

Currently trying to figure out why healthreport prefs are not accessible.
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.
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.
Attachment #714176 - Attachment is obsolete: true
Attachment #716087 - Flags: feedback?(rnewman)
Attachment #716087 - Attachment description: Part 3: WIP crashreporter pref fetching → Part 3: WIP crashreporter pref fetching v2
Notifications appearing once again, typo derp.
Attachment #714170 - Attachment is obsolete: true
Cleaning up a few loose ends. Crashreporter problem still present.
Attachment #716087 - Attachment is obsolete: true
Attachment #716087 - Flags: feedback?(rnewman)
Add another two messages between Gecko and Java: persisting some prefs to SharedPreferences, and launching an activity from Gecko.
Attachment #716882 - Flags: feedback?(mark.finkle)
Earlier patch was borked on apply and merge, fixed now.
Attachment #716882 - Attachment is obsolete: true
Attachment #716882 - Flags: feedback?(mark.finkle)
Attachment #716888 - Flags: feedback?(mark.finkle)
Added r14 specific UI (SwitchPreference) to data choice preferences xml file.
Attachment #716881 - Attachment is obsolete: true
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.
Attachment #716888 - Flags: feedback?(mark.finkle)
Attachment #716888 - Flags: feedback?(bnicholson)
Attachment #716888 - Flags: feedback+
Remove more (obsolete) Telemetry notification prefs references.
Attachment #716115 - Attachment is obsolete: true
Updated patch to move setting of SharedPreferences to background thread - fixes StrictMode warnings.
Attachment #716888 - Attachment is obsolete: true
Attachment #716888 - Flags: feedback?(bnicholson)
Attachment #718168 - Flags: feedback?(mark.finkle)
Attachment #718168 - Flags: feedback?(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?
Attachment #718168 - Flags: feedback?(bnicholson) → feedback+
Thanks for the feedback, Brian. Took out the synchronized, it's totally unnecessary, and fixed indenting/extraneous-background nits.
Attachment #718168 - Attachment is obsolete: true
Attachment #718168 - Flags: feedback?(mark.finkle)
Removed more unnecessary Telemetry prefs.
Attachment #718167 - Attachment is obsolete: true
Added compatibility for pre/post ICS CheckBoxPreference and SwitchPreference.
Attachment #714175 - Attachment is obsolete: true
Support pre/post-ICS UI for preferences.
Attachment #716896 - Attachment is obsolete: true
Fixed bug in crashreporter messaging.
Attachment #718812 - Attachment description: Part 4: Messaging between Gecko and Java v3 → Part 4: Messaging between Gecko and Java v4
Attachment #718197 - Attachment is obsolete: true
Attachment #718809 - Flags: feedback?(rnewman)
Attachment #718809 - Flags: feedback?(gps)
Attachment #718810 - Flags: feedback?(rnewman)
Attachment #718810 - Flags: feedback?(gps)
Attachment #718811 - Flags: feedback?(rnewman)
Attachment #718811 - Flags: feedback?(gps)
Attachment #718812 - Flags: feedback?(rnewman)
Attachment #718812 - Flags: feedback?(gps)
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?
Attachment #719133 - Flags: feedback?(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
Attachment #718809 - Flags: feedback?(rnewman) → feedback+
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.
Attachment #718810 - Flags: feedback?(rnewman) → feedback+
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.
Attachment #718811 - Flags: feedback?(rnewman) → feedback+
> ::: 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 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"?
Attachment #718812 - Flags: feedback?(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.
Attachment #719133 - Flags: feedback?(rnewman) → feedback+
Fixed nits, removed telemetry strings, and added localization notes to reference desktop strings.
Attachment #718809 - Attachment is obsolete: true
Attachment #718809 - Flags: feedback?(gps)
Fixed spacing.
Attachment #718810 - Attachment is obsolete: true
Attachment #718810 - Flags: feedback?(gps)
Merged in Part 3b to handle older Android versions correctly, and cleaned up subclass/superclass messiness.
Attachment #718811 - Attachment is obsolete: true
Attachment #719133 - Attachment is obsolete: true
Attachment #718811 - Flags: feedback?(gps)
> 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.
Attachment #718812 - Attachment is obsolete: true
Attachment #718812 - Flags: feedback?(gps)
Attachment #720047 - Flags: feedback?(rnewman)
Sorry this is taking so long, writing tests for this now, and figuring out (yet again) why some Gecko prefs are not accessible.
Indenting typo.
Attachment #720036 - Attachment is obsolete: true
Access Gecko prefs from DataNotification callback properly.
Attachment #720047 - Attachment is obsolete: true
Attachment #720047 - Flags: feedback?(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?
No, you are totally correct about that - uploaded a fixed patch.
Attachment #720583 - Attachment is obsolete: true
Priority: -- → P3
Attachment #720033 - Attachment is obsolete: true
Attachment #720034 - Attachment is obsolete: true
Attachment #720582 - Attachment is obsolete: true
Attachment #720768 - Attachment is obsolete: true
Unbit-rotted to apply on current services-central.
Patches applied to github clone of services repo. Commits at updated url.

Unbitrotted conflicts with bug 802130.
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Component: Client: Desktop → Client: Android
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).
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?)
Flags: needinfo?(ibarlow)
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.
Attachment #705165 - Attachment is obsolete: true
Depends on: 862116
Flags: needinfo?(ibarlow)
No longer depends on: 862116
Depends on: 862176
(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! :)
First pass hooking in Android system notifications. Functional, but needs strings, icons, etc, and some strict mode debugging.
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.)
Flags: needinfo?(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.
Flags: needinfo?(ibarlow)
SHIP IT SHIP IT
No longer blocks: 828654
Blocks: 868442
Priority: P3 → P1
No longer depends on: 862176
Comment on attachment 732521 [details] [diff] [review]
Part 1: Data reporting doorhanger notification v7

Obsoleting Gecko-based fhr patches.
Attachment #732521 - Attachment is obsolete: true
Attachment #732522 - Attachment is obsolete: true
Attachment #732523 - Attachment is obsolete: true
Attachment #732524 - Attachment is obsolete: true
Attachment #747900 - Flags: review?(rnewman)
Attachment #747901 - Flags: review?(rnewman)
Attachment #747902 - Flags: review?(rnewman)
Github pull request for other reviewing alternatives (based on the git inbound): https://github.com/mozilla/mozilla-central/pull/14
Attachment #747900 - Flags: review?(rnewman) → review+
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 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?
Try build for this plus other FHR stuff:

https://tbpl.mozilla.org/?tree=Try&rev=613beacb0448
Adding missing res files for part 1.
Attachment #748052 - Flags: review?(rnewman)
Adding missing DataReportingNotification.java and big notification icons.
Attachment #748053 - Flags: review?(rnewman)
ACTUALLY add the icons.
Attachment #748053 - Attachment is obsolete: true
Attachment #748053 - Flags: review?(rnewman)
Attachment #748054 - Flags: review?(rnewman)
Attached patch Part 0. v2Splinter Review
Attachment #747900 - Attachment is obsolete: true
Attachment #747901 - Attachment is obsolete: true
Attachment #747902 - Attachment is obsolete: true
Attachment #748052 - Attachment is obsolete: true
Attachment #748054 - Attachment is obsolete: true
Attachment #747901 - Flags: review?(rnewman)
Attachment #747902 - Flags: review?(rnewman)
Attachment #748052 - Flags: review?(rnewman)
Attachment #748054 - Flags: review?(rnewman)
Attachment #748059 - Flags: review+
Attached patch Part 1. v2 (obsolete) — Splinter Review
Attached patch Part 2. v2 (obsolete) — Splinter Review
Attached patch Part 2. v3 (obsolete) — Splinter Review
Updated with shorter strings from bug 862116. Does not include dynamic string selection based on notification space available.
Attachment #748061 - Attachment is obsolete: true
Attached patch Part 1. v3 (obsolete) — Splinter Review
Tiny fix in confvars.sh that didn't apply cleanly on m-i.
Attachment #748060 - Attachment is obsolete: true
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]
The UI doesn't degrade to not show the crashreporter checkbox if built without crashreporter.
Attached patch Part 1b: hide crash reporter. v1 (obsolete) — Splinter Review
Attachment #748142 - Flags: review?(liuche)
Attached patch Part 1b: hide crash reporter. v2 (obsolete) — Splinter Review
MOZ_CRASHREPORTER is either "" or "1", so using #if instead of #ifdef.
Attachment #748142 - Attachment is obsolete: true
Attachment #748142 - Flags: review?(liuche)
Attachment #748155 - Flags: review?(liuche)
Attached patch Part 1c: hide data choices v1 (obsolete) — Splinter Review
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.)
Don't send notification if data collection isn't being built.
Attached patch Part 1c: hide data choices v2 (obsolete) — Splinter Review
Fixed wrong build flag.
Attachment #748245 - Attachment is obsolete: true
Fixed wrong build flag.
Attachment #748247 - Attachment is obsolete: true
Set the default value of healthreport upload enabled pref upon notification, for about:hr to use.
Attached patch Part 1: Data reporting UI v1 (obsolete) — Splinter Review
Folded patch for data reporting UI.
Attachment #748077 - Attachment is obsolete: true
Attachment #748155 - Attachment is obsolete: true
Attachment #748304 - Attachment is obsolete: true
Attachment #748306 - Attachment is obsolete: true
Attachment #748155 - Flags: review?(liuche)
Folded patch for Android notification of datareporting, including setting FHR SharedPreference for about:hr
Attachment #748072 - Attachment is obsolete: true
Attachment #748307 - Attachment is obsolete: true
Attachment #748308 - Attachment is obsolete: true
whitespace changes
Attachment #748351 - Attachment is obsolete: true
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.
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.
https://hg.mozilla.org/mozilla-central/rev/bfe5a09bd4db
https://hg.mozilla.org/mozilla-central/rev/42adc74b087d
https://hg.mozilla.org/mozilla-central/rev/5b345b6dc928
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 23
Please add MozTrap test-cases (there's no appropriate flag in this component).
Flags: needinfo?(fennec)
Blocks: 871674
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
Depends on: 871978
(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.
Blocks: 872754
Adding Delphine to this bug to ensure the new strings in this feature are verified localized in time for shipping.
QA Contact: lebedel.delphine
Sorry I QA localization on B2G, won't be able to help in this case!
QA Contact: lebedel.delphine
Duplicate of this bug: 866352
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.