Closed
Bug 1267419
Opened 8 years ago
Closed 8 years ago
Support notifications on Android in headless mode
Categories
(Core :: DOM: Notifications, defect)
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)
1.59 KB,
patch
|
jchen
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.86 KB,
patch
|
nalexander
:
review+
|
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.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
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 3•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
Attachment #8745135 -
Attachment is obsolete: true
Comment 5•8 years ago
|
||
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/integration/mozilla-inbound/rev/ad8d1e0fb333 https://hg.mozilla.org/integration/mozilla-inbound/rev/854d197ffe55
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad8d1e0fb333 https://hg.mozilla.org/mozilla-central/rev/854d197ffe55
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Target Milestone: mozilla48 → mozilla49
Comment 9•8 years ago
|
||
[Tracking Requested - why for this release]: Push notifications
tracking-fennec: ? → 48+
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
tracking-firefox48:
--- → ?
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
Tracking for 48 - is this ready to be considered for uplift? Ni on jchen and nalexander to get that answer.
Assignee | ||
Comment 12•8 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?
Comment 13•8 years ago
|
||
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•8 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 15•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/125947b0ec85 https://hg.mozilla.org/releases/mozilla-aurora/rev/61314f00fede
Comment 17•8 years ago
|
||
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
Flags: needinfo?(nchen)
Jim will be back on Monday and can address it then.
Flags: needinfo?(snorp)
Assignee | ||
Comment 20•8 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
Flags: needinfo?(nchen)
You need to log in
before you can comment on or make changes to this bug.
Description
•