[SMS] Hold high-priority wake lock starting when we receive notification that we intend to display to the user and ending when we display the toast notification

RESOLVED FIXED
(NeedInfo fromanyone)

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug, NeedInfo)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
In bug 860799, Gecko provides Gaia with the ability to cause frames that are expecting system messages to have an elevated priority while in the background.

The trick is, we need to keep that elevated priority until we are actually finished handling the message.  To do this, we must hold the "high-priority" wake lock.

I'll attach a simple patch.
(Assignee)

Comment 1

6 years ago
tef+ because this addresses the root certification blocker in bug 860799.
blocking-b2g: --- → tef+
(Assignee)

Comment 2

6 years ago
Actually, we'll probably have to do this for more than the SMS app.
Component: Gaia::SMS → Gaia
(Assignee)

Updated

6 years ago
Summary: [SMS] Hold high-priority wake lock starting when we receive notification of an SMS and ending when we display the toast notification → [SMS] Hold high-priority wake lock starting when we receive notification that we intend to display to the user and ending when we display the toast notification
(Assignee)

Comment 3

6 years ago
Etienne, can you help me with the notification semantics here?

I noticed that the closeCB of NotificationHelper.send is called when the notification is cleared from the notification tray, not when the toast notification stops showing.  And I also noticed that if I kill the SMS app, I can still click on the notification in the tray and get to the SMS.

So my question is: Do I need to try to keep the SMS app alive while the toast notification is showing, so that clickCB exists?  Or will the notifications code do the right thing if the SMS app is closed before the user clicks on it?

Here's the WIP code so you can see what I'm talking about.  At issue is whether we should release the wake lock immediately after NotificationHelper.send or if we should hold it until clickCB or callCB is called, as we do now.

https://github.com/jlebar/gaia/commit/74db39ef265d19f1ec639ca479a1e6b0625061b8
Flags: needinfo?(etienne)
(In reply to Justin Lebar [:jlebar] from comment #3)
> I noticed that the closeCB of NotificationHelper.send is called when the
> notification is cleared from the notification tray, not when the toast
> notification stops showing. And I also noticed that if I kill the SMS app,
> I can still click on the notification in the tray and get to the SMS.

Gaia apps can register for the "notification" system message, sent when the click callback doesn't exist anymore [1].
It's an (arguably glorious) hack from Vivien :)

> 
> So my question is: Do I need to try to keep the SMS app alive while the
> toast notification is showing, so that clickCB exists?  Or will the
> notifications code do the right thing if the SMS app is closed before the
> user clicks on it?

The sms app will do the right thing even if it was killed [2] (it was one of the 
requirement to remove the sms app "background service").

> 
> Here's the WIP code so you can see what I'm talking about.  At issue is
> whether we should release the wake lock immediately after
> NotificationHelper.send or if we should hold it until clickCB or callCB is
> called, as we do now.

I think we should release it after the send. No need to keep the sms app alive for potentially hours when it can handle the notification correctly anyway (ie. bringing you to the right conversation thread).


[1] https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/﷒0﷓
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L148-L171
Flags: needinfo?(etienne)
(Assignee)

Comment 5

6 years ago
Created attachment 741990 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9418

Pointer to Github pull-request
(Assignee)

Updated

6 years ago
Component: Gaia → Gaia::SMS
(Assignee)

Updated

6 years ago
Attachment #741990 - Flags: review?(etienne)
(Assignee)

Comment 6

6 years ago
I looked through everyone who uses NotificationHelpers, and it looks like we just need to do the SMS app.  Or at least, the other guys aren't getting woken up by system messages, so they're a different case.
Justin, seem like you are the most active in this bug, perhaps you want to take this bug? thanks
Flags: needinfo?(justin.lebar+bug)
Comment on attachment 741990 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9418

r=me, small nit on github
Attachment #741990 - Flags: review?(etienne) → review+
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 9

6 years ago
Merged into Gaia master.  https://github.com/mozilla-b2g/gaia/commit/c6b17d0000c43f7bdf3f46f96a4e9af3bc16c0d8

That's all I need to do, right?  This will be uplifted automatically?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: needinfo?(justin.lebar+bug)
Resolution: --- → FIXED
v1-train: 12438a9
v1.0.1: 0b1056b
status-b2g18: --- → fixed
status-b2g18-v1.0.1: --- → fixed
(Assignee)

Comment 11

6 years ago
This seems to be causing Travis failures, which I'll look into.
Justin- Your good here I think... there are failures (Which makes me sad =/) but they are not related to your patch...
(Assignee)

Comment 13

6 years ago
Hm.  Maybe this should be holding the CPU wake lock; otherwise, it's possible that we'll not buzz the phone when you receive an SMS...
(Assignee)

Updated

6 years ago
Depends on: 867649
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Flags: needinfo?

Comment 15

6 years ago
Can you please provide steps to verify this fix - as we can perform blackbox testing from the UI?
You need to log in before you can comment on or make changes to this bug.