787064 - System Message API: add broadcastMessage()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Used for broadcasting a specific message to registered Apps in the situation that pageURI and manifestURI are not available for the systemmessage service caller.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch: add broadcastMessage

add broadcastMessage() for the situation that pageURI and manifestURI are not available.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 657001 [details] [diff] [review]
Patch: add broadcastMessage

add internal _sendMessage()

Comment 3

[[:fabrice] Fabrice Desré]

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 ...

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch (v2): add broadcastMessage. r=fabrice

Address nits in comment 3.

Comment 5

[Philipp von Weitershausen [:philikon]]

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!

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

[Philipp von Weitershausen [:philikon]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> I think Bug 777206 is what you are looking for, right?

Correcto. Thanks.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b0a04c791e84

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0a04c791e84

Comment 10

[Gene Lian (I already quit Mozilla)]

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. ;)

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Gene, thanks for pointing this out. I will file a bug to improve this.

Comment 12

[Gene Lian (I already quit Mozilla)]

(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

[Gene Lian (I already quit Mozilla)]

Ah! It wouldn't. Never mind. But complexity is still an issue. ;)