Closed Bug 566586 Opened 15 years ago Closed 15 years ago

[f10s] Remove MessagePasser code

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 3 obsolete files)

Patch v0.3
9.91 KB, patch
mfinkle
: review+
stechz
: review+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — — Splinter Review
No description provided.
Attachment #445937 - Flags: review?(webapps)
Attachment #445937 - Flags: review?(mark.finkle)
Summary: [f10s] Remove MessageManager code → [f10s] Remove MessagePasser code
Comment on attachment 445937 [details] [diff] [review] Patch >+ window.messageManager.addMessageListener("FennecMozScrolledAreaChanged", this); >+ window.messageManager.addMessageListener("FennecMozAfterPaint", this); Why not just messageManager? >- Browser.messagePasser.sendMessage(oldBrowser, "FennecBlur"); >+ Browser.sendMessage(oldBrowser, "FennecBlur"); I think sendMessage would be a great candidate for a browser binding method, assuming mfinkle agrees.
Attachment #445937 - Flags: review?(webapps) → review-
(In reply to comment #1) > (From update of attachment 445937 [details] [diff] [review]) > >+ window.messageManager.addMessageListener("FennecMozScrolledAreaChanged", this); > >+ window.messageManager.addMessageListener("FennecMozAfterPaint", this); > > Why not just messageManager? Because if it missed the "namespace" I always wonder if I'm going to call a custom function. I mostly prefer using window.XXX for that. > > >- Browser.messagePasser.sendMessage(oldBrowser, "FennecBlur"); > >+ Browser.sendMessage(oldBrowser, "FennecBlur"); > > I think sendMessage would be a great candidate for a browser binding method, > assuming mfinkle agrees. Good catch, I think the browser binding probably already have one! :)
Attached patch Patch v0.2 (obsolete) — — Splinter Review
This patch add a sendMessage method to browser.xml.
Assignee: nobody → 21
Attachment #445937 - Attachment is obsolete: true
Attachment #446483 - Flags: review?(webapps)
Attachment #446483 - Flags: review?(mark.finkle)
Attachment #445937 - Flags: review?(mark.finkle)
Attachment #446483 - Flags: review?(mark.finkle) → review-
Comment on attachment 446483 [details] [diff] [review] Patch v0.2 >- Browser.messagePasser.sendMessage(oldBrowser, "FennecBlur"); >+ oldBrowser.sendMessage("FennecBlur"); Sorry, I should have commented sooner. We did not want "sendMessage" in browser.xml since browser.messageManager.sendAsyncMessage(...) works just fine. We can add a helper to Util for creating the JSON, if we need to. >- Browser.messagePasser.sendMessage(browser, "FennecFocus"); >+ browser.sendMessage("FennecFocus"); Same >+ <method name="sendMessage"> >+ <parameter name="aMessage"/> >+ <body><![CDATA[ >+ let data = Array.prototype.slice.call(arguments, 1); >+ this.messageManager.sendAsyncMessage(aMessage, data); >+ ]]></body> >+ </method> >+ Please remove. We don't need it and I do not like forcing the use of an Array for the JSON. Note: I do not like using a simple Array for passing JSON over messages. I do like using an Object so we have named parameters. Just a comment, I like using "window.addMessageListener", instead of just "addMessageListener" since it's more explicit for the listener and it's what we would do for "addEventListener" too.
> Sorry, I should have commented sooner. We did not want "sendMessage" in > browser.xml since browser.messageManager.sendAsyncMessage(...) works just fine. OK, neat. I agree. > Please remove. We don't need it and I do not like forcing the use of an Array > for the JSON. Sorry for the confusion Vivien :) I agree that we shouldn't be forcing an array here, and since we have messageManager on browser, the convenience method isn't necessary. > Just a comment, I like using "window.addMessageListener", instead of just > "addMessageListener" since it's more explicit for the listener and it's what we > would do for "addEventListener" too. Except the message listener isn't really tied to the window in any way.
(In reply to comment #5) > > Just a comment, I like using "window.addMessageListener", instead of just > > "addMessageListener" since it's more explicit for the listener and it's what we > > would do for "addEventListener" too. > > Except the message listener isn't really tied to the window in any way. Fine. I have been persuaded to remove "window."
Attached patch Patch v0.3 (obsolete) — — Splinter Review
Attachment #446483 - Attachment is obsolete: true
Attachment #446709 - Flags: review?(webapps)
Attachment #446709 - Flags: review?(mark.finkle)
Attachment #446483 - Flags: review?(webapps)
Comment on attachment 446709 [details] [diff] [review] Patch v0.3 Wrong patch? Looks like an old patch from bug 566640.
Attached patch Patch v0.3 — — Splinter Review
Oups, sorry for that.
Attachment #446709 - Attachment is obsolete: true
Attachment #446836 - Flags: review?(webapps)
Attachment #446836 - Flags: review?(mark.finkle)
Attachment #446709 - Flags: review?(webapps)
Attachment #446709 - Flags: review?(mark.finkle)
Comment on attachment 446836 [details] [diff] [review] Patch v0.3 >+ sendMessage: function sendMessage(aBrowser, aName) { >+ let frameLoader = aBrowser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader; >+ let manager = frameLoader.messageManager; >+ manager.sendAsyncMessage(aName, Array.prototype.slice.call(arguments, 2)); >+ }, aBrowser.messageManager.sendAsyncMessage(aName, Array.prototype.slice.call(arguments, 2)); should work. Note: I don't like the Array->JSON part, but we can debate that later.
Attachment #446836 - Flags: review?(mark.finkle) → review+
Comment on attachment 446836 [details] [diff] [review] Patch v0.3 Looks good. I'm fine with removing the argument based stuff (even moving away from Util.receiveMessage if we like). This would be a good bug to file and to consider if we want to use anything on top of message manager or not.
Attachment #446836 - Flags: review?(webapps) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: