Closed Bug 789005 Opened 8 years ago Closed 8 years ago

System message API: decrease complexity of 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

Attachments

(1 file, 2 obsolete files)

According to the comment here [1], the logic should be corrected.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=787064#c10
Attached patch Patch: remove a redundant loop (obsolete) — Splinter Review
Avoid a redundant searching loop in broadcastMessage()
Comment on attachment 660688 [details] [diff] [review]
Patch: remove a redundant loop

Hi Fabrice,
Currently, "this._pages.forEach" are called twice in broadcastMessage(). I did some refactoring in this patch to remove a redundant searching loop in broadcastMessage(). Could you please help review this? Thanks.
Attachment #660688 - Flags: review?(fabrice)
Comment on attachment 660688 [details] [diff] [review]
Patch: remove a redundant loop

Review of attachment 660688 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'd like to check if we can get a small improvement.

::: dom/messages/SystemMessageInternal.js
@@ +74,5 @@
> +
> +        aPage.pending.push(aMessage);
> +        if (aPage.pending.length > kMaxPendingMessages) {
> +          aPage.pending.splice(0, 1);
> +        }

could this block also be part of the common function ? (rename it since it won't be just notifying observers).
Attachment #660688 - Flags: review?(fabrice) → review-
Comment #3 addressed
Attachment #660688 - Attachment is obsolete: true
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 660688 [details] [diff] [review]
> Patch: remove a redundant loop
> 
> Review of attachment 660688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'd like to check if we can get a small improvement.
> 
> ::: dom/messages/SystemMessageInternal.js
> @@ +74,5 @@
> > +
> > +        aPage.pending.push(aMessage);
> > +        if (aPage.pending.length > kMaxPendingMessages) {
> > +          aPage.pending.splice(0, 1);
> > +        }
> 
> could this block also be part of the common function ? (rename it since it
> won't be just notifying observers).
I moved this part into that common function according to your opinion. The function has been renamed '_processPage' because it pushes the message into the page's pending queue, and it also extracts some page attributes to observers.
Attachment #660730 - Flags: review?(fabrice)
Comment on attachment 660730 [details] [diff] [review]
Patch (v2): remove a redundant loop

Review of attachment 660730 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed. Thanks!

::: dom/messages/SystemMessageInternal.js
@@ +44,5 @@
> +    debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));
> +    ppmm.broadcastAsyncMessage("SystemMessageManager:Message" , { type: aType,
> +                                                                  msg: aMessage,
> +                                                                  manifest: aManifestURI.spec });
> +    // Queue the message for pages that registered an handler for this type.

Nit: this comment is now incorrect.

@@ +59,5 @@
>  
>    broadcastMessage: function broadcastMessage(aType, aMessage) {
> +    debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));
> +    // Find pages that registered an handler for this type.
> +    // Queue the message for the pages.

Nit: remove the second comment line.
Attachment #660730 - Flags: review?(fabrice) → review+
Nits in comment 6 addressed. r=fabrice.
Thanks!
Attachment #660730 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3d505a8be46d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.