948334 - Convert Email to new Notification API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[:gerard-majax]

While investigating the email app in context of bug 938541, I came accross this:
$ grep -R 'NotificationHelper' apps/email/
apps/email/shared/test/unit/mocks/mock_notification_helper.js:var MockNotificationHelper = {
apps/email/shared/js/notification_helper.js:var NotificationHelper = {
apps/email/test/config.js:        exports: 'NotificationHelper'
apps/email/js/mail_app.js:        exports: 'NotificationHelper'
$

It seems that NotificationHelper has been used in the past, but now the app is using the Notification API, not mozNotification anymore. So we have a useless reference to this helper: https://github.com/mozilla-b2g/gaia/commit/93e3fa9b7907429fe1dbcef513526a192bf6bfeb removes the usage of NotificationHelper.

This commit also contains a buggy/stale reference to navigator.mozNotification, which should be replaced by window.mozNotification.

Comment 1

[:gerard-majax]

Looks like I've been a bit too quick. There is an obvious usage of NotificationHelper at https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/sync.js#L9.

Comment 2

[:gerard-majax]

So we want to make use of the new API for this part in fact :)

Comment 3

[:gerard-majax]

apps/email/js/sync.js:
  - getting icon URI

Comment 4

[:gerard-majax]

Again, too fast.

apps/email/js/sync.js:
  - getting icon URI
  - encoding object as string parameter of the icon (message_list, account id)

Comment 5

[:gerard-majax]

I think email might be semi broken right now, with the landing of bug 890440: we do not remove automatically notifications sent via the new API. But I suspect email was relying on this and not closing its notifications.

Comment 6

[James Burke [:jrburke]]

Indeed, email does not clear notifications on its own, because it was relying on the old behavior. However, that will change when bug 915643 and bug 922722 land (they should land together).

However bug 930794 blocks those changes from landing, so it would be good to have bug 930794 fixed for us to close out the loop. I have asked in that bug if we can get that in place for 1.4.

Comment 7

[:gerard-majax]

Someone from Email should do the migration. This is pretty simple, and there are already examples of it in Gaia.

Comment 8

[James Burke [:jrburke]]

Email just uses notification_helper for the getIconURI function, but uses `new Notification()` to create new notifications. If there is a suggested replacement for the getIconURI function, I can switch to that. Otherwise, I do not think there is anything more to do in the email app for this and we could close this bug.

Comment 9

[:gerard-majax]

(In reply to James Burke [:jrburke] from comment #8)
> Email just uses notification_helper for the getIconURI function, but uses
> `new Notification()` to create new notifications. If there is a suggested
> replacement for the getIconURI function, I can switch to that. Otherwise, I
> do not think there is anything more to do in the email app for this and we
> could close this bug.

Thanks, I was doing some cleanup in my bugs, so I think we can close this one then :)