865429 - [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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Justin Lebar (not reading bugmail)]

tef+ because this addresses the root certification blocker in bug 860799.

Comment 2

[Justin Lebar (not reading bugmail)]

Actually, we'll probably have to do this for more than the SMS app.

Comment 3

[Justin Lebar (not reading bugmail)]

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

Comment 4

[Etienne Segonzac (: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/content/shell.js#714
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L148-L171

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9418

Pointer to Github pull-request

Comment 6

[Justin Lebar (not reading bugmail)]

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.

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

Justin, seem like you are the most active in this bug, perhaps you want to take this bug? thanks

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 741990 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9418

r=me, small nit on github

Comment 9

[Justin Lebar (not reading bugmail)]

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?

Comment 10

[James Lal [:lightsofapollo]]

v1-train: 12438a9
v1.0.1: 0b1056b

Comment 11

[Justin Lebar (not reading bugmail)]

This seems to be causing Travis failures, which I'll look into.

Comment 12

[James Lal [:lightsofapollo]]

Justin- Your good here I think... there are failures (Which makes me sad =/) but they are not related to your patch...

Comment 13

[Justin Lebar (not reading bugmail)]

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...

Comment 14

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 15

[Deepa Subramanian]

Can you please provide steps to verify this fix - as we can perform blackbox testing from the UI?