1065535 - [Notifications] swiping a notification should delete it from the DB when the app is closed

Status: RESOLVED DUPLICATE of bug 1144495

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

Description

[Robert Bindar]

Steps:
1. Get a notification from the email app(could be any other app)
2. Close the app, without interacting with the notification in any way.
3. Close the notification by swipping it.
3.5 The 'close' event is gonna launch the app in the background.
3.6 When closing, the notification should be deleted from the database.
4. Reboot the phone.
5. You'll still see the notification there, because it was not deleted from the DB.

Comment 1

[Robert Bindar]

Attachment: app_closed_db.patch

We can use a similar approach for mozChromeNotifications to clean up AlertsService.js and move all these edge cases in the parent side.

Comment 2

[Robert Bindar]

The try run is green
https://tbpl.mozilla.org/?tree=Try&rev=b39a627f6e02

Comment 3

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

Comment on attachment 8488946 [details] [diff] [review]
app_closed_db.patch

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

I tested this on the Flame, and it does indeed fix the bug as mentioned. It is a little unfortunate we are coupling the alertService and NotificationDB directly like this, but I can't think of a better way to do it. r+ from me, but let's also get feedback from Alexandre.

::: b2g/components/AlertsHelper.jsm
@@ +158,5 @@
>              Services.io.newURI(listener.target, null, null),
>              Services.io.newURI(listener.manifestURL, null, null)
>            );
>          }
> +        if (detail.type === kDesktopNotificationClose) {

We should delete this notification before relaunching the app since the app may want to fetch notifications on restart, and we should make sure the DELETE gets queued before the FETCH in that case.

Also, let's add a comment here explaining why we are deleting the notification from the DB manually like this.

Comment 4

[:gerard-majax]

Comment on attachment 8488946 [details] [diff] [review]
app_closed_db.patch

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

I'm fine with the changes with the remarks being addressed, and more explanations of why.

::: b2g/components/AlertsHelper.jsm
@@ +160,5 @@
>            );
>          }
> +        if (detail.type === kDesktopNotificationClose) {
> +          let payload = { origin: listener.manifestURL, id: listener.dbId };
> +          NotificationDB.queueTask("delete", payload);

I think we should also use the cpmm/ppmm mechanisms rather than exposing the NotificationDB object. And I agree, that we need to explain why we are doing this, because as far as I remember, this should already be done by other part of the code.

::: dom/notification/NotificationDB.jsm
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +this.EXPORTED_SYMBOLS = ['NotificationDB'];

Let's try to avoid this, please :)

Comment 5

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

Let's fix this in bug 1144495, since someone is actively working on it.