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)
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 | ||
Updated•6 years ago
|
Assignee: nobody → vlad.baicu
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
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)
Comment 6•6 years ago
|
||
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)
Assignee | ||
Comment 7•6 years ago
|
||
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)
Comment 9•6 years ago
|
||
(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)
Updated•6 years ago
|
Keywords: checkin-needed
Comment 10•6 years ago
|
||
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 11•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
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
Comment 14•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attachment #8992037 -
Flags: review?(sdaswani) → review?(nchen)
Updated•6 years ago
|
Attachment #8992037 -
Flags: review?(nchen)
Comment 15•6 years ago
|
||
(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•6 years ago
|
||
Brian Jones can we use 'The Mozilla Location Service is running. Visit Settings in Firefox to disable.' ?
Flags: needinfo?(bbermes) → needinfo?(brjones)
Comment 17•6 years ago
|
||
Also, Joni we will need a SUMO page explaining this. Can you provide?
Flags: needinfo?(jsavage)
Comment 18•6 years ago
|
||
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)
Comment 19•6 years ago
|
||
Thanks Joni!
@petru can you review the draft SUMO page for accuracy please?
Flags: needinfo?(sdaswani) → needinfo?(petru.lingurar)
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
Thanks for all your feedback. Here's the published article: https://support.mozilla.org/kb/mozilla-location-service-firefox-android
Assignee | ||
Comment 22•6 years ago
|
||
I think that we should also mention that the notification is visible only from Android Oreo onwards
Assignee | ||
Comment 23•6 years ago
|
||
Also, as a suggestion, maybe some screenshots would fit nicely in there ?
Flags: needinfo?(jsavage)
Comment 24•6 years ago
|
||
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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•