879555 - Replace FHR upload enabled toggle with link to FHR settings

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Add ShowPreferences message to Android about:healthreport wrapper. r=rnewman

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 2

[Nick Alexander :nalexander [he/him]]

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 3

[Chenxia Liu [:liuche]]

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

Comment 4

[Richard Newman [:rnewman]]

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

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Add ShowSettings message to Android about:healthreport wrapper. r=rnewman

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 6

[Nick Alexander :nalexander [he/him]]

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 7

[Richard Newman [:rnewman]]

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.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Add ShowSettings message to Android about:healthreport wrapper. r=rnewman

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 9

[Nick Alexander :nalexander [he/him]]

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.

Comment 10

[Chenxia Liu [:liuche]]

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

Comment 11

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/services/services-central/rev/d5da469d2c49

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 13

[Richard Newman [:rnewman]]

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 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d5da469d2c49

Comment 15

[bhavana bajaj [:bajaj]]

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.

Comment 16

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/releases/mozilla-beta/rev/89d6bdd9e08f