1474961 - Change StumblerService to a foreground service when targeting Android O

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Comment 1

[Vlad Baicu]

Attachment: Bug 1474961 - Change StumblerService to a foreground service when targeting Android Oreo.

Also made broadcasts involving Stumbler explicit.

Review commit: https://reviewboard.mozilla.org/r/256250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256250/

Comment 2

[Vlad Baicu]

The patch depends on https://bugzilla.mozilla.org/show_bug.cgi?id=1450447 to land first in order to work properly.

I think there are still some modifications needed for the service's foreground notification, such as text, icon, action, etc but I am not sure of the specs. Susheel could you help out with that ?

Comment 3

[Vlad Baicu]

Attachment: CurrentStumblerNotification

This is how the Stumbler's notification currently looks like as tested on my colleague's OnePlus 6

Comment 4

[:sdaswani]

Andreas and Barbara, are you OK with how the notification looks currently? https://bugzilla.mozilla.org/attachment.cgi?id=8991354

If not then we need you to specify how the text, icon, and what action is performed to customize the notification.

Also NI'ing Chris who has been driving this forward.

Comment 5

[Chris Peterson [:cpeterson]]

LGTM, though it is not clear that the service is related to Fennec. What if the notification text say something like "Firefox Location Service running"? However, the name listed in Fennec's settings menu is "Mozilla Location Service".

What happens if the user taps the "Mozilla Location Service running" notification? It would be cool if it would jump directly to Fennec's settings menu so the user could toggle the service.

Comment 6

[Andreas Bovens [:abovens]]

Vlad, can you share what part of the notification is set by the OS, and what is modifiable? It's unclear to me from looking at the screenshot. I suppose "Fennec petrulingurar" will be replaced by "Firefox"?

Comment 7

[Vlad Baicu]

Yes, it will be replaced by "Firefox" on release builds."Fennec petrulingurar" is the name of the notification given by the OS from the build the screenshot was taken from and "Mozilla Location Service running" is the title of the notification set by us.

According to the docs, these are all the options that we can set on notifications: https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder

From that list, I think the relevant ones for us in this case are icon, title, description and the action when the user taps on the notification but we can of course go much further than this.

Comment 8

[:sdaswani]

Re-adding NIs.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Vlad Baicu from comment #7)
> Yes, it will be replaced by "Firefox" on release builds."Fennec
> petrulingurar" is the name of the notification given by the OS from the
> build the screenshot was taken from and "Mozilla Location Service running"
> is the title of the notification set by us.

In that case, you can ignore my suggestion to replace the notification's "Mozilla Location Service running" string with "Firefox Location Service running". The notification is already titled "Fennec petrulingurar" or "Firefox" and the name "Mozilla Location Service" matches the option name in Fennec's settings menu.

Comment 10

[Andreas Bovens [:abovens]]

FWIW, I agree with Chris about the wording.

Opening the relevant section in the Settings when the notification is tapped would be a good thing to implement.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8991352 [details]
Bug 1474961 - Change StumblerService to a foreground service when targeting Android Oreo.

https://reviewboard.mozilla.org/r/256250/#review263736

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:93
(Diff revision 1)
>  import android.widget.Toast;
>  
>  import org.json.JSONArray;
>  import org.json.JSONException;
>  import org.json.JSONObject;
> +import org.mozilla.mozstumbler.service.mainthread.SafeReceiver;

Move this to another group

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/LocalPreferenceReceiver.java:29
(Diff revision 1)
>  public class LocalPreferenceReceiver extends BroadcastReceiver {
>      // This allows global debugging logs to be enabled by doing
>      // |adb shell setprop log.tag.PassiveStumbler DEBUG|
>      static final String LOG_TAG = "PassiveStumbler";
>  
> +    @SuppressLint("NewApi")

Does this go away if you use `Build.VERSION.SDK_INT` below instead of `AppConstants`?

Comment 12

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1474961 - Change StumblerService to a foreground service when targeting Android Oreo.

Also made broadcasts involving Stumbler explicit.

Review commit: https://reviewboard.mozilla.org/r/256954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256954/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6763cfa891a
Change StumblerService to a foreground service when targeting Android Oreo. r=jchen

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Andreas Bovens [:abovens] from comment #10)
> Opening the relevant section in the Settings when the notification is tapped
> would be a good thing to implement.

I filed bug 1476720 on this.

Comment 16

[:sdaswani]

Brian Jones can we use 'The Mozilla Location Service is running. Visit Settings in Firefox to disable.' ?

Comment 17

[:sdaswani]

Also, Joni we will need a SUMO page explaining this. Can you provide?

Comment 18

[Joni Chan]

Sure, I've drafted a SUMO page. Please add any suggestions here: https://docs.google.com/document/d/1afwL_Hf3gAcBc7Ewu8J8MuwOH0hmqxOjRp_36XWaVXw/edit?usp=sharing

Is it going to be linked from the product?

Comment 19

[:sdaswani]

Thanks Joni!

@petru can you review the draft SUMO page for accuracy please?

Comment 20

[Petru-Mugurel Lingurar [:petru]]

Added my suggestions:
- that the feature can also be disabled by tapping on the notifications - bug 1476720
- tried to emphasize that this is a feature

Comment 21

[Joni Chan]

Thanks for all your feedback. Here's the published article: https://support.mozilla.org/kb/mozilla-location-service-firefox-android

Comment 22

[Vlad Baicu]

I think that we should also mention that the notification is visible only from Android Oreo onwards

Comment 23

[Vlad Baicu]

Also, as a suggestion, maybe some screenshots would fit nicely in there ?

Comment 24

[Joni Chan]

Thanks for the suggestions. I've added mention of Android Oreo. We'll add screenshots once this moves to Beta in case anything changes.