735947 - support the desktop notifications API in b2g

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: wip

We can add support for navigator.mozNotification by implementing the nsIAlertsService and nsIContentPermissionPrompt components.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: test html file

test file.

So, currently events from chrome -> content works fine, but I'm not catching events from content -> chrome

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: patch

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: test html file

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 606292 [details] [diff] [review]
patch

>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
>         this.sendEvent(window, 'ContentStart');
>+        AlertsHelper.init();

I would rather prefer AlertsHelper to subscribe to the ContentStart method in AlertsHelper.init and move this call to shell.start.

>@@ -420,3 +422,36 @@ Services.obs.addObserver(function onCons
>   serverSocket.asyncListen(listener);
> })();
> 
>+AlertsHelper = {
>+  _listeners: {},
>+  _count: 0,
>+
>+  init: function alert_init() {
>+    content.addEventListener("desktop-notification-click", this, false, true);
>+    content.addEventListener("desktop-notification-close", this, false, true);

Similarly with the mozApps patch. I'm worried about relying on custom events here.

>+  },
>+
>+  handleEvent: function alert_handleEvent(e) {
nit: i think the rest of the code use |evt| instead of |e|

>+    if (!e.detail || !e.detail.id)
>+      return;
>+    let listener = this._listeners[e.detail.id];

let detail = evt.detail;
if (!detail || !detail.id)
  return;

let listener = this._listeners[detail.id];


>+    let topic = e.type == "desktop-notification-click" ? "alertclickcallback" : "alertfinished";

nit: the rest of the file use ' istead of "

>+    listener.observer.observe(null, topic, listener.cookie);
>+
>+    // we're done with this notification
>+    if (topic === "alertfinished")
>+      delete this._listeners[e.detail.id];

nit: seems like you should store id at some point

>+  },
>+
>+  registerListener: function alert_registerListener(aCookie, aAlertListener) {

nit: the rest of the code do not use the 'a' prefix

>+    let id = "alert" + this._count++;
>+    this._listeners[id] = { observer: aAlertListener, cookie: aCookie };
>+    return id;
>+  },
>+
>+  showAlertNotification: function alert_showAlertNotification(aImageUrl, aTitle, aText, aTextClickable, 
>+                                                              aCookie, aAlertListener, aName) {
>+    let id = this.registerListener(aCookie, aAlertListener);
>+    shell.sendEvent(content, "desktop-notification", { id: id, icon: aImageUrl, title: aTitle, text: aText } );

As above I'm worried about those 'proprietary' events.

>+  }
>+}
>diff --git a/b2g/components/AlertsService.js b/b2g/components/AlertsService.js
>+AlertsService.prototype = {
>+  classID: Components.ID("{5dce03b2-8faa-4b6e-9242-6ddb0411750c}"),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAlertsService]),
>+
>+  showAlertNotification: function(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener, aName) {
>+  	let browser = Services.wm.getMostRecentWindow("navigator:browser");
>+    browser.AlertsHelper.showAlertNotification(aImageUrl, aTitle, aText, aTextClickable, aCookie, aAlertListener, aName);

nit: the indentation seems broken here

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We don't have a spec defining how content should handle notifications send by the DOM notifications API.  We have to define it.

I'm not thrilled with the approach here, but I think it works for a v0.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: patch v2

updated according to IRC discussion: 
We send a single mozChromeEvent form chrome to content, and expect mozContentEvent from content to chrome.

Sorry I could not switch to single quote strings ;)

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 606631 [details] [diff] [review]
patch v2

>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
>+    // we're done with this notification
>+    if (topic === "alertfinished")
>+      delete this._listeners[detail.id];
>+  },
>+
>+  registerListener: function alert_registerListener(aCookie, aAlertListener) {

nit: aCookie -> cookie, aAlertListener -> alertListener

>+    let id = "alert" + this._count++;
>+    this._listeners[id] = { observer: aAlertListener, cookie: aCookie };
>+    return id;
>+  },
>+
>+  showAlertNotification: function alert_showAlertNotification(aImageUrl, aTitle, aText, aTextClickable, 
>+                                                              aCookie, aAlertListener, aName) {
nit: remove the 'a' prefix

>+    let id = this.registerListener(aCookie, aAlertListener);
>+    shell.sendEvent(content, "mozChromeEvent", { type: "desktop-notification", id: id, icon: aImageUrl, 
>+                                                 title: aTitle, text: aText } );
>+  }
>+}

Per IRC I'm not a fan of the custom events mechanism but until we figure out something cleaner let's fo forward and starts the discussion in dev-webapi. 

r+ with the relevant thread in dev-webapi :)

Comment 8

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e239861bd56e

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

\o/

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e239861bd56e