Closed Bug 1267419 Opened 4 years ago Closed 4 years ago
Support notifications on Android in headless mode
1.59 KB, patch
|Details | Diff | Splinter Review|
8.86 KB, patch
|Details | Diff | Splinter Review|
Currently, Fennec is able to start Gecko in headless mode and deliver a push message, but we're not able to show a notification from Gecko in that case. We should fix that.
Use the application context for showing alert notifications, so we don't rely on GeckoApp activity being created. r=me for trivial change.
Attachment #8745134 - Flags: review+
The Android-specific handler for PushRecord.getLastVisit used to be implemented in GeckoApp, which is unavailable in headless mode. Moving the handler to PushService makes background notifications work.
Comment on attachment 8745135 [details] [diff] [review] Move PushRecord.getLastVisit handler to PushService.java (v1) Review of attachment 8745135 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with this approach, but I don't want to see History:... cloned into the PushService:... There are no non-test consumers of the message (that I know of). Can you keep the message name, etc, and just move the listener? Basically, don't duplicate and keep the test functional. I feel this is a common pattern that we get wrong across the board. Is this the time we can address it?
I did miss the test in the previous patch. I changed the name back in this patch so the test should work again. What did you mean by don't duplicate? The previous patch removed the listener from GeckoApp and added it to PushService, so I don't think it was duplicating the listener.
Attachment #8745388 - Flags: review?(nalexander)
Attachment #8745135 - Attachment is obsolete: true
Comment on attachment 8745388 [details] [diff] [review] Move PushRecord.getLastVisit handler to PushService.java (v2) Review of attachment 8745388 [details] [diff] [review]: ----------------------------------------------------------------- My error about the duplication earlier -- I missed the deletion. If it's green on try, it's good for me. Thanks!
Attachment #8745388 - Flags: review?(nalexander) → review+
We want push for 48
tracking-fennec: --- → ?
[Tracking Requested - why for this release]: Push notifications
Note: Android Devices Build ID 20160505030327 - Observed on Samsung Galaxy Tab 2 tablet 1) Access https://serviceworke.rs/push-simple_demo.html 2) Set Notification Delay to 15 seconds 3) Press [Try to conquer Italy!] button 4) Press the ^ button and access the Task Manager (Task Manager window is displayed) 5) Press the (X) to close Firefox Nightly 6) Wait for a notification to appear in the notification area/bar Observe: No Fx icon appears in the notification area. PLEASE NOTE: This is odd, if one closes Firefox via Recent Apps feature, the Fx notification does appear in the notification area. To see the difference and why I chose to reopen this issue: 1) Repeat steps 1-3 2) Press the Home button on the device 3) Press Recent Apps button on the device and close Firefox (Dbl Chk, you will see Fx is not displayed in the Task Manager, it is indeed closed.) 4) Wait for notification to appear Observe: The Fx icon is displayed in the notification area.
Tracking for 48 - is this ready to be considered for uplift? Ni on jchen and nalexander to get that answer.
Comment on attachment 8745134 [details] [diff] [review] Use application context for showing notifications (v1) Requesting uplift for both patches. Approval Request Comment [Feature/regressing bug #]: Push notifications [User impact if declined]: Cannot show push notifications in 48 [Describe test coverage new/current, TreeHerder]: Locally, m-c [Risks and why]: Some risk; enabling push notifications could expose bugs but it's better than no push notifications at all. [String/UUID change made/needed]: None
Attachment #8745134 - Flags: approval-mozilla-aurora?
Hi Jim, Do you have any feedback on Michelle's test results based on comment #10? It seems that the behaviors are inconsistent?
(In reply to Gerry Chang [:gchang] from comment #13) > Hi Jim, > Do you have any feedback on Michelle's test results based on comment #10? It > seems that the behaviors are inconsistent? This bug is ready for uplifting. That issue is know and is being tracked separately.
Comment on attachment 8745134 [details] [diff] [review] Use application context for showing notifications (v1) OK, taking it then. thanks!
Attachment #8745134 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out for Android test_get_last_visited.html failures. https://hg.mozilla.org/releases/mozilla-aurora/rev/59db6ee36a99 https://treeherder.mozilla.org/logviewer.html#?job_id=2701861&repo=mozilla-aurora#L3645
James, do you know who could help with this? Thanks
Jim will be back on Monday and can address it then.
Re-uplifted to 48 since bug 1275095 has been fixed. https://hg.mozilla.org/releases/mozilla-beta/rev/150bf636e0dc https://hg.mozilla.org/releases/mozilla-beta/rev/89f21e80a2ad
You need to log in before you can comment on or make changes to this bug.