Closed Bug 735947 Opened 12 years ago Closed 12 years ago

support the desktop notifications API in b2g

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: fabrice, Assigned: fabrice)

Details

Attachments

(2 files, 3 obsolete files)

Attached patch wip (obsolete) — Splinter Review
We can add support for navigator.mozNotification by implementing the nsIAlertsService and nsIContentPermissionPrompt components.
Attachment #606043 - Flags: feedback?(21)
OS: Linux → Gonk
Hardware: x86_64 → All
Attached file test html file (obsolete) —
test file.

So, currently events from chrome -> content works fine, but I'm not catching events from content -> chrome
Assignee: nobody → fabrice
Attached patch patch (obsolete) — Splinter Review
Attachment #606043 - Attachment is obsolete: true
Attachment #606043 - Flags: feedback?(21)
Attachment #606292 - Flags: review?(21)
Attached file test html file
Attachment #606044 - Attachment is obsolete: true
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
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.
Attached patch patch v2Splinter Review
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 ;)
Attachment #606292 - Attachment is obsolete: true
Attachment #606292 - Flags: review?(21)
Attachment #606631 - Flags: review?(21)
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 :)
Attachment #606631 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/e239861bd56e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.