836737 - we should not launch the app when we close a notification

Status: VERIFIED FIXED

Description

[Julien Wajsberg [:julienw]]

STR:
* launch the app
* receive a SMS
* kill the app (long press home button and swipe up the app)
* click on the "clear all notification" in the notification screen

Expected:
* the notifications are cleared and that's it

Actual:
* the notifications are cleared and the Sms app is opened

With the attached patch, I can see tha

Comment 1

[Julien Wajsberg [:julienw]]

Created attachment 708561 [details] [diff] [review]
patch v1

one liner which check the type of the notification and doesn't launch the app if the user merely closed it

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 708561 [details] [diff] [review]
patch v1

carry forward r+ from :vingtetun which reads my patch over my shoulders

Comment 3

[Julien Wajsberg [:julienw]]

We need a tef+ on that because this affects _all_ apps which uses notification handlers.

Comment 4

[Julien Wajsberg [:julienw]]

+ this is very low risk as there are only 2 cases and I tested both cases.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 708561 [details] [diff] [review]
patch v1

obsoleting, we have a new use case that needs to be taken care of

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

If we fix it the way we suggest, which is still sending back the event and fixing the listeners to check if the notification has been cancelled or not we would have to fix the following call sites:

apps/communications/dialer/js/dialer.js:  window.navigator.mozSetMessageHandler('notification', handleNotification);
apps/costcontrol/js/app.js:    window.navigator.mozSetMessageHandler('notification',
apps/sms/js/sms.js:  window.navigator.mozSetMessageHandler('notification',

Comment 7

[Alex Keybl [:akeybl]]

Tracking, but we won't tef+ until we find/land a fix for v1.x.

Comment 8

[Andrew Overholt [:overholt]]

This blocks a CS blocker so it should be tef+.

Comment 9

[Julien Wajsberg [:julienw]]

Created attachment 710576 [details] [diff] [review]
gaia patch v1

We'll receive a notification if the user clicks on the notif or if he closes it,
but we want to do stuff only when the user clicks on it.

The gecko patch needs to land first.
---
 apps/communications/dialer/js/dialer.js |    6 ++++-
 apps/costcontrol/js/app.js              |    7 +++++-
 apps/sms/js/sms.js                      |   37 ++++++++++++++++++-------------
 3 files changed, 32 insertions(+), 18 deletions(-)

Comment 10

[Julien Wajsberg [:julienw]]

Created attachment 710577 [details] [diff] [review]
gecko patch v2

Bug 836737 - we should not launch the app when we close a notification a=tef+

add the topic in the message

Comment 11

[Julien Wajsberg [:julienw]]

Created attachment 710578 [details] [diff] [review]
gaia patch v1

(same patch, bigger context.)

We'll receive a notification if the user clicks on the notif or if he closes it,
but we want to do stuff only when the user clicks on it.

The gecko patch needs to land first.
---
 apps/communications/dialer/js/dialer.js |    6 ++++-
 apps/costcontrol/js/app.js              |    7 +++++-
 apps/sms/js/sms.js                      |   37 ++++++++++++++++++-------------
 3 files changed, 32 insertions(+), 18 deletions(-)

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 710577 [details] [diff] [review]
gecko patch v2

Review of attachment 710577 [details] [diff] [review]:
-----------------------------------------------------------------

Let's discuss about this flag before r+'ing it.

::: b2g/chrome/content/shell.js
@@ +677,2 @@
>          gSystemMessenger.sendMessage("notification", {
> +            topic: topic,

I wonder if we really want to pass the topic or if we want to pass a boolean flag instead?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 710578 [details] [diff] [review]
gaia patch v1

Review of attachment 710578 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the idea of the patch - but this should be changed if we decided to use a boolean flag.

Comment 14

[Julien Wajsberg [:julienw]]

Created attachment 711296 [details] [diff] [review]
gaia patch v2

use a boolean instead of the topic value

Comment 15

[Julien Wajsberg [:julienw]]

Created attachment 711297 [details] [diff] [review]
gecko patch v3

we use a boolean instead of the topic value now

Comment 16

[Julien Wajsberg [:julienw]]

checkin-needed for the gecko patch.

I'll land the gaia patch myself after this.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24995693c6ea

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24995693c6ea

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/fd1acc2aa727
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/a9b537435d6b

Comment 20

[Julien Wajsberg [:julienw]]

reseting the flags as I need to land the gaia part now.

Comment 21

[Julien Wajsberg [:julienw]]

gaia master: https://github.com/julienw/gaia/commit/7cec8abf54f661238ba4d4765e53f61485f54643

need uplift on the correct branches on gaia

Comment 22

[Julien Wajsberg [:julienw]]

we need an uplift here.

(and I'd like an explanation of which flags need to be set to reliably trigger an uplift :) )

Comment 23

[Julien Wajsberg [:julienw]]

changing components as this may make this bug visible :)

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> gaia master:
> https://github.com/julienw/gaia/commit/
> 7cec8abf54f661238ba4d4765e53f61485f54643
> 
> need uplift on the correct branches on gaia

Looks like this commit is on the required branches:

$ git branch --contains 7cec8abf54f661238ba4d4765e53f61485f54643
  master
  v1-train
  v1.0.0
* v1.0.1

Comment 26

[Julien Wajsberg [:julienw]]

Seems like I've put a wrong commit.

The good commit is : https://github.com/julienw/gaia/commit/8683cf6d9cc428b73b438bbed24e4f48970bd8a1

sorry about that.

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

I thought it was a little weird that the bug numbers didn't match up.

v1-train: 8d413db447d50281853b9005374373f7d2fd2b5e
v1.0.1: 9eb15be2aee695828abcc3321fb7c0e5e4bb87a6
v1.0.0: 95ca8df0bff9295b71de967465f60ccf42d2390f

Comment 28

[mdaniloff]

Verified Fixed in Unagi build #20130215070202
Dec 5th Kernel
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a9e4f8912607
Gaia   21ba59d933c66024cb351c2379315301d5352e0c
Update Channel: Nightly