Closed Bug 787064 Opened 13 years ago Closed 13 years ago

System Message API: add broadcastMessage()

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file, 1 obsolete file)

Used for broadcasting a specific message to registered Apps in the situation that pageURI and manifestURI are not available for the systemmessage service caller.
Blocks: 782488
Attached patch Patch: add broadcastMessage (obsolete) — Splinter Review
add broadcastMessage() for the situation that pageURI and manifestURI are not available.
Comment on attachment 657001 [details] [diff] [review] Patch: add broadcastMessage add internal _sendMessage()
Attachment #657001 - Flags: review?(fabrice)
Comment on attachment 657001 [details] [diff] [review] Patch: add broadcastMessage Review of attachment 657001 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/SystemMessageInternal.js @@ +46,5 @@ > > + broadcastMessage: function broadcastMessage(aType, aMessage) { > + this._pages.forEach(function(aPage) { > + if (aPage.type == aType) { > + this._sendMessage(aType, aMessage, aPage.uri, aPage.manifest); Why don't you call directly this.sendMessage(...) ? ::: dom/messages/interfaces/nsISystemMessagesInternal.idl @@ +21,5 @@ > */ > void sendMessage(in DOMString type, in jsval message, in nsIURI pageURI, in nsIURI manifestURI); > > /* > + * Allow any internal user to braodcast a message of a given type. Nit: s/braodcast/broadcast @@ +22,5 @@ > void sendMessage(in DOMString type, in jsval message, in nsIURI pageURI, in nsIURI manifestURI); > > /* > + * Allow any internal user to braodcast a message of a given type. > + * Webapp which registers the message will be launched. * The application that registers ...
Attachment #657001 - Flags: review?(fabrice)
Attachment #657001 - Flags: review+
Attachment #657001 - Attachment is obsolete: true
Comment on attachment 657088 [details] [diff] [review] Patch (v2): add broadcastMessage. r=fabrice Review of attachment 657088 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/SystemMessageInternal.js @@ +104,5 @@ > + _sendMessage: function _sendMessage(aType, aMessage, aPageURI, aManifestURI) { > + debug("Broadcasting " + aType + " " + JSON.stringify(aMessage)); > + ppmm.broadcastAsyncMessage("SystemMessageManager:Message" , { type: aType, > + msg: aMessage, > + manifest: aManifestURI }); Do we have a bug on file to replace this broadcasting with specific sendAsyncMessage call(s)? If not, we should!
(In reply to Philipp von Weitershausen [:philikon] from comment #5) > Comment on attachment 657088 [details] [diff] [review] > Patch (v2): add broadcastMessage. r=fabrice > > Review of attachment 657088 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/messages/SystemMessageInternal.js > @@ +104,5 @@ > > + _sendMessage: function _sendMessage(aType, aMessage, aPageURI, aManifestURI) { > > + debug("Broadcasting " + aType + " " + JSON.stringify(aMessage)); > > + ppmm.broadcastAsyncMessage("SystemMessageManager:Message" , { type: aType, > > + msg: aMessage, > > + manifest: aManifestURI }); > > Do we have a bug on file to replace this broadcasting with specific > sendAsyncMessage call(s)? If not, we should! Philipp, I think Bug 777206 is what you are looking for, right?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > I think Bug 777206 is what you are looking for, right? Correcto. Thanks.
Target Milestone: --- → mozilla17
Target Milestone: mozilla17 → mozilla18
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 782489
QA Contact: jsmith
Whiteboard: [qa+]
Comment on attachment 657088 [details] [diff] [review] Patch (v2): add broadcastMessage. r=fabrice Review of attachment 657088 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/SystemMessageInternal.js @@ +46,5 @@ > > + broadcastMessage: function broadcastMessage(aType, aMessage) { > + this._pages.forEach(function(aPage) { > + if (aPage.type == aType) { > + this._sendMessage(aType, aMessage, aPage.uri, aPage.manifest); Just a comment that this logic here will result in O(n^2) complexity because this._sendMessage() itself will also loop the _pages, which is kind of redundant. But the result should be fine. ;)
Attachment #657088 - Flags: feedback-
Gene, thanks for pointing this out. I will file a bug to improve this.
Depends on: 789005
(In reply to Gene Lian [:gene] from comment #10) > Comment on attachment 657088 [details] [diff] [review] > Patch (v2): add broadcastMessage. r=fabrice > > Review of attachment 657088 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/messages/SystemMessageInternal.js > @@ +46,5 @@ > > > > + broadcastMessage: function broadcastMessage(aType, aMessage) { > > + this._pages.forEach(function(aPage) { > > + if (aPage.type == aType) { > > + this._sendMessage(aType, aMessage, aPage.uri, aPage.manifest); > > Just a comment that this logic here will result in O(n^2) complexity because > this._sendMessage() itself will also loop the _pages, which is kind of > redundant. But the result should be fine. ;) Wait! I'm afraid this could also be a bug. Supposing two pages registered the same type, wouldn't it eventually broadcast totally 4 messages? (sent from mobile and sorry for any wrong typo)
Ah! It wouldn't. Never mind. But complexity is still an issue. ;)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: