Replace FHR upload enabled toggle with link to FHR settings

RESOLVED FIXED in Firefox 23

Status

defect
RESOLVED FIXED
6 years ago
10 months ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

unspecified
Firefox 24
All
Android

Firefox Tracking Flags

(firefox23+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Blocks: 868442
No longer blocks: 868442
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.
Attachment #765609 - Flags: review?(rnewman)
Attachment #765609 - Flags: feedback?(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);
Attachment #765609 - Flags: feedback?(liuche) → feedback+
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
Status: NEW → ASSIGNED
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
Attachment #765970 - Flags: review?(rnewman)
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?
Attachment #765970 - Flags: feedback?(liuche)
Attachment #765609 - Attachment is obsolete: true
Attachment #765609 - Flags: review?(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.
Attachment #765970 - Flags: review?(rnewman) → review+
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.
Attachment #765970 - Attachment is obsolete: true
Attachment #765970 - Flags: feedback?(liuche)
Attachment #766046 - Flags: review?
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.
Attachment #766046 - Flags: review? → review+
> 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.
https://hg.mozilla.org/services/services-central/rev/d5da469d2c49
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 24
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.
Attachment #766046 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d5da469d2c49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
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.
Attachment #766046 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.