890440 - [System] Remove the System workarounds that removed all notifications when an app is launched or displayed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Leo]

1. Title:  The handset is clearing all the notifications when click on one notification
2. Precondition: Have several long SMS/MMS threads. SMS delivery reports ON.
3. Tester's Action: 1. Receive some messages
                    2. Check notifications bar
                    3. Select one message notification
                    4.Check the notifications bar again
4. Detailed Symptom (ENG.) : The handset is clearing all the notifications
5. Expected: The handset should clear only the notification previously checked
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on v1-train
8. Gaia Revision:  613e69ee0a009399130ad2cbb8b9d0462cd1fc70
9. Personal email id: [e-mail]

Comment 1

[hanj.kim25]

Some investigation.

Precondition: have 2 sms messages unread on notification.

When 1st message is tapped, the tab function is called.

tap: function ns_tap(notificationNode) {
  window.dispatchEvent(event);
  ...
  this.removeNotification(notificationNode.dataset.notificationID, false);

This launches the sms app and removes the 1st notification. 

Then when app is launched, the 2nd notification is removed by 'appopen' event.

window.addEventListener('appopen', this.handleAppopen.bind(this));

handleAppopen: function ns_handleAppopen(evt) {
  var manifestURL = evt.detail.manifestURL,
      selector = '[data-manifest-u-r-l="' + manifestURL + '"]';


To Alive:
Would there be an easy fix for this?
I'm not sure how you'll like having 'if app == sms, do nothing' in handleAppopen.

Comment 2

[Borja Salguero [:borjasalguero]]

Julien could you take a look as well? Thanks!

Comment 3

[Julien Wajsberg [:julienw]]

This is done on purpose in 1.1, sorry we won't change this.

Comment 4

[Wayne Chang [:wchang]]

If "all the notifications" that are cleared are from the SMS app, then i believe this is not a bug but a current behaviour. As the SMS app will clear all its related notifications.

If notifications from other apps are also cleared, then this would be a bug, please renom leo? if this is the case.

Comment 5

[Julien Wajsberg [:julienw]]

To be clear (once again), since we don't have access to the "new" Notification API that is implemented in mozilla central, but not in b2g18, there are some use cases that we can't do. In particular we can't precisely control the notifications from the app. So we made a number of simplifications and this is what we're doing in 1.1:

* when an app is displayed, either:
  + launched directly
  + launched from a notification
  + is fullscreen when coming out of lockscreen
 we remove all the notifications that this app has sent.

Note: I'm not sure if the app is launched with an activity; I think we're removing the notification too in that case.

This is not perfect, but I think this is the best compromise we can do with the current tools we have.

If you want more, we need to uplift bug 782211 and do some work in system and the gecko glue to properly support the new notification mechanism and do work in the Sms app. I don't think we want to do this now.

Thus renominating. Please put koi? if it's not leo+ again, as I definitely want to improve this in the next version.

Comment 6

[Wayne Chang [:wchang]]

This is now koi? as agreed by leo partners.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Clear ni? since unnecessary now.

Comment 9

[Julien Wajsberg [:julienw]]

Bugmorphing this to a System bug.

We implemented workarounds in System and Gecko to remove all notifications for an app when that app :
* is launched
* is displayed
* is toplevel when we go out of the lockscreen

This was bug 824549 and bug 868816.

Before removing the workarounds in the system, all apps that uses notifications will need to be changed to handle their own notifications if necessary.

SMS counterpart will be done in bug 855165, I don't know if other bugs exist for other apps already.

Comment 10

[Jason Smith [:jsmith]]

Not blocking - not critical to 1.2 feature set.

Comment 11

[Caio Lima(:caiolima)]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Bugmorphing this to a System bug.
> 
> We implemented workarounds in System and Gecko to remove all notifications
> for an app when that app :
> * is launched
> * is displayed
> * is toplevel when we go out of the lockscreen
> 
> This was bug 824549 and bug 868816.
> 
> Before removing the workarounds in the system, all apps that uses
> notifications will need to be changed to handle their own notifications if
> necessary.
> 
> SMS counterpart will be done in bug 855165, I don't know if other bugs exist
> for other apps already.

I don't know if it is the current behavior, but I think that the System should trigger a 'applaunch' event. Is it the current behavior?

Comment 12

[Julien Wajsberg [:julienw]]

Caio, I don't understand what you want to say here :)

You should have a look to the patches of the two bugs.

Comment 14

[:gerard-majax]

Okay so

Comment 15

[:gerard-majax]

As far as I could understand, both bug 929895 (koi+) and bug 926318 (v1.3+) are manifestations of the workaround that this bugs aims at removing.

Comment 16

[Jason Smith [:jsmith]]

Not a blocker, but feel free to keep working on this.

Comment 17

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

this blocks a 1.3+, 1.3?

Comment 18

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #17)
> this blocks a 1.3+, 1.3?

This is not a blocker - we've discussed this multiple times in triage that this is a feature we should definitely work on, but won't block 1.3. The solution that should be done to resolve the blocking need on bug 926318 should involve blacklisting the dialer app, as that's the current workaround we're suggesting for any blocking bugs related to this problem. This is on product's backlog for a future release, so we will consider this feature at some point.

Comment 19

[Julien Wajsberg [:julienw]]

The more I think of it, the more I think we should not remove tapped notifications for new-style notifications. Only the Notification API clients should remove their notifications, the System app should not touch them.

Comment 20

[:gerard-majax]

After looking the code everywhere and thinking about it for two days, and discussing with julienw about this, 

I think the plan should be:
 - remove the hacks in system app
 - change notification helper to use the new API
 - implement a "simple" interface in notification helper that does the same minimum as in the old API and mimic the behavior: taping removes the notif from the helper directly
 - move all apps to the new simple interface (and this should be trivial)
 - convert each app after to make use of the new API

Comment 21

[:gerard-majax]

Sorry for the noise.

Comment 22

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

this is blocking bug 926318, 1.3?

Comment 23

[Julien Wajsberg [:julienw]]

I'd be happier with removing 1.3+ on bug 926318.

I know we moved this a lot already, but now Alexandre is _really_ working on it these days, so I'm confident it will be implemented correctly for 1.4.

Comment 24

[Anthony Ricaud (:rik)]

Yeah, this is definitely not a blocker for 1.3.

Comment 25

[:gerard-majax]

We definitively can't remove the hacks in system app 'like this'. Until the API is not unsable anymore, and after discussing with julienw, we should keep the old behavior for apps that have the old API. We should be able to distinguish the API from Gecko events.

As far as I've checked for now, the 'id' is generated differently depending on the API, so we could use this.

Comment 26

[:gerard-majax]

id field of the mozChromeEvent in the system app for a notification sent through the new API: "id":"app://uitest.gaiamobile.org/manifest.webapp#notag:{45a6fffc-da7b-440e-8cf6-088fc4645cb6}"

and in the case of the old API:
"id":"app-notif-{490fdc61-89dc-47c2-8701-4086aabcd819}"

This is being set by gecko at https://hg.mozilla.org/mozilla-central/file/9f12a9fab080/b2g/components/AlertsService.js#l70

Comment 27

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/14719

Please find attached a PR that detects whether a notification was sent through the old API or through the new one, and disable the hacks in the case of the new API. This is needed for bug 910999.

Comment 28

[:gerard-majax]

I'm waiting green travis at https://travis-ci.org/mozilla-b2g/gaia/builds/15587037

Comment 29

[:gerard-majax]

Looks like it got stalled :(, triggering a new one.

Comment 30

[:gerard-majax]

https://travis-ci.org/mozilla-b2g/gaia/builds/15588831

Comment 31

[:gerard-majax]

Travis is green now.

Comment 32

[:gerard-majax]

https://github.com/lissyx/gaia/commit/b92445c187396445aed6a657bf9e037d988bf5d6

I'm not marking this FIXED yet, the workaround is still in place until we remove and do not support mozNotification anymore.

Comment 33

[Julien Wajsberg [:julienw]]

I think you should resolve/fix it and file a new bug for mozNotification (and make it depends on another bug to drop support for mozNotification in B2G).

Comment 34

[Julien Wajsberg [:julienw]]

(I doubt this will happen soon ;) )

Comment 35

[:gerard-majax]

Followup bug 952454 is about ditching those workarounds and API version detection, bug 952453 is about total diappearance of mozNotification API.

Comment 36

[:gerard-majax]

Comment on attachment 8348187 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14719

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: improper notification removal: notifications sent with the new API are expected to be programmatically closed, not closed when tapped by user
[Testing completed]: this has been in gaia for a couple of weeks, working on all devices
[Risk to taking this patch] (and alternatives if risky): low, we are only impacting applications using the new Notification API
[String changes made]: none

Comment 37

[Jason Smith [:jsmith]]

Comment on attachment 8348187 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/14719

a- - see the blocking bug for more details.

Comment 38

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR, 1.3 uplift

This is a 1.3 uplift PR.

Comment 39

[Gregor Wagner [:gwagner]]

Comment on attachment 8390757 [details] [review]
Github PR, 1.3 uplift

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): verification problem in bug 910999
[User impact] if declined: Notification API for voicemail doesn't work. (cert blocker)
[Testing completed]: on trunk for a while and locally
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 40

[Jason Smith [:jsmith]]

Too risky for 1.3 - we've already found blocking regressions from this. We need a contextual solution here.

Comment 41

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #40)
> Too risky for 1.3 - we've already found blocking regressions from this. We
> need a contextual solution here.

Apparently the regression concern I had wasn't right. According to Gregor, we've got no alternative here, so I'm resetting the blocking flag.

Comment 42

[Michael Henretty [:mikehenrty][:mhenretty]]

v1.3: https://github.com/mozilla-b2g/gaia/commit/961a6a261d2fdb5e06e3d05e4b47a65eaf0ea910

Comment 43

[Julien Wajsberg [:julienw]]

Wow uplifting this with all other follow-ups gives me the creeps. I wonder if a hacky hack wouldn't have been possible to fix the 1.3+ bug. *crossing fingers*