Closed Bug 566586 Opened 14 years ago Closed 14 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: