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.

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.

Working doorhanger-popup version now that packaging fixed.

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.

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.

Notifications appearing once again, typo derp.

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

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

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

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

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.

Remove more (obsolete) Telemetry notification prefs references.

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

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?

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

Removed more unnecessary Telemetry prefs.

Added compatibility for pre/post ICS CheckBoxPreference and SwitchPreference.

Support pre/post-ICS UI for preferences.

Fixed bug in crashreporter messaging.

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 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 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 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.

> ::: 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"?

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.

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

Fixed spacing.

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

> 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.

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.

Access Gecko prefs from DataNotification callback properly.

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.

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.

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?)

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.

(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.)

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.

SHIP IT SHIP IT

Comment on attachment 732521 [details] [diff] [review] Part 1: Data reporting doorhanger notification v7 Obsoleting Gecko-based fhr patches.

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

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.

Adding missing DataReportingNotification.java and big notification icons.

ACTUALLY add the icons.

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

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

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.

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

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.

Fixed wrong build flag.

Fixed wrong build flag.

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

Folded patch for data reporting UI.

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

whitespace changes

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

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

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

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

(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.

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/

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

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