Closed Bug 1474961 Opened 6 years ago Closed 6 years ago

Change StumblerService to a foreground service when targeting Android O

Categories

(Firefox for Android Graveyard :: General, enhancement)

Firefox 63
enhancement
Not set
blocker

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: vlad.baicu, Assigned: vlad.baicu, NeedInfo)

References

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(3 files)

No description provided.
Assignee: nobody → vlad.baicu
Blocks: android-o
Depends on: 1450447
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 ?
Flags: needinfo?(sdaswani)
Attached image CurrentStumblerNotification (deleted) —
This is how the Stumbler's notification currently looks like as tested on my colleague's OnePlus 6
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.
Flags: needinfo?(sdaswani)
Flags: needinfo?(cpeterson)
Flags: needinfo?(bbermes)
Flags: needinfo?(abovens)
Whiteboard: --do_not_change--[priority:high]
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.
Flags: needinfo?(cpeterson)
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"?
Flags: needinfo?(abovens) → needinfo?(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.
Flags: needinfo?(vlad.baicu)
Re-adding NIs.
Flags: needinfo?(cpeterson)
Flags: needinfo?(abovens)
Attachment #8991352 - Flags: review?(sdaswani) → review?(nchen)
(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.
Flags: needinfo?(cpeterson)
Keywords: checkin-needed
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.
Flags: needinfo?(abovens)
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`?
Attachment #8991352 - Flags: review?(nchen) → review+
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6763cfa891a Change StumblerService to a foreground service when targeting Android Oreo. r=jchen
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attachment #8992037 - Flags: review?(sdaswani) → review?(nchen)
Depends on: 1476315
Depends on: 1476596
(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.
Brian Jones can we use 'The Mozilla Location Service is running. Visit Settings in Firefox to disable.' ?
Flags: needinfo?(bbermes) → needinfo?(brjones)
Also, Joni we will need a SUMO page explaining this. Can you provide?
Flags: needinfo?(jsavage)
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?
Flags: needinfo?(jsavage) → needinfo?(sdaswani)
Thanks Joni! @petru can you review the draft SUMO page for accuracy please?
Flags: needinfo?(sdaswani) → needinfo?(petru.lingurar)
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
Flags: needinfo?(petru.lingurar)
Thanks for all your feedback. Here's the published article: https://support.mozilla.org/kb/mozilla-location-service-firefox-android
I think that we should also mention that the notification is visible only from Android Oreo onwards
Also, as a suggestion, maybe some screenshots would fit nicely in there ?
Flags: needinfo?(jsavage)
Thanks for the suggestions. I've added mention of Android Oreo. We'll add screenshots once this moves to Beta in case anything changes.
Flags: needinfo?(jsavage)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: