Closed Bug 805537 Opened 12 years ago Closed 12 years ago

navigator.mozSocial.openChatWindow's callback should be called only once

Categories

(Firefox Graveyard :: SocialAPI, defect)

Product:
Firefox Graveyard
Component:
SocialAPI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox18-)

Status:
RESOLVED FIXED
Milestone:
Firefox 19
Tracking Flags:
Tracking Status
firefox18 - ---

People

(Reporter: florian, Assigned: markh)

Details

Attachments

(1 file, 3 obsolete files)

As landed
10.15 KB, patch
markh
: review+
Details | Diff | Splinter Review 
It's currently called each time a DOMContentLoaded event fires in the chat window. That surprised me, and I think (gavin agreed on IRC) it's not the expected behavior.
I would expect the mozSocial code to forget the callback after it's been called once.
Assignee: nobody → mhammond

        Attachment #677665 -
        Flags: review?(mixedpuppy)

    
    

    
  
Comment on attachment 677665 [details] [diff] [review]
Remove DOMContentLoaded handler and nuke callback for good measure.

Only question I have around this change is whether we should consider a situation where two different content area's open the same chat (e.g. sidebar and worker), and need a window ref in return.  I believe the API docs would lead developers to believe they would get at least one callback in each case.  If the chat is already open, should openChatWindow return the window ref or perhaps call the callback with the ref?

        Attachment #677665 -
        Flags: review?(mixedpuppy) → review+

    
    

    
  
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Only question I have around this change is whether we should consider a
> situation where two different content area's open the same chat (e.g.
> sidebar and worker), and need a window ref in return.  I believe the API
> docs would lead developers to believe they would get at least one callback
> in each case.  If the chat is already open, should openChatWindow return the
> window ref or perhaps call the callback with the ref?

If the chat is already open, openChat arranges to still make a callback (this is an old bug we already fixed and has tests.

But you are correct in that there is a problem if the 2 chats are requested *before* the load event has fired - in that case the second one would probably be called too early.  But that behaviour wasn't introduced by this patch, so how about we open a new bug for this (it would involve turning self._callback into an array of callbacks, and have openChat check the loadState and either callback immediately or append the new callback to the array.

    
    

    
  


  
I think this one handles all those cases correctly - there is no test for the "new chat requested before onload has fired" as I can't see how to test that reliably - but there are tests that (a) callback isn't fired twice and (b) second request for a chat does callback.

        Attachment #677665 -
        Attachment is obsolete: true

        Attachment #677969 -
        Flags: review?(mixedpuppy)

    
    

    
  
Comment on attachment 677969 [details] [diff] [review]
updated so new chat window requests before onload have fired are called back onload

Hoping jaws will clear his review queue quicker!

        Attachment #677969 -
        Flags: review?(mixedpuppy) → review?(jaws)

    
    

    
  
Comment on attachment 677969 [details] [diff] [review]
updated so new chat window requests before onload have fired are called back onload

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

::: browser/base/content/socialchat.xml
@@ +56,5 @@
>          <parameter name="aCallback"/>
>          <body><![CDATA[
> +          // callbacks to be called when onload fires - more might be added
> +          // if the same chat is requested before onload has fired.  Set back
> +          // to null in, so null means onload has already fired and new

// to null in openChat, so null means onload has already fired and new

@@ +328,5 @@
>                  this.selectedChat = cb;
>                if (cb.collapsed)
>                  this.showChat(cb);
> +              if (aCallback) {
> +                if (cb._callbacks == null) {

if (!cb._callbacks) {

        Attachment #677969 -
        Flags: review?(jaws) → review+

    
    

    
  
Actually, you can keep the explicit check for null here.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rebased and new test (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry - the old patch was out of date and needed reworking to make work - enough that I can't carry the r+ forward.

There is now an initChatBox method instead of the constructor, and the timing of adding the unload event was changed (it is now added in DOMContentLoaded).  Also, I added a test that the sub-iframe in the chatboxes firing an unload event didn't cause our event handlers to also be removed

        Attachment #677969 -
        Attachment is obsolete: true

        Attachment #681299 -
        Flags: review?(jaws)

    
    

    
  
Comment on attachment 681299 [details] [diff] [review]
rebased and new test

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

::: browser/base/content/socialchat.xml
@@ +347,5 @@
> +          let iframeWindow = aChatBox.iframe.contentWindow;
> +          aChatBox.addEventListener("DOMContentLoaded", function DOMContentLoaded() {
> +            aChatBox.removeEventListener("DOMContentLoaded", DOMContentLoaded);
> +            aChatBox.isActive = !aChatBox.minimized;
> +            for (let cb of aChatBox._callbacks) {

s/cb/callback/ since cb is also used as a shortname for chatbox.

@@ +384,5 @@
> +                if (cb._callbacks == null) {
> +                  // DOMContentLoaded has already fired, so callback now.
> +                  aCallback(cb.iframe.contentWindow);
> +                } else {
> +                  // onload for this chat is yet to fire...

// DOMContentLoaded for this chat has yet to fire.

::: browser/base/content/test/browser_social_chatwindow.js
@@ +363,5 @@
>        // The nub should lose the activity flag here too
>        todo(!chatbar.nub.hasAttribute("activity"), "Bug 806266 - nub should no longer have activity");
>        // TODO: tests for bug 806266 should arrange to have 2 chats collapsed
>        // then open them checking the nub is updated correctly.
> +      // Now we wlll go and change the embedded iframe in the second chat and

s/wlll/will/

@@ +375,5 @@
> +        executeSoon(function() {
> +          let evt = iframe2.contentDocument.createEvent("CustomEvent");
> +          evt.initCustomEvent("socialChatActivity", true, true, {});
> +          iframe2.contentDocument.documentElement.dispatchEvent(evt);
> +          ok(second.hasAttribute("activity"), "second chat still have activity after unloading sub-iframe");

s/have/has/

        Attachment #681299 -
        Flags: review?(jaws) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        As landed
        — 
          — Splinter Review
      

    


  

        Attachment #681299 -
        Attachment is obsolete: true

        Attachment #681837 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8072a58a9e86
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19

    
    

    
  
Is this Social API fix still desired on FF18? If so, please nominate asap.

    
    

    
  
personally, I think we can live without this on 18 as it is fairly easy for providers to work around.  Gavin, what do you think?

    
    

    
  
Yeah, I think this hasn't been much of a problem in practice (Facebook doesn't use subframes in their chat panels, and doesn't reload them, IIRC).

          tracking-firefox18:
          + → -

    
  
Product: Firefox → Firefox Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: