Closed Bug 836737 Opened 9 years ago Closed 9 years ago

we should not launch the app when we close a notification

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Assignee: nobody → felash
Attached patch patch v1 (obsolete) — Splinter Review
one liner which check the type of the notification and doesn't launch the app if the user merely closed it
Attachment #708561 - Flags: review?(21)
Comment on attachment 708561 [details] [diff] [review]
patch v1

carry forward r+ from :vingtetun which reads my patch over my shoulders
Attachment #708561 - Flags: review?(21) → review+
Keywords: checkin-needed
We need a tef+ on that because this affects _all_ apps which uses notification handlers.
+ this is very low risk as there are only 2 cases and I tested both cases.
Comment on attachment 708561 [details] [diff] [review]
patch v1

obsoleting, we have a new use case that needs to be taken care of
Attachment #708561 - Attachment is obsolete: true
Blocks: 832082
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',
Tracking, but we won't tef+ until we find/land a fix for v1.x.
This blocks a CS blocker so it should be tef+.
blocking-b2g: tef? → tef+
Attached patch gaia patch v1 (obsolete) — Splinter Review
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(-)
Attachment #710576 - Flags: review?(21)
Attached patch gecko patch v2 (obsolete) — Splinter Review
Bug 836737 - we should not launch the app when we close a notification a=tef+

add the topic in the message
Attachment #710577 - Flags: review?(21)
Attached patch gaia patch v1 (obsolete) — Splinter Review
(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(-)
Attachment #710576 - Attachment is obsolete: true
Attachment #710576 - Flags: review?(21)
Attachment #710578 - Flags: review?(21)
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?
Attachment #710577 - Flags: review?(21)
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.
Attachment #710578 - Flags: review?(21)
Attached patch gaia patch v2Splinter Review
use a boolean instead of the topic value
Attachment #710578 - Attachment is obsolete: true
Attachment #711296 - Flags: review?(21)
Attached patch gecko patch v3Splinter Review
we use a boolean instead of the topic value now
Attachment #710577 - Attachment is obsolete: true
Attachment #711297 - Flags: review?(21)
checkin-needed for the gecko patch.

I'll land the gaia patch myself after this.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24995693c6ea
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
reseting the flags as I need to land the gaia part now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
gaia master: https://github.com/julienw/gaia/commit/7cec8abf54f661238ba4d4765e53f61485f54643

need uplift on the correct branches on gaia
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
we need an uplift here.

(and I'd like an explanation of which flags need to be set to reliably trigger an uplift :) )
Keywords: checkin-needed
changing components as this may make this bug visible :)
Component: General → Gaia
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
(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
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
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.