Closed
Bug 735947
Opened 12 years ago
Closed 12 years ago
support the desktop notifications API in b2g
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: fabrice, Assigned: fabrice)
Details
Attachments
(2 files, 3 obsolete files)
2.57 KB,
text/html
|
Details | |
7.22 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
We can add support for navigator.mozNotification by implementing the nsIAlertsService and nsIContentPermissionPrompt components.
Attachment #606043 -
Flags: feedback?(21)
Assignee | ||
Updated•12 years ago
|
OS: Linux → Gonk
Hardware: x86_64 → All
Assignee | ||
Comment 1•12 years ago
|
||
test file. So, currently events from chrome -> content works fine, but I'm not catching events from content -> chrome
Assignee: nobody → fabrice
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #606043 -
Attachment is obsolete: true
Attachment #606043 -
Flags: feedback?(21)
Attachment #606292 -
Flags: review?(21)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #606044 -
Attachment is obsolete: true
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e239861bd56e
\o/
Comment 10•12 years ago
|
||
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.
Description
•