Closed
Bug 789005
Opened 12 years ago
Closed 12 years ago
System message API: decrease complexity of broadcastMessage()
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: hsinyi, Assigned: hsinyi)
References
Details
Attachments
(1 file, 2 obsolete files)
4.64 KB,
patch
|
Details | Diff | Splinter Review |
According to the comment here [1], the logic should be corrected.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=787064#c10
Assignee | ||
Comment 1•12 years ago
|
||
Avoid a redundant searching loop in broadcastMessage()
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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-
Assignee | ||
Comment 4•12 years ago
|
||
Comment #3 addressed
Attachment #660688 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #660730 -
Flags: review?(fabrice)
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
Nits in comment 6 addressed. r=fabrice.
Thanks!
Attachment #660730 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Target Milestone: --- → mozilla18
Comment 9•12 years ago
|
||
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.
Description
•