Closed Bug 855165 Opened 11 years ago Closed 10 years ago

[fugu][Messages] Synchronize the notification with the actual message state

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g18?)

RESOLVED FIXED
Tracking Status
b2g18 ? ---

People

(Reporter: sync-1, Assigned: gerard-majax)

References

Details

(Whiteboard: [u=commsapps-user c=messaging p=0][comms-triaged][mentor=:julienw])

Attachments

(1 file)

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.19.037
 Firefox os  v1.0.1
 Mozilla build ID: 20130310070203.
 
 +++ This bug was initially created as a clone of Bug #427675 +++
 
 [WuYing][6915]
 DEFECT DESCRIPTION:
 If the UI stay in SMS message list, then received a new SMS, read the new SMS, but the notification can't disappear
 
 REPRODUCING PROCEDURES:
 1.the UI stay in SMS message list
 2.received a new SMS
 3.read this new SMS
 4.in this time, the SMS prompt in the notification cannot disappear.---KO
 
 
 EXPECTED BEHAVIOUR:
 After have readed new SMS, the prompt icon shoud disapper in notification bar.
 
 
 ASSOCIATE SPECIFICATION: 
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 medium
 REPRODUCING RATE:
 5/5
 For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #427675 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
 DEFECT DESCRIPTION:
 
 REPRODUCING PROCEDURES:
 
 EXPECTED BEHAVIOUR:
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:
There are two problems:

1.we close the app sms, and then we receive several messages from other people. Then we open the app 'sms' again.All of the messages without read, but their notification is disappear! I think it's much better to clear a 'sms' notification after a end user read it.

2.The event "handleAppopen" will just been triggered when we start the  sms app. Imagine this, we are just in this sms application, and we receive a lot short messages. Then we read all of these new messages. All of the notifications are still display on the toorbar Without trigger the event "handleAppopen" although we have read all of them and they are not new any longer.
We needed Bug 782211 to properly implement the notification handling. It landed 10 days ago in moz-central.

So either we uplift this to b2g18 (and I think it will be challenging, there are lots of parts and follow-ups), either we switch back to at least gecko 22.

I suspect we'll need code in both System and Sms applications.
tracking-b2g18: --- → ?
(In reply to Julien Wajsberg [:julienw] from comment #2)
> We needed Bug 782211 to properly implement the notification handling. It
> landed 10 days ago in moz-central.
> 
> So either we uplift this to b2g18 (and I think it will be challenging, there
> are lots of parts and follow-ups), either we switch back to at least gecko
> 22.
> 
> I suspect we'll need code in both System and Sms applications.

I totally agree with that maybe we need code both in system and sms application. We need system event which can listen the status wheather a message is read or not. Then do something in notification.js such as close a sms notification.
Based on Julien's comment 2 this looks like it would actually be too risky to take in v1.1 and might have to wait for v1.next when gecko is updated.
blocking-b2g: --- → koi?
That's a bit sad, from a UX point of view, this is one of the biggest feedback/issue I'm getting from real people testing on Geeksphone Keon.
See Bug 868816 for yet another work around we might get in v1.1.
the Problem is still existed in 

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.107
Firefox os  v1.0.1
Mozilla build ID:20130514070202

Could we fix it in 1.1?
blocking-b2g: koi? → leo?
See the previous comments, we won't fix it in 1.1, as we don't have the necessary tools yet. See also Bug 868816 comment 11.

I agree this is unfortunate but doing it now would be extremely risky and would require a lot of work.
blocking-b2g: leo? → ---
Nom'ing for Koi given the riskiness of implementing in Leo.
blocking-b2g: --- → koi?
For koi we should have clear use cases around the notifications so that we don't miss anything.
Guys, I was studying about the "navigator.mozNotification" and this API doesn't alow us to remove a notification, only create new ones, right? I agree with the idea of an application control its notifications, like the Android API.

What do you think about this approach?
Caio> it's already done (see bug 782211) but just not available in Firefox OS 1.0.1 and 1.1. It will be available in Firefox OS 1.2 though.
So, We could put this bug as dependent of bug 782211. This way, the solution would be only in the SMS application.

Do you think that it's a good idea if I work on this bug as a branch of bug 782211?
It's definitely dependent on bug 782211, thanks :)

You can work on this of course, but I think the whole notification handling should be reworked then. We need to use a real state object instead of using the image url for example.
Depends on: 782211
> Julien Wajsberg

How could I verify if my source tree has the implementations of bug 782211? I'm have some problems because I'm newbie.

After verified It, I think i could start work in this bug.
You just need a mozilla-central tree, but you'll need to apply https://bug782211.bugzilla.mozilla.org/attachment.cgi?id=670640 in gaia too, because this has not landed yet.
If do I build my gecko as mozilla-central solve this problem?

I was looking into my gaia tree and It doesn't have a dom/src/notification/Notification.cpp
Yep, most of the notification implementation is in gecko.

If you know how to build your FxOS build, then just build it out of master.

Eg in your B2G directory:

BRANCH=master ./config.sh <device>
./build.sh
./flash.sh gecko

see https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Preparing_for_your_first_B2G_build#Building_a_branch
I'm looking the sms app from Gaia source tree, but I didn't find where the notification is trigged when a sms is recieved.

The MessageManager has a listener for _mozMobileMessage('recieved'), that is onMessageReceived

This method doesn't trigger a notification. Using the informations of this discussion, I guess that this notification is trigged by system app. Could anyone confirm this?
not really.

have a look to the badly-named activity_handler.js file. Here we register a message handler on "sms-received" that is sent by gecko. You'll see that we also register a message handler for "notification", which is currently used when the notification is clicked or dismissed.
I have already found the code from notification and I've made some tests to validate my guesses. The SMS app uses  the shared/NotificationHelper to trigger a new notification. But in current implementation, there is no logic to remove the notification.

Analysing the W3C specification(see http://www.w3.org/TR/notifications/) thre is a close event. It is implemented in "dom/Notification.webidl" and in "dom/src/notofication/Notification.cpp" in mozilla-central. However, looking into "system/js/notifications.js" from Gaia's system app, there is no implementation of a listener for "desktop-notification-close".

So, I can work to solve this bug, but don't you think that is better create a new bug report to solve this issue in Gaia's system application and after solve this bug from sms? Because it will be a problem to 3rd party application developers too.

My blueprint is make the system app listen to notifications' "desktop-notification-close" event and after implement triggers in SMS application to force notifications' close.
Please read again my comment 17 ;)

one of the patches in bug 782211 has not landed. I'll do it myself later today so that we can move forward.
> Please read again my comment 17 ;)

Sorry, I didn't see your comment. So I will apply it in a branch to study the code and try to move forward.

See about the https://bugzilla.mozilla.org/show_bug.cgi?id=892528

I was talking with vingtetun on IRC and he/she said me that there is somebody working in this part of API. Are you in this team?
Depends on: 899574
I have found an important thing in current Notification API. The method "navigator.mozNotification.createNotification" return the notification object implemented by "/dom/src/notification/DesktopNotification.h" and "/dom/src/notification/DesktopNotification.cpp". The "dom/webidl/DesktopNotification.webidl" is the interface to Gaia layer, but it doesn't have a close method defined. I'm thinking in implement a close in this interface, at least as a proof of concept.
Actually, to use the new Notification API, you must use "new Notification(...)" instead of the "createNotification" method.

see http://notifications.spec.whatwg.org/#api for more information.
Ok then. In this case, I'll focus on change the notification_helper to use this new API, given that it is according with the W3C specification. This is the best way to follow in my mind. I'll try using this object correctly now. Thank you!
bug 900279 is the new bug for the gaia patch for the System app.
Assignee: nobody → ticaiolima
Depends on: 900279
Julien, I'm having problems to instance a new Notification. having a litte view in the Notification, I've found that it is PrefEnabled. So, I've added the line prefs.push(["dom.webnotifications.enabled", true]); in /gaia/build/preferences.js. But with this configuration, I'm getting this return from console:

"[JavaScript Error: "TypeError: Notification is not a constructor" {file: "app://sms.gaiamobile.org/shared/js/notification_helper.js" line: 55}]"

I'm running this build in emulator. have you already used this object?
To be fair I don't know much more.

Have you tried with "mozNotification" ?

Gregor, do you have any insight on how the API currently works ?
Flags: needinfo?(anygregor)
I'm trying to change the "DesktopNotification" now, but having some compilation problems. Sorry for this doubt, but there is a way of partial build of B2G as the Firefox has with "./mach dom", for instance?
You can still do the same in the gecko tree (see the "gecko" subdirectory in the B2G directory). Or even use an external gecko tree (see https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Customization_with_the_.userconfig_file).

But I really think that modifying DesktopNotification is not a good path, rather we should understand how to use the new Notification API.
Yes, You are right. I'm doing it to try to understand how the biding between C++ and javascript with the webidl works.

My guess about the Notification error is because of PrefEnabled modifier. I was looking in a way to see the preferences of My B2G build and set values for them. I've tried adding prefs.push(['dom.notifications.enabled',true]) in "/gaia/build/preferences.js" 

Using the gdb, I had success in mark a breakpoint into Notification.cpp. in my build steps, the Notification.webidl is processed. A think that the critical point here is know if the Notification binding is being done or how it's being done.
(In reply to Caio Lima(:caiolima) from comment #33)
> Yes, You are right. I'm doing it to try to understand how the biding between
> C++ and javascript with the webidl works.
> 
> My guess about the Notification error is because of PrefEnabled modifier. I
> was looking in a way to see the preferences of My B2G build and set values
> for them. I've tried adding prefs.push(['dom.notifications.enabled',true])
> in "/gaia/build/preferences.js" 

be careful, it's "dom.webnotifications.enabled". I filed bug 900960 to enable the new Notification object by default, I don't know why it was disabled.

To change a pref the best is probably to create a file 'build/custom-prefs.js' with the following content:

  user_pref('dom.notifications.enabled', true);

Then you need to "make reset-gaia" to flash everything otherwise the new prefs are not used.

> 
> Using the gdb, I had success in mark a breakpoint into Notification.cpp. in
> my build steps, the Notification.webidl is processed. A think that the
> critical point here is know if the Notification binding is being done or how
> it's being done.

It's all a little magical, but it depends on that pref being set for sure.
I have another guess now. The gaia/xul-runner-sdk/idl has the nsIDOM*.idl of all objects that we can use in the gaia's dom. I didn't find the nsIDOMNotification.idl yet. In my opinion, The Notification API doesn't have a interface to Gaia's dom yet. I'll see how could I create new dom objects. What do you think about it?
The fact is that I know I can access a Notification object inside my Firefox Aurora :) So it should work.

I'm building a mozilla-central gecko to try this out.
We have too many open bugs around this :(
mhenretty is starting to work on bug 892528 today or on monday. It should solve the persisting, querying and closing problem.
Please sync with mike if you already have some code.
Flags: needinfo?(anygregor)
Gregor: here we're focussing on the Sms implementation, so no work duplication so far :) but we have a hard time just accessing the API
Depends on: 892528
Julien, I will wait your report about usage of new Notification API. I'm rebuilding my B2G with mozilla-central either.
So, it "kind of" works. I mean, something happens when I do 

  new Notification("title", { body: "something" });

But it crashes ;)

I tried in the browser only, we should try in an app instead.

see http://everlong.org/mozilla/notifications/ for my testcase.
Depends on: 901214
Sorry for ask, but where did you test this app? Emulator, Simulator or device?
on the device
Ok. I've applied your patch from 900960. It's in mozilla-central, right? So, I'll rebuild my gecko and see what happens.
Blocks: koi-comms
blocking-b2g: koi? → ---
Whiteboard: [u=commsapps-user c=messaging p=0]
Blocks: comms_backlog
No longer blocks: koi-comms
Guys, I've find the solution and started to work in this bug now. To enable the new Notification API, I've used the |BRANCH=master ./config.sh <device>| and built the new source tree.

However, I'm with a new problem. There is a global object created in sms/notification.js called Notification. This object is overriding the Notification API. I've renamed the Notification of sms/notification.js and the new Notification API worked well.

But I don't know if this is the right implementation, given this object is used by other parts of sms application. Could anyone suggest another way to solve this(if it is possible)?
(In reply to Caio Lima(:caiolima) from comment #44)
> Guys, I've find the solution and started to work in this bug now. To enable
> the new Notification API, I've used the |BRANCH=master ./config.sh <device>|
> and built the new source tree.
> 
> However, I'm with a new problem. There is a global object created in
> sms/notification.js called Notification. This object is overriding the
> Notification API. I've renamed the Notification of sms/notification.js and
> the new Notification API worked well.
> 
> But I don't know if this is the right implementation, given this object is
> used by other parts of sms application. Could anyone suggest another way to
> solve this(if it is possible)?

That's fine, can you rename it "Notice"? That seems like it will be sufficiently similar enough.
Summary: [Buri][SMS]If the UI stay in SMS message list, then received a new SMS, read the SMS, but the notification can't disappear → [Messages] If the UI stay in SMS message list, then received a new SMS, read the SMS, but the notification can't disappear
(In reply to Rick Waldron from comment #45) 
> That's fine, can you rename it "Notice"? That seems like it will be
> sufficiently similar enough.

Yes, Of course I can!
 
Since it's a kind of prefactor to this bug fix, I guess It will be better to file a new bug to this, because it's an object used by a lot of other soruces in sms . Im my first impact set, there is over 10 files to be changed.

I've already filed a bug, could anyone confirm it? So I can work on this!

https://bugzilla.mozilla.org/show_bug.cgi?id=906544
Depends on: 906544
Caio> Thanks, makes sense to have this in another bug :)
Summary: [Messages] If the UI stay in SMS message list, then received a new SMS, read the SMS, but the notification can't disappear → [Messages] Synchronize the notification with the actual message state
Blocks: 890440
Dears, as a FFOS phone user, I really confused by the status of messages, I think it shoudn't keep un-synced in the third official release, what do you think?
blocking-b2g: --- → koi?
I agree but we need all the dependant bugs before doing anything, that's why I think it will be in 1.3 rather than 1.2, because feature freeze is very soon.
If we rush to have this in 1.2, we will probably have to fix numerous bugs during the convergence phase, and I'd rather do this quietly during the 1.3 work.
(In reply to Julien Wajsberg [:julienw] from comment #51)
> If we rush to have this in 1.2, we will probably have to fix numerous bugs
> during the convergence phase, and I'd rather do this quietly during the 1.3
> work.

Yes. Jack Liu, If you have a look on the dependence tree,you will notice that to solve this bug, we are working to make the new Notification API stable on FFOS. It's not being a trivial work. I agree with you, but as Julien said, It's better to do things right. :)
I think there is still some time before v1.2 shipment, although it's ideal that we have every thing ready at the very beginning but customer won't wait, so we just do as the Chinese saying -- cross the unfamiliar river carefully by exploring the stones under water.
Actually, I guess that the bug that It's really depending is https://bugzilla.mozilla.org/show_bug.cgi?id=899574

With this bug solved, I can start work to sync the SMS.
Let's move forward anyway, and I'm confident we'll take it in 1.2 if this is ready and in a good shape.
comms-triage: dependency with 2 bugs so waiting for them to be solved. Put ni? to engineering team to review the feasibility of taking it for v1.2 once they are solved.
Flags: needinfo?(felash)
Whiteboard: [u=commsapps-user c=messaging p=0] → [u=commsapps-user c=messaging p=0][comms-triaged]
I'm only afraid of small problems that will get discovered as we move closer and closer to the release, that's why I'd want to do this only after the branching and have this in 1.3 for sure, and backport to 1.2 if we see everything works ok.
Flags: needinfo?(felash)
blocking-b2g: koi? → 1.3?
Whiteboard: [u=commsapps-user c=messaging p=0][comms-triaged] → [u=commsapps-user c=messaging p=0][comms-triaged][mentor=:julienw]
Guys, I'm waiting for the bug #899574 be fixed to start work on this. But I gues we could start some discussions about the implementation. I have already have some blueprints:

1. My first idea is to store the notification ids "thread-wise". So, when the thread start to be loaded, we can get the notifications and close them. In this case, how the thread_id is generated?

2. Will we think about the notifications when the system restart?
(In reply to Caio Lima(:caiolima) from comment #58)
> Guys, I'm waiting for the bug #899574 be fixed to start work on this. But I
> gues we could start some discussions about the implementation. I have
> already have some blueprints:
> 
> 1. My first idea is to store the notification ids "thread-wise". So, when
> the thread start to be loaded, we can get the notifications and close them.
> In this case, how the thread_id is generated?

The thread id is automatically generated by the gecko API.


> 
> 2. Will we think about the notifications when the system restart?

Yep I think we'll need to store them in an Indexed DB, but maybe not in this bug, as this is not even implemented in the System app yet.

(see why I want to avoid async storage for the drafts feature ? ;) or we could make asyncStorage evolve to be able to use different object stores.)

But maybe we'll have to store a mapping table anyway for supporting the case where the SMS app restarts. Or abuse the image url again since we don't have bug 899585 yet ?
(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #59) 
> The thread id is automatically generated by the gecko API.

Could you point me where it happens on Messaging APP, please? =)

> Yep I think we'll need to store them in an Indexed DB, but maybe not in this
> bug, as this is not even implemented in the System app yet.

Maybe We can do it on this bug. Why not? 
 
> (see why I want to avoid async storage for the drafts feature ? ;) or we
> could make asyncStorage evolve to be able to use different object stores.)

I didn't get your point here. Do you mind clarify it?

> But maybe we'll have to store a mapping table anyway for supporting the case
> where the SMS app restarts. Or abuse the image url again since we don't have
> bug 899585 yet ?

Sorry for this, but I don't understand the meaning of "abuse" in this statement.
BTW, I prefer to store a mapping table, as you said. Even we can't do it on device restart, we will support the app restarting, as you said and it's not so difficult to implement. What do you think about the idea of store them as a list by Thread ID?
Not blocking, but we will definitely take a patch on this bug as soon as it is fixed !
minused per Comment 61
blocking-b2g: 1.3? → -
(In reply to Caio Lima(:caiolima) from comment #60)
> (In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday
> and friday) from comment #59) 
> > The thread id is automatically generated by the gecko API.
> 
> Could you point me where it happens on Messaging APP, please? =)

Well, when we send a message or receive a message, we have the threadId as a property of the message.

> 
> > Yep I think we'll need to store them in an Indexed DB, but maybe not in this
> > bug, as this is not even implemented in the System app yet.
> 
> Maybe We can do it on this bug. Why not? 

I like doing features in small steps. Easier to review, easier to find bugs !

>  
> > (see why I want to avoid async storage for the drafts feature ? ;) or we
> > could make asyncStorage evolve to be able to use different object stores.)
> 
> I didn't get your point here. Do you mind clarify it?

The current asyncStorage lib uses one db with one objectstore. Could be useful to make it evolve so that we can specify an alternative objectstore if we want to use it.

> 
> > But maybe we'll have to store a mapping table anyway for supporting the case
> > where the SMS app restarts. Or abuse the image url again since we don't have
> > bug 899585 yet ?
> 
> Sorry for this, but I don't understand the meaning of "abuse" in this
> statement.

We use the "image url" to put some parameters in the notification, so that we can retrieve them when the user taps the notification. I say "abuse" because the "image url" is not meant to do this, this is not clean.

> BTW, I prefer to store a mapping table, as you said. Even we can't do it on
> device restart, we will support the app restarting, as you said and it's not
> so difficult to implement. What do you think about the idea of store them as
> a list by Thread ID?

Problem is that I don't know what we can store.
So, Now we can start work on that. I'll try to implement the ideas presented on comment 60. Maybe it could be more clear. =)
Note that you'll need a patch for bug 890440 to be able to do all the necessary code here.
(In reply to Julien Wajsberg [:julienw] from comment #65)
> Note that you'll need a patch for bug 890440 to be able to do all the
> necessary code here.

Yes. Based on this, this code should depend on the 890440, right? But in the current state, It's the opposite.
I don't want to land bug 890440 until we do this (and probably other things in other apps), because this would break some use cases, hence this dependency direction.

I mean, we can do all the work here and land here, and still not land bug 890440. But you need the patch in bug 890440 to do the work here correctly ;)

Is this clear ?
(In reply to Julien Wajsberg [:julienw] from comment #67)
> I don't want to land bug 890440 until we do this (and probably other things
> in other apps), because this would break some use cases, hence this
> dependency direction.
> 
> I mean, we can do all the work here and land here, and still not land bug
> 890440. But you need the patch in bug 890440 to do the work here correctly ;)
> 
> Is this clear ?

Yes. I was with this in mind =)
Summary: [Messages] Synchronize the notification with the actual message state → [fugu][Messages] Synchronize the notification with the actual message state
Is there anyone handling this issue? I would like to arrange someone to steal it.
Flags: needinfo?(ticaiolima)
(In reply to styang from comment #69)
> Is there anyone handling this issue? I would like to arrange someone to
> steal it.

I'm sorry, I've forgot to unset it. I'm a little busy, so you can steal it with no problem =D
Flags: needinfo?(ticaiolima)
Assignee: ticaiolima → administration
Assignee: administration → nobody
blocking-b2g: - → fugu+
Flags: needinfo?(tzhuang)
Flags: needinfo?(ehung)
Why is this a fugu specific bug?
Flags: needinfo?(styang)
After discussed with our partner, it will not block fugu.
blocking-b2g: fugu+ → ---
Flags: needinfo?(styang)
Flags: needinfo?(ehung)
blocking-b2g: --- → 1.3?
Unfortunately this needs all the work in bug 890440 that is in 1.4 only. Seems both difficult and risky to uplift all this to 1.3. But we should have this in 1.4 (at last!).
Not a regression, so this is not a blocker.
blocking-b2g: 1.3? → -
Looking at this right now. My plan is:

 1. Switch to Notification instead of mozNotification ;
 2. Make use of Notification.get() when we enter in a thread ;
 3. Close the notification that matches the thread.
Depends on: 951150
This is a first pull request. It lacks tests for checking that notifications are closed upon reading of the thread.
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
Comment on attachment 8359816 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15307

I left a few comments/questions on the PR. Please r=? me when ready
Dears, nominate to 1.3 because it confuses end user.
blocking-b2g: - → 1.3?
(In reply to Jack Liu from comment #79)
> Dears, nominate to 1.3 because it confuses end user.

We understand that, but we can't hold a release commonly on issues that we've already shipped with, unless there's special reason on why we shipped with this in 1.1, but shouldn't ship with it in 1.3.
blocking-b2g: 1.3? → -
(In reply to Jason Smith [:jsmith] from comment #80)
> (In reply to Jack Liu from comment #79)
> > Dears, nominate to 1.3 because it confuses end user.
> 
> We understand that, but we can't hold a release commonly on issues that
> we've already shipped with, unless there's special reason on why we shipped
> with this in 1.1, but shouldn't ship with it in 1.3.

And this depends on other bugs that are not 1.3+ and that I'm not confident to backport at this time.
We need some help for the UX side. What should be the behavior when we receive multiple SMS for one thread ?

The way it is currently implemented, we will only update the notification with the latest SMS we receive.
Flags: needinfo?(aymanmaat)
Blocks: 960081
Blocks: 938540
(In reply to Alexandre LISSY :gerard-majax from comment #82)
> We need some help for the UX side. What should be the behavior when we
> receive multiple SMS for one thread ?
> 
> The way it is currently implemented, we will only update the notification
> with the latest SMS we receive.

For the sake of space and clarity of communication (that there is more than one new message associated to a single thread) i would be inclined to trade out the preview message text that is associated to the new message in the notification tray with ‘’n’ unread messages’, where ’n’ is the number of new messages associated to that thread.

But i don't think this is a blocker because currently the user is notified in the notification tray that there is a new message (which is the most important thing) and will discover the other new messages in the thread when they get to the thread. Therefore number of new messages associated to a single thread in the notification tray is a nice to have.
Flags: needinfo?(aymanmaat)
Thanks Ayman. So if everyone is okay with this, then I propose that the present bug only does migration to the new API as it currently does, and that we open a follow up for 1.4 that we do nice things with those notifications.
Blocks: 960092
Comment on attachment 8359816 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15307

Travis is green at https://travis-ci.org/mozilla-b2g/gaia/builds/17001317

This PR converts test to the new API and also adds two test cases:
 - checking that when we switch to a new thread, the Notification.get() is called with the correct tag to filter
 - checking that when we switch to a new thread, we call the close() method of the notification object

As far as I can say those are the only new tests we need.
Attachment #8359816 - Flags: review?(waldron.rick)
Comment on attachment 8359816 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15307

I deleted my old obsolete comments, but several new comments. Please r? me when ready
Attachment #8359816 - Flags: review?(waldron.rick) → review-
Comment on attachment 8359816 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15307

Thanks for the comment. All of your previous remarks should be addressed, I agree the code is nicer this way :)
Attachment #8359816 - Flags: review- → review?(waldron.rick)
Comment on attachment 8359816 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/15307

Just the last two things: 

1. jshint fixes
2. Update `MockNotification` => `Notification`

and then this is r+ me and you can go ahead and land it—Thanks!!
Attachment #8359816 - Flags: review?(waldron.rick) → review+
Hi Jason,

Per comment#88, seems it can be fixed in v1.3, sorry but I have to nominate again.
blocking-b2g: - → 1.3?
Jack: no it can't, see comment 74. Sorry (I mean it).
blocking-b2g: 1.3? → ---
(In reply to Rick Waldron [:rwaldron] from comment #88)
> Comment on attachment 8359816 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/15307
> 
> Just the last two things: 
> 
> 1. jshint fixes
> 2. Update `MockNotification` => `Notification`
> 
> and then this is r+ me and you can go ahead and land it—Thanks!!

Done. Travis running at https://travis-ci.org/mozilla-b2g/gaia/builds/17049234
https://github.com/mozilla-b2g/gaia/commit/4efcdad4637d6af1654dcd441409b187a00b81ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This is so good, thanks !
Flags: needinfo?(tzhuang)
Depends on: 981401
Depends on: 983631
Depends on: 994823
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: