Closed Bug 787064 Opened 9 years ago Closed 9 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.
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
https://hg.mozilla.org/mozilla-central/rev/b0a04c791e84
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.