Closed Bug 927363 Opened 6 years ago Closed 6 years ago

Add/Remove SystemMessageManager:GetPendingMessages:Return on demand

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla27

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

Attachments

(1 file)

We don't need to add the SystemMessageManager:GetPendingMessages:Return listener during the initialization of the SystemMessageManager. We can simply add it on demand when sending the SystemMessageManager:GetPendingMessages request.
Assignee: nobody → ferjmoreno
Attached patch v1Splinter Review
Attachment #817786 - Flags: review?(gene.lian)
Comment on attachment 817786 [details] [diff] [review]
v1

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

::: dom/messages/SystemMessageManager.js
@@ +250,5 @@
>  
>    // nsIDOMGlobalPropertyInitializer implementation.
>    init: function sysMessMgr_init(aWindow) {
>      debug("init");
> +    this.initDOMRequestHelper(aWindow, ["SystemMessageManager:Message"]);

Looks good to me but have one question not related to this bug. r=gene

In bug 915598, the default initDOMRequestHelper goes in a way of adding weak reference, which means the "SystemMessageManager:Message" listener would be removed at any points even if the window is still alive. Right? How can this happen? Does it mean the child window would probably miss some system messages sent from the parent?
Attachment #817786 - Flags: review?(gene.lian) → review+
Thanks Gene!

https://hg.mozilla.org/integration/b2g-inbound/rev/3c0d710561bf

Regarding your question, it's actually possible that the listener is removed if the object is GC'd, I'm creating a bug for this. Now that we fixed bug 918479, I believe we are safe adding strong listeners in these specific cases. We still need to review all APIs using DOMRequestHelper.
https://hg.mozilla.org/mozilla-central/rev/3c0d710561bf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Please reopen bugs if you back out patches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> Please reopen bugs if you back out patches.

Yes, I know. :) We're leaving comments at the same time.

Let's keep the original codes as they are since it hasn't fixed fix any bugs. Instead, just some enhancement (cause regression though...). Resolved this one as WONTFIX.

Fernanodo, please feel free to reopen it if you still think we need to fix that. Otherwise, I'd prefer keeping that.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
Also, you don't need to backout from both b2g-inbound and m-c. They merge to each other. One branch is fine and reduces the odds of hitting merge conflicts later.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #8)
> Also, you don't need to backout from both b2g-inbound and m-c. They merge to
> each other. One branch is fine and reduces the odds of hitting merge
> conflicts later.

*and inbound. That was 3x more work than you needed to do.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.