Closed
Bug 787064
Opened 13 years ago
Closed 13 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•13 years ago
|
||
add broadcastMessage() for the situation that pageURI and manifestURI are not available.
Assignee | ||
Comment 2•13 years ago
|
||
Comment on attachment 657001 [details] [diff] [review]
Patch: add broadcastMessage
add internal _sendMessage()
Attachment #657001 -
Flags: review?(fabrice)
Comment 3•13 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•13 years ago
|
Attachment #657001 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #657001 -
Attachment is obsolete: true
Comment 5•13 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•13 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•13 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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla17
Assignee | ||
Updated•13 years ago
|
Target Milestone: mozilla17 → mozilla18
Comment 9•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Blocks: system-message-api
Updated•13 years ago
|
QA Contact: jsmith
Whiteboard: [qa+]
Comment 10•13 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•13 years ago
|
||
Gene, thanks for pointing this out. I will file a bug to improve this.
Comment 12•13 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•13 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
•