Closed
Bug 836737
Opened 12 years ago
Closed 12 years ago
we should not launch the app when we close a notification
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(blocking-b2g:tef+, 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)
4.00 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → felash
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 3•12 years ago
|
||
We need a tef+ on that because this affects _all_ apps which uses notification handlers.
Assignee | ||
Comment 4•12 years ago
|
||
+ this is very low risk as there are only 2 cases and I tested both cases.
Assignee | ||
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
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',
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
(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 12•12 years ago
|
||
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 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
use a boolean instead of the topic value
Attachment #710578 -
Attachment is obsolete: true
Attachment #711296 -
Flags: review?(21)
Assignee | ||
Comment 15•12 years ago
|
||
we use a boolean instead of the topic value now
Attachment #710577 -
Attachment is obsolete: true
Attachment #711297 -
Flags: review?(21)
Attachment #711296 -
Flags: review?(21) → review+
Attachment #711297 -
Flags: review?(21) → review+
Assignee | ||
Comment 16•12 years ago
|
||
checkin-needed for the gecko patch.
I'll land the gaia patch myself after this.
Keywords: checkin-needed
Comment 17•12 years ago
|
||
Keywords: checkin-needed
Comment 18•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 19•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fd1acc2aa727
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/a9b537435d6b
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Target Milestone: --- → B2G C4 (2jan on)
Assignee | ||
Comment 20•12 years ago
|
||
reseting the flags as I need to land the gaia part now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•12 years ago
|
||
gaia master: https://github.com/julienw/gaia/commit/7cec8abf54f661238ba4d4765e53f61485f54643
need uplift on the correct branches on gaia
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Assignee | ||
Comment 22•12 years ago
|
||
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
Assignee | ||
Comment 23•12 years ago
|
||
changing components as this may make this bug visible :)
Component: General → Gaia
Comment 24•12 years ago
|
||
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
status-b2g18-v1.0.1:
--- → affected
Comment 25•12 years ago
|
||
(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
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 26•12 years ago
|
||
Seems like I've put a wrong commit.
The good commit is : https://github.com/julienw/gaia/commit/8683cf6d9cc428b73b438bbed24e4f48970bd8a1
sorry about that.
Comment 27•12 years ago
|
||
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•12 years ago
|
||
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.
Description
•