Closed Bug 1267419 Opened 4 years ago Closed 4 years ago

Support notifications on Android in headless mode

Categories

(Core :: DOM: Push Notifications, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed
fennec 48+ ---

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #8745135 - Flags: review?(nalexander)
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?
Attachment #8745135 - Flags: review?(nalexander)
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+
https://hg.mozilla.org/mozilla-central/rev/ad8d1e0fb333
https://hg.mozilla.org/mozilla-central/rev/854d197ffe55
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
We want push for 48
tracking-fennec: --- → ?
[Tracking Requested - why for this release]: Push notifications
tracking-fennec: ? → 48+
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.
Flags: needinfo?(nchen)
Flags: needinfo?(nalexander)
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
Flags: needinfo?(nchen)
Flags: needinfo?(nalexander)
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?
Flags: needinfo?(nchen)
(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.
Flags: needinfo?(nchen)
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+
Depends on: 1275095
James, do you know who could help with this? Thanks
Flags: needinfo?(snorp)
Jim will be back on Monday and can address it then.
Flags: needinfo?(snorp)
You need to log in before you can comment on or make changes to this bug.