909932 - Implement an extended notification api accessible to addons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Federico Paolinelli]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

With bug 815202, it was proposed to implement a notification api that takes inspiration from chrome notification api: 
http://developer.chrome.com/dev/extensions/notifications.html

It should be implemented in a separate Notifications.jsm module that exports some of the new notifications features to addons.
Given that a bit of the infrastructure needed for this job was done while fixing 815202 , this bug depends on that.

Comment 1

[Federico Paolinelli]

Attachment: Proposed patch

First try. There are a few aspects I'd like to discuss over irc.

I preferred to let the client choose the notificationId because it felt more easy to use. In the downloads example it would have taken a map between every download and its notificationid, which felt a bit clunky.

In any case, the downloads.js file already uses this new api and seems to be working.

Comment 2

[Federico Paolinelli]

Attachment: bug-909932-fix

Made notify return a generated notificationid if not specified.
Moved the id from being a separate parameter to options argument.
Removed a commented legacy row in NOtificationHelper class.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 808784 [details] [diff] [review]
bug-909932-fix

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

Feedback super+.

Sorry for the delay here. I'm ashamed. This looks really good. Just a few changes in the JS and I think its ready!

::: mobile/android/chrome/content/downloads.js
@@ +198,5 @@
> +  this.title = aDownload.displayName;
> +  this.message = aDownload.percentComplete + "%";
> +  this.progress = aDownload.percentComplete;
> +  this.buttons = aButtons;
> +  this.id = Downloads.getNotificationIdFromDownload(aDownload);

This is a bit weird for us :) but we can do it. But, (pro-tip!) you might remove some of the code duplication here by calling

NotificationOptions.apply(aDownload, [aTitle, aDownload.displayName, aDownload.percentComplete + "%");

or using NotificationOptions.call if you want.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply

::: mobile/android/modules/Notifications.jsm
@@ +13,5 @@
> +function log(msg) {
> +  Services.console.logStringMessage(msg);
> +}
> +
> +var _notificationsMap = {};

I'd move this map to be a property of Notifications... but maybe its fine here. Slightly more sandboxed away :)

@@ +87,5 @@
> +      msg.cookie = this._cookie;
> +
> +    if (this._progress) {
> +      msg.progress_value = this._progress;
> +      msg.progress_max = 100;

Should we force ongoing if its a progress notification here?

@@ +134,5 @@
> +
> +  _initIdService: function() {
> +    if (!this._idService) {
> +      this._idService = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +    }

Probably better to just use a lazily created idService here. You can do it using javascript getters/setters like this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/JNI.jsm#24

@@ +148,5 @@
> +      id = idService.generateUUID().toString();
> +
> +    let notification = null;
> +    if (id in _notificationsMap) {
> +      notification = _notificationsMap[id];

Here I'd just do:

let notification = _notifs[id];
if (!notification) {
  notification = new Notification(id, options);
  _notifs[id] = notification;
}

@@ +162,5 @@
> +  clear: function notif_clear(aId) {
> +    if (!(aId in _notificationsMap))
> +      return;
> +    let notification = _notificationsMap[aId];
> +    notification.clear();

Similarly here. I'd rather not bother with the aId check and just do
if (notification)
  notification.clear();

@@ +169,5 @@
> +  observe: function notif_observe(aSubject, aTopic, aData) {
> +    let data = JSON.parse(aData);
> +    let id = data.id;
> +    if (!(id in _notificationsMap))
> +      return;

And here :) Also, it would be nice to log a message if this happens. i.e. Why are we getting messages for non-existent notifications.

Comment 4

[Federico Paolinelli]

Attachment: bug-909932-fix

Made the changes proposed (or at least, I hope I understood correctly).
I also removed the "when" parameter from optional to auto generated when a notification is created. This allow to preserve the order of notifications as discussed in bug 921776

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 816119 [details] [diff] [review]
bug-909932-fix

I have a feeling this is good, but wanted to get mfinkle in for a sr? on the api.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 816119 [details] [diff] [review]
bug-909932-fix

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

Looks really good. A few changes :)

For mfinkle, the API looks something like:
id = Notifications.notify({
  icon:    // required by Android. Must be a drawable uri,
  title:   // required by Android
  message: // string
  priority 
  buttons: [{
    name,  // Proposed we change this to id  
    title,
    icon   // Must be a drawable uri
  }],
  ongoing
  progress // int between 0 and 100
  onCloseCallback: function(id), // Proposed we change this to onCancel
  onClickCallback: function(id, cookie) // Proposed change to onClick
  cookie
}

Notifications.cancel(id);

::: mobile/android/base/NotificationHelper.java
@@ +166,5 @@
>              final String id = message.getString(ID_ATTR);
>              b.appendQueryParameter(ID_ATTR, id);
>              if (message.has(COOKIE_ATTR)) {
>                  b.appendQueryParameter(COOKIE_ATTR,
>                                         message.getString(COOKIE_ATTR));

Hmm... not this patch, but I don't think we really need these cookies here anymore. They're stored in JS and passed back to you in JS...

::: mobile/android/chrome/content/downloads.js
@@ +176,5 @@
> +    Downloads.resumeClickCallback(aId, aCookie);
> +  }
> +};
> +
> +function NotificationOptions (aDownload, aTitle, aMessage) {

Since this is contaminating the global namespace, lets rename it to DownloadNotifOptions.

::: mobile/android/modules/Notifications.jsm
@@ +10,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["Notifications"];
> +
> +function log(msg) {
> +  Services.console.logStringMessage(msg);

Comment this line out when this lands.

@@ +59,5 @@
> +    if ("progress" in aOptions && aOptions.progress != null)
> +      this._progress = aOptions.progress;
> +
> +    if ("onCloseCallback" in aOptions && aOptions.onCloseCallback != null)
> +      this._onClose = aOptions.onCloseCallback;

This is only called if the notification is canceled right? Maybe we should name it onCancel (lets drop the callback part too...)

@@ +86,5 @@
> +      msg.cookie = this._cookie;
> +
> +    if (this._progress) {
> +      msg.progress_value = this._progress;
> +      msg.progress_max = 100;

So we're forcing progress values to be between 0 and 100? Lets make sure we doc that :)

@@ +98,5 @@
> +      let buttonName;
> +      for (buttonName in this._buttons) {
> +        let button = this._buttons[buttonName];
> +        let obj = {
> +          name: button.name,

I wonder if the word "id" would be a better match to our other api pieces

Comment 7

[Federico Paolinelli]

So, from my understanding, we are suppressing the cookie parameter and we do not want to be able to set the notification id from outside. 
The changes are quite easy to implement, BUT:

- not having a "hook" parameter would result in maintaining a map between notification id and the entity it relates to, in order to understand what the callbacks are referring to (ie, which download the user wants to cancel)

- not being able to define our id would result maintaining another map, between the entity and the notification id, in order to be able to update / cancel the notification using the same id (ie, we want to update the download progress and we must use the same id we received during creation). 
Moreover, an optional id parameter is necessary even if we do not want to let the client pick a notification id, because it needs to be used whenever the client wants to update an existing notification.

Maybe the most common use case is just to have a single notification with a single id and I am fussing too much...

Comment 8

[Wesley Johnston (:wesj)]

(In reply to Federico Paolinelli from comment #7)
> - not having a "hook" parameter would result in maintaining a map between
> notification id and the entity it relates to, in order to understand what
> the callbacks are referring to (ie, which download the user wants to cancel)

I think you're saying its useful to be able to pass some data that comes back to you so that you don't have to go lookup things again (for instance, the download this notification refers to?). I'd agree with that :)

> - not being able to define our id would result maintaining another map,
> between the entity and the notification id, in order to be able to update /
> cancel the notification using the same id (ie, we want to update the
> download progress and we must use the same id we received during creation). 

I think you're talking about the 'button.id' thing here? I'm fine with not allowing users to set the notification id. Aer you saying we should do the same thing for buttons?

Comment 9

[Federico Paolinelli]

(In reply to Wesley Johnston (:wesj) from comment #8)
> 
> > - not being able to define our id would result maintaining another map,
> > between the entity and the notification id, in order to be able to update /
> > cancel the notification using the same id (ie, we want to update the
> > download progress and we must use the same id we received during creation). 
> 
> I think you're talking about the 'button.id' thing here? I'm fine with not
> allowing users to set the notification id. Aer you saying we should do the
> same thing for buttons?

I am talking about notification id. I do agree that for several reasons (incluiding avoiding id conflicts between different clients) letting the api generate the id for us is better. On the other hand, that would result in forcing the client to remember the id in some way, in order to reuse it to perform any further action on the notification, such as deletion or update.
Just think about updating the progress in the download example. We would need to map in some way the ongoing downloads to the ids. On top of that, we would need to differentiate between notification creation and update.

For the same reason, given that notify is used for both the kind of actions (create AND update), an optional id parameter is necessary. If we decide to create the id by ourselves, we can throw if the id parameter contains an unknown id.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 816119 [details] [diff] [review]
bug-909932-fix

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

Talked this over on IRC. Most of the changes are just naming, so I'll r+, but we need a new patch :)

Comment 11

[Federico Paolinelli]

Attachment: bug-909932-fix

This includes wes' suggestions. I removed the cookie parameter and changed downloads.js in order to provide download.guid as notification id.

Comment 12

[Federico Paolinelli]

There are still a couple of things that needs to be fixed:
- updating a notification does not remove unset properties, so for example if we want to remove the buttons (like when the download is finished) the buttons are still there. Easy to fix

- a synchronization issue that is happening because a notification is immediately added to the map whereas calling cancel() ends up in a message to gecko and only after the notification is removed the record is removed from the map. The effect (I have a deja vu about this) is that calling "cancel" - "show" results in removing the record from the map because cancel is handled after show. Need to figure out a way to avoid this.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 819197 [details] [diff] [review]
bug-909932-fix


>diff --git a/mobile/android/base/NotificationHelper.java b/mobile/android/base/NotificationHelper.java

>+    private static final String BUTTON_EVENT = "notif-button-clicked";
>     private static final String CLICK_EVENT = "notification-clicked";

Unless we have a string length issue, just use "notification-button-clicked"


I'm not 100% convinced about passing user given IDs into the Notification API. I'm mainly worrying about conflicts. It looks like you have both talked about it a bit though and understand the issues.

I see the problem with making extra mapping systems. The downloads.guid is a nice ID, but others might not be so easy. We don't use that pattern in many other APIs.

Why did you decide to make ID public and remove cookie?

Comment 14

[Federico Paolinelli]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 819197 [details] [diff] [review]
> bug-909932-fix
> 
> 
> 
> I'm not 100% convinced about passing user given IDs into the Notification
> API. I'm mainly worrying about conflicts. It looks like you have both talked
> about it a bit though and understand the issues.
> 
> I see the problem with making extra mapping systems. The downloads.guid is a
> nice ID, but others might not be so easy. We don't use that pattern in many
> other APIs.
> 
> Why did you decide to make ID public and remove cookie?

We have to choose between public ids and risk of conflicts and a slightly less nicer api.

With a public id:
- it is easier to map to android notification api, since it makes no difference between creating or updating a notification, you just need to pass the same id
- the client does not have to implement its own mapping
- since it is public, it can be used as a hook parameter in place of the cookie (that's why we decided to remove the cookie).

What I really liked was the fact that we were having a nearly direct mapping with android notification api and the client did not have to bother between creation and update. 

Now, given that I don't like being exposed to conflicts, I am not sure if clashing is something we absolutely need to avoid. If it is, this is not a viable solution. 
I can change the api in order to have an optional id parameter to be used only to update existing notifications. In case id is passed but it's not in the map the method will throw. An alternative solution could be splitting the notify method in "create" and "update".

Having "create" and "update" would allow the use of an external id, because we would be able to throw if an existing id is passed. 

In this scenario I think reintroducing the cookie parameter would be helpful, otherwise the client would be forced to keep a double mapping in order to understand what the callbacks are referring to.

In the meanwhile I will try to solve the cancel related problem I wrote about in comment 12.

Comment 15

[Federico Paolinelli]

I was trying to solve the cancel / notify surpassing issue, and I w'd also say that having generated ids could be a solution of that. 

The problem here is that after a sequence of 
- cancel(id)
- notify(id) 

the cancel function sends the event up to java code and the record gets removed from the map only when the "cancelled" event is sent back to js. On the other hand, if we force the id to be created from the api, we would have a sequence of
- cancel(id)
- mappedId = notify()

So the oldId would die and there would be no such issues.

I'll try to assemble a new patch to see how it looks like.

Comment 16

[Federico Paolinelli]

Attachment: bug-909932-fix

Here it is.
The notification id is always generated from inside. 
I preferred to split the notify method into two create / update. Since update requires the id I preferred to have both methods instead of letting the client pass an id to notify in order to update an existing notification.

I reintroduced the cookie, but this time it is not passed up to java (as wes suggested some days ago) and it's hold inside the notification object.

I added more logic into downloads.js, in order to maintain a map between  downloads and notifications. This is required to be able to update / cancel notifications. 

When a non ongoing notification was clicked, a cancel event was sent before the click one. As a result, the notification was removed from the map before the event could be handled. I just swapped that into NotificationHelper.java.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 819512 [details] [diff] [review]
bug-909932-fix

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

The id stuff looks fine to me.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 819512 [details] [diff] [review]
bug-909932-fix

>diff --git a/mobile/android/modules/Notifications.jsm b/mobile/android/modules/Notifications.jsm

>+Notification.prototype = {

>+    if ("progress" in aOptions && aOptions.progress != null)
>+      this._progress = aOptions.progress;
>+    else
>+      this._progree = null;

Typo: _progree -> _progress

>+    if ("cookie" in aOptions && aOptions.cookie != null)
>+      this._cookie = aOptions.cookie;
>+    else
>+      this.cookie = null;

Typo: cookie -> _cookie

r+ with typos fixed

Comment 19

[Federico Paolinelli]

Attachment: bug-909932-fix

Typos fixed

Comment 20

[Federico Paolinelli]

I think we could land this. (?)

Comment 21

[Wesley Johnston (:wesj)]

Yes.
https://hg.mozilla.org/integration/fx-team/rev/3a6adc13ebf8

We'll need to doc this. I'll try to get a page started (I have a lot of docs to do...). I put in a stub page though if you want to help :)

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/Notifications.jsm

Comment 22

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/3a6adc13ebf8