per Arun: In the interest of simplifying the location of FHR controls on Android, we should replace the on/off toggle in about:healthreport with a link to FHR settings.

This uses JNI to call a static void method that start the preferences activity in the current context. It appears to be fine to call startActivity directly from JNI. I justify this as follows: since startActivity returns immediately, control should be returned to Gecko and the JS execution context immediately. Any activity onPause will be handled asynchronously by the Java UI thread exactly as if it came from an Android event.

Comment on attachment 765609 [details] [diff] [review] Add ShowPreferences message to Android about:healthreport wrapper. r=rnewman Review of attachment 765609 [details] [diff] [review]: ----------------------------------------------------------------- liuche: can you verify I didn't do anything foolish with makePrefIntent? Thanks.

Comment on attachment 765609 [details] [diff] [review] Add ShowPreferences message to Android about:healthreport wrapper. r=rnewman Review of attachment 765609 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. My only additional nit would be that the filename is now too specific (maybe GeckoDataReporting?) but I leave that up to you. ::: mobile/android/base/DataReportingNotification.java @@ +57,5 @@ > + * > + * @return intent. > + */ > + public static Intent makePreferencesIntent(final Context context) { > + final Intent prefIntent = new Intent(GeckoApp.ACTION_LAUNCH_SETTINGS); This is the roundabout way to launch Data Choices, because it launches GeckoApp first. I think it's slower. It's here because we don't know if Gecko is running when a user launches the settings from the Android system notification (bug 873072), but it's definitely unnecessary if you're launching from about:healthreport. The only change to launch data choices settings directly is the Intent construction. Use this instead: Intent prefIntent = new Intent(context, GeckoPreferences.class);

As discussed Vidyo: I'd like this to be implemented through a Gecko message which opens a pref pane by name. Seems more general, and this seems like a risky time to be adding JNI calls. Lukas, we'd like to get this and Bug 882745 into 23 if we can. They should be pretty small and pretty low-risk, so I wouldn't be worried about them landing early in Beta, but we'll get them into m-c and then I'll come begging :D

This uses a Gecko message to start the preferences activity in the current Android context. I also fix a typo in JNI (revealed by an earlier approach to this patch) and some incidental formatting in GeckoApp.java. Feedback: :liuche

Comment on attachment 765970 [details] [diff] [review] Add ShowSettings message to Android about:healthreport wrapper. r=rnewman liuche: I extracted a different helper. Just sanity checking here. I verified locally: * Data notification still links to data reporting section of Settings * ShowSettings message in about:healthreport correctly displays data reporting section of Settings * Hardware back button returns from settings to about:healthreport I observe that: * Software back button returns from settings to about:healthreport, but the icon suggests it will return to the main page of settings. Problem?

Comment on attachment 765970 [details] [diff] [review] Add ShowSettings message to Android about:healthreport wrapper. r=rnewman Review of attachment 765970 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +691,4 @@ > Log.e(LOGTAG, "Received Contact:Add message with no email nor phone number"); > + } > + } else if (event.equals("Settings:Show")) { > + // null strings return "null" (http://code.google.com/p/android/issues/detail?id=13830) Oh FFS.

This version handles the relevant messages in BrowserApp rather than Gecko:App, since this is Fennec-specific functionality. I re-tested with this change and saw no differences. Carrying forward r+ per IRC conversation.

Comment on attachment 766046 [details] [diff] [review] Add ShowSettings message to Android about:healthreport wrapper. r=rnewman Review of attachment 766046 [details] [diff] [review]: ----------------------------------------------------------------- Carry-forward didn't take.

> I observe that: > > * Software back button returns from settings to about:healthreport, but the > icon suggests it will return to the main page of settings. Problem? I thought about that too, and these were my conclusions: - Technically, the title should say "Data Choices" (because that's the name of the screen), but "Settings" is better because it's more informative if you've just jumped into the screen from elsewhere. - It's okay to jump right back because the user didn't navigate through the menus, and probably just wants to change one preference. It's possibly even annoying to navigate back though menus. - the upside of having a full menu hierarchy is that people will know where to find "Data Choices" Anyways, feel free to file a bug on this. I've gone back and forth, and haven't done it because it takes a non-trivial amount of work with building the backstack and maintaining a hierarchy. > The only change to launch data choices settings directly is the Intent > construction. Use this instead: > Intent prefIntent = new Intent(context, GeckoPreferences.class); I don't think you actually addressed this - the code still seems to be launching an Intent to start GeckoApp to start GeckoPreferences. This is fine, but as I noted, a little slower. > liuche: I extracted a different helper. Just sanity checking here. Extraction looks fine to me.

Tracking, we would consider this enhancement in early 23 betas.

Comment on attachment 766046 [details] [diff] [review] Add ShowSettings message to Android about:healthreport wrapper. r=rnewman [Approval Request Comment] Bug caused by (feature/regressing bug #): New work to support FHR data settings access. User impact if declined: FHR web content will only be able to toggle data sharing, rather than taking the user to the appropriate settings pane. Testing completed (on m-c, etc.): Manually tested, now baking on s-c before merge to m-c. Risk to taking this patch (and alternatives if risky): Slim; refactor of existing code to support a pure additive chunk. String or IDL/UUID changes made by this patch: None.

Comment on attachment 766046 [details] [diff] [review] Add ShowSettings message to Android about:healthreport wrapper. r=rnewman [Triage Comment] Switching the auora nom to beta and approving as this is needed for Fx23 beta 1.