938541 - Convert NotificationHelper to new Notification API

Status: RESOLVED DUPLICATE of bug 1095109

(Firefox OS Graveyard :: Gaia, defect)

Description

[:gerard-majax]

We need to update the NotificationHelper and make it to use the new Notification API.

Comment 1

[Julien Wajsberg [:julienw]]

I don't think we should do this, or at least not like you think.

If you simply change the helper to make it send notifications using the new API, it will make all apps magically use the new API, and I think they're not ready to do it. When you use the new API you also need some code to remove/replace the notification.

There are 2 different ways doing this actually:

1. apps that currently use the NotificationHelper should be converted to use the new Notification API without using the helper, and in the end we should remove it.

2. make a real NotificationHelper that would handle in a graceful way the remove/replace operations with a nice API. That could also handle how we retrieve existing notifications when the app is launched.

We could have a look at how it's done in the Email app. Flagging James so that he can give us pointers and advices, as Gaia's only user of this API ;)

Comment 2

[James Burke [:jrburke]]

Email creates new Notification objects in this file:

https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/sync.js

We still use NotificationHelper, but only to get the iconURL. So some helper for that is probably still useful going forward.

We will be using Notification.get() but I am still working on that in a branch, not finished yet. I can see it would be useful to have a helper that is just "getExistingNotificationByTag" (although something with a shorter name), which is what I will be doing for the Notification.get() work, to either update its contents or to close it.

Although, other apps may want different filters on the Notification.get() API. It is probably not too much for the app to Array.filter() got get the ones of interest. For email, we use the "tag" property, and the email account ID is the tag value.

We only bind .onclick on the new Notification, and only because the mozSetMessageHandler listener is not called if the app is running in the background. I would have preferred to get all notifications via the mozSetMessageHandler, but that is probably a bigger standards issue. Email does not care about onclose.

Comment 3

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> I don't think we should do this, or at least not like you think.
> 
> If you simply change the helper to make it send notifications using the new
> API, it will make all apps magically use the new API, and I think they're
> not ready to do it. When you use the new API you also need some code to
> remove/replace the notification.
> 
> There are 2 different ways doing this actually:
> 
> 1. apps that currently use the NotificationHelper should be converted to use
> the new Notification API without using the helper, and in the end we should
> remove it.
> 
> 2. make a real NotificationHelper that would handle in a graceful way the
> remove/replace operations with a nice API. That could also handle how we
> retrieve existing notifications when the app is launched.
> 
> We could have a look at how it's done in the Email app. Flagging James so
> that he can give us pointers and advices, as Gaia's only user of this API ;)

Thanks, I suspected there would be some app-side changes to perform, and I was thinking of the first proposition.

Comment 4

[:gerard-majax]

Current files that are referencing 'NotificationHelper':
 - apps/calendar/js/notification.js
 - apps/calendar/test/unit/notification_test.js
 - apps/communications/dialer/js/dialer.js
 - apps/communications/facebook/js/fb_sync_init.js
 - apps/costcontrol/js/message_handler.js
 - apps/email/js/mail_app.js
 - apps/email/test/config.js
 - apps/sms/js/activity_handler.js
 - apps/sms/test/unit/activity_handler_test.js
 - apps/system/js/battery_manager.js
 - apps/system/js/bluetooth_transfer.js
 - apps/system/js/icc_worker.js
 - apps/system/test/unit/bluetooth_transfer_test.js
 - apps/wappush/js/wappush.js
 - apps/wappush/test/unit/wappush_test.js

Comment 5

[: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:
 1: remove the hacks in system app
 2: change notification helper to use the new API
 3: 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
 4: move all apps to the new simple interface (and this should be trivial)
 5: convert each app after to make use of the new API

Comment 6

[:gerard-majax]

I should more or less have a correct solution for now for steps 1 and 2, partly 3.

Comment 7

[:gerard-majax]

We can't really do steps 1 in fact: we want to keep this for compatibility with old apps, at least until we completely remove mozNotification.

So I reworked my PR and have it this way:
 - detect in system app (based on id as documented in bug 890440 comment 26) whether it's from old or new API
 - if we have an old API user, then we keep the workarounds for now
 - if it's the new API, we do not do anything

I also resurected the old send() method of NotificationHelper to make it use the old API. I'm going to start proceeding with step 4: opening bugs for all apps, and migrate then on the new API.

We will at the same time document and maybe reconsider the existence/use of Notification Helper once the new API is getting used everywhere.

Comment 8

[:gerard-majax]

I've opened bug on all apps, and documented each usage. Complete set is:

apps/calendar/js/notification.js:
  - sending notification reminder for events
  - event encoded in icon params as a url
  
apps/communications/dialer/js/dialer.js:
  - sending notification for missed calls
  - no payload, opens call log
  
apps/communications/facebook/js/fb_sync_init.js:
  - sending notification on facebook sync errors
  - no payload
  
apps/costcontrol/js/message_handler.js:
  - sending notification on phone usage changes (error in topup, balance threshold, data usage)
  - on click opens the correct panel
  - encoding type of error in the icon params
  
apps/email/js/sync.js:
  - getting icon URI
  
apps/sms/js/activity_handler.js:
  - getting icon URI
  - message class 0 encoded in icon URL
  - message parameters (thread id, number, id) encoded in icon url
  - sending notification on new sms/mms
  
apps/system/js/battery_manager.js:
  - sending notification of power saving mode
  
apps/system/js/bluetooth_transfer.js:
  - sending notification of bluetooth transfers
    - started
    - confirmation
    - sent finished successfully
    - receive finished successfully
    - sent finished with error
    - receive finished with error
    - multiple transfers report
  
apps/system/js/icc_worker.js:
  - sending notification from STK CMD_SET_UP_IDLE_MODE_TEXT
  
apps/wappush/js/wappush.js:
  - sending wappush notification
  - message timestamp encoded in URL

For each app, I propose to have a deeper look at the usage and try to assess the amount of work needed for migration.

Comment 9

[:gerard-majax]

A couple of applications are hacking the icon URI to encode some data. There is no way to attach an opaque pointer/structure to the new Notification API, so one proposal would be to augment the API of NotificationHelper to ease this and avoid relying on this hackish behavior. Some users of this could make use of the tags available.

Some of the usages of getIconURI are legits though.

Comment 10

[:gerard-majax]

So Dialer seems to make a simple usage of it. I'm currently testing and getting feedback on the code it makes us to write with julienw and etienne_s.

Comment 11

[:gerard-majax]

The new notification API does not allow us to easily put data payload into the object. This is not used in all apps, but it can be a justification to keep and augment the NotificationHelper that could be able to attach data to notifications.

This is okay in some case but not in all cases: if the app is not running then the notification is triggered as a system message, and its payload is limited and defined at https://hg.mozilla.org/mozilla-central/file/9e03cd21db08/b2g/chrome/content/shell.js#l780

This means that the object received by a message handler on notification will only contain the following properties: clicked, title, body, imageURL. The best way to associate a data payload with a notification is to rely its ID. We don't have it in the case of the system message, but we could add it.

Comment 12

[Julien Wajsberg [:julienw]]

Nope, the best way to associate a data payload is currently through the icon URI ;)

Comment 13

[:gerard-majax]

How do you feel about using NotificationHelper to store locally the payload (through asyncStorage maybe?) and the need to augment the system messages sent from B2G shell.js to add the notification's id to the object we receive ?

Comment 14

[James Burke [:jrburke]]

Some spec-related discussion for possible input:

I gave feedback on whatwg about a few notifications-related things here:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-September/040900.html

and for the data storage, Jonas suggested using a structured clone thing, which ended up with Anne filing this ticket to track that:

https://github.com/whatwg/notifications/issues/3

So it may be worth commenting on that ticket providing feedback, or to work out a way to provide a "data" property on the notification object, using that ticket.

But in short, if the system can make it appear that there is a "data" object on the notification object when given to the app, I would be all for it, and it would help tie into improving the standard. For my purposes, I was fine with the limitation being a JSON-serializable structure, but see the thread for Jonas' comments about structured clone, or just start commenting in that notifications ticket perhaps.

Comment 15

[:gerard-majax]

This is what julienw and I discussed past week, and this really sounds like the best idea. I was giving a look to have a quick and not too dirty solution, since I have no idea on the timeframe we have for this kind of changes of the specification.

But definitively, you answered some question I was gonna be searching soon, thanks for this input !

Comment 16

[:gerard-majax]

This has been documented in bug 899585.

Comment 17

[:gerard-majax]

communications/dialer and communications/facebook are the two least users: only sending a notification, no specific payload attached.

Comment 18

[:gerard-majax]

For apps with basic usage of Notification, like dialer and facebook sync, there is a slight usage that might make an updated version of NotificationHelper.send() useful. However, the added value is not very clear and I fear we might end up with boilerplate complexity in this helper rather than simplifying the app code. For more advanced usages, I'm not convinced it's useful.

There might be room for permission checking helper. I'm going to pursue with patching apps, starting by the easy ones.

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Alexandre LISSY :gerard-majax from comment #17)
> communications/dialer and communications/facebook are the two least users:
> only sending a notification, no specific payload attached.

But in dialer we _want_ another behavior: removing all "missed called" notifications when we enter the "call log" panel, and only then.

This is bug 926318.

Comment 20

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> (In reply to Alexandre LISSY :gerard-majax from comment #17)
> > communications/dialer and communications/facebook are the two least users:
> > only sending a notification, no specific payload attached.
> 
> But in dialer we _want_ another behavior: removing all "missed called"
> notifications when we enter the "call log" panel, and only then.
> 
> This is bug 926318.

Sure, but I'm only talking about the current usage :)

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hey guys, 

:gerard-majax made me aware that this bug exists. We found a need for a Notification wrapper for L10n purposes and we want to reuse NotificationHelper. See bug 1095109. Because it also moves NotificationHelper to use W3C Notification API, I'm marking this bug as a dupe of that one.