Closed Bug 789005 Opened 12 years ago Closed 12 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
Blocks: 787064
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: