bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Support notifications on Android in headless mode

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
mozilla49
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48+ fixed, firefox49 fixed, fennec48+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8745134 [details] [diff] [review]
Use application context for showing notifications (v1)

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+
(Assignee)

Comment 2

2 years ago
Created attachment 8745135 [details] [diff] [review]
Move PushRecord.getLastVisit handler to PushService.java (v1)

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)
(Assignee)

Comment 4

2 years ago
Created attachment 8745388 [details] [diff] [review]
Move PushRecord.getLastVisit handler to PushService.java (v2)

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)
(Assignee)

Updated

2 years ago
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+

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ad8d1e0fb333
https://hg.mozilla.org/mozilla-central/rev/854d197ffe55
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

2 years ago
Target Milestone: mozilla48 → mozilla49
(Assignee)

Comment 8

2 years ago
We want push for 48
tracking-fennec: --- → ?
[Tracking Requested - why for this release]: Push notifications
tracking-fennec: ? → 48+
status-firefox47: --- → unaffected
status-firefox48: --- → affected
tracking-firefox48: --- → ?
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.
tracking-firefox48: ? → +
Flags: needinfo?(nchen)
Flags: needinfo?(nalexander)
(Assignee)

Comment 12

2 years ago
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)
(Assignee)

Comment 14

2 years ago
(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+
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
status-firefox48: fixed → affected
Flags: needinfo?(nchen)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 20

2 years ago
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
status-firefox48: affected → fixed
Flags: needinfo?(nchen)
You need to log in before you can comment on or make changes to this bug.