Closed
Bug 787064
Opened 11 years ago
Closed 11 years ago
System Message API: add broadcastMessage()
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: hsinyi, Assigned: hsinyi)
References
Details
(Whiteboard: [qa+])
Attachments
(1 file, 1 obsolete file)
5.74 KB,
patch
|
airpingu
:
feedback-
|
Details | Diff | Splinter Review |
Used for broadcasting a specific message to registered Apps in the situation that pageURI and manifestURI are not available for the systemmessage service caller.
Assignee | ||
Comment 1•11 years ago
|
||
add broadcastMessage() for the situation that pageURI and manifestURI are not available.
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 657001 [details] [diff] [review] Patch: add broadcastMessage add internal _sendMessage()
Attachment #657001 -
Flags: review?(fabrice)
Comment 3•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #657001 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #657001 -
Attachment is obsolete: true
Comment 5•11 years ago
|
||
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!
Assignee | ||
Comment 6•11 years ago
|
||
(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?
Comment 7•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > I think Bug 777206 is what you are looking for, right? Correcto. Thanks.
Blocks: 787175
Assignee | ||
Comment 8•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/b0a04c791e84
Assignee | ||
Updated•11 years ago
|
Target Milestone: --- → mozilla17
Assignee | ||
Updated•11 years ago
|
Target Milestone: mozilla17 → mozilla18
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0a04c791e84
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Blocks: system-message-api
Updated•11 years ago
|
QA Contact: jsmith
Whiteboard: [qa+]
Comment 10•11 years ago
|
||
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-
Assignee | ||
Comment 11•11 years ago
|
||
Gene, thanks for pointing this out. I will file a bug to improve this.
Comment 12•11 years ago
|
||
(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)
Comment 13•11 years ago
|
||
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.
Description
•