Bug 1113574 - Check that the room <chatbox> is still open before resuming the tour from a join notification. r=dmose,markh a=gavin
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Tue, 23 Dec 2014 02:35:28 -0500
changeset 242598 bc52fc22602f3e4d9869be8d1f18b2e3c988a848
parent 242597 c2fe047664b68346982fbf443582739b41432b73
child 242599 9c8cb5f619d0ac801c49b57d673f17753f627f48
push id4311
push userraliiev@mozilla.com
push dateMon, 12 Jan 2015 19:37:41 +0000
treeherdermozilla-beta@150c9fed433b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdmose, markh, gavin
bugs1113574
milestone36.0a2
Bug 1113574 - Check that the room <chatbox> is still open before resuming the tour from a join notification. r=dmose,markh a=gavin The room participant list only gets updated after getting a push notification and this can lead to the Rooms store still showing the user in their room even after they have left. To avoid this, we check that the <chatbox> for the room is actually still present.
browser/components/loop/MozLoopService.jsm
browser/modules/Chat.jsm
--- a/browser/components/loop/MozLoopService.jsm
+++ b/browser/components/loop/MozLoopService.jsm
@@ -779,40 +779,47 @@ let MozLoopServiceInternal = {
             "Successfully staged loop report for telemetry upload." :
             ("Failed to stage loop report. Error: " + e.data.fail));
         }
         worker.postMessage(job);
       });
     }, pc.id);
   },
 
+  getChatWindowID: function(conversationWindowData) {
+    // Try getting a window ID that can (re-)identify this conversation, or resort
+    // to a globally unique one as a last resort.
+    // XXX We can clean this up once rooms and direct contact calling are the only
+    //     two modes left.
+    let windowId = ("contact" in conversationWindowData) ?
+                   conversationWindowData.contact._guid || gLastWindowId++ :
+                   conversationWindowData.roomToken || conversationWindowData.callId ||
+                   gLastWindowId++;
+    return windowId.toString();
+  },
+
+  getChatURL: function(chatWindowId) {
+    return "about:loopconversation#" + chatWindowId;
+  },
+
   /**
    * Opens the chat window
    *
    * @param {Object} conversationWindowData The data to be obtained by the
    *                                        window when it opens.
    * @returns {Number} The id of the window.
    */
   openChatWindow: function(conversationWindowData) {
     // So I guess the origin is the loop server!?
     let origin = this.loopServerUri;
-    // Try getting a window ID that can (re-)identify this conversation, or resort
-    // to a globally unique one as a last resort.
-    // XXX We can clean this up once rooms and direct contact calling are the only
-    //     two modes left.
-    let windowId = ("contact" in conversationWindowData) ?
-                   conversationWindowData.contact._guid || gLastWindowId++ :
-                   conversationWindowData.roomToken || conversationWindowData.callId ||
-                   gLastWindowId++;
-    // Store the id as a string, as that's what we use elsewhere.
-    windowId = windowId.toString();
+    let windowId = this.getChatWindowID(conversationWindowData);
 
     gConversationWindowData.set(windowId, conversationWindowData);
 
-    let url = "about:loopconversation#" + windowId;
+    let url = this.getChatURL(windowId);
 
     let callback = chatbox => {
       // We need to use DOMContentLoaded as otherwise the injection will happen
       // in about:blank and then get lost.
       // Sadly we can't use chatbox.promiseChatLoaded() as promise chaining
       // involves event loop spins, which means it might be too late.
       // Have we already done it?
       if (chatbox.contentWindow.navigator.mozLoop) {
@@ -1094,62 +1101,76 @@ this.MozLoopService = {
           sound: "room-joined",
           title: room.roomName,
           message: MozLoopServiceInternal.localizedStrings.get("rooms_room_joined_label"),
           selectTab: "rooms"
         });
       }
     });
 
-    // Resume the tour (re-opening the tab, if necessary) if someone else joins
-    // a room of ours and it's currently open.
-    LoopRooms.on("joined", (e, room, participant) => {
-      let isOwnerInRoom = false;
-      let isOtherInRoom = false;
-
-      if (!this.getLoopPref("gettingStarted.resumeOnFirstJoin")) {
-        return;
-      }
-
-      if (!room.participants) {
-        return;
-      }
-
-      // The particpant that joined isn't necessarily included in room.participants (depending on
-      // when the broadcast happens) so concatenate.
-      for (let participant of room.participants.concat(participant)) {
-        if (participant.owner) {
-          isOwnerInRoom = true;
-        } else {
-          isOtherInRoom = true;
-        }
-      }
-
-      if (!isOwnerInRoom || !isOtherInRoom) {
-        return;
-      }
-
-      this.resumeTour("open");
-    });
+    LoopRooms.on("joined", this.maybeResumeTourOnRoomJoined.bind(this));
 
     // If expiresTime is not in the future and the user hasn't
     // previously authenticated then skip registration.
     if (!MozLoopServiceInternal.urlExpiryTimeIsInFuture() &&
         !LoopRooms.getGuestCreatedRoom() &&
         !MozLoopServiceInternal.fxAOAuthTokenData) {
       return Promise.resolve("registration not needed");
     }
 
     let deferredInitialization = Promise.defer();
     gInitializeTimerFunc(deferredInitialization);
 
     return deferredInitialization.promise;
   }),
 
   /**
+   * Maybe resume the tour (re-opening the tab, if necessary) if someone else joins
+   * a room of ours and it's currently open.
+   */
+  maybeResumeTourOnRoomJoined: function(e, room, participant) {
+    let isOwnerInRoom = false;
+    let isOtherInRoom = false;
+
+    if (!this.getLoopPref("gettingStarted.resumeOnFirstJoin")) {
+      return;
+    }
+
+    if (!room.participants) {
+      return;
+    }
+
+    // The particpant that joined isn't necessarily included in room.participants (depending on
+    // when the broadcast happens) so concatenate.
+    for (let participant of room.participants.concat(participant)) {
+      if (participant.owner) {
+        isOwnerInRoom = true;
+      } else {
+        isOtherInRoom = true;
+      }
+    }
+
+    if (!isOwnerInRoom || !isOtherInRoom) {
+      return;
+    }
+
+    // Check that the room chatbox is still actually open using its URL
+    let chatboxesForRoom = [...Chat.chatboxes].filter(chatbox => {
+      return chatbox.src == MozLoopServiceInternal.getChatURL(room.roomToken);
+    });
+
+    if (!chatboxesForRoom.length) {
+      log.warn("Tried to resume the tour from a join when the chatbox was closed", room);
+      return;
+    }
+
+    this.resumeTour("open");
+  },
+
+  /**
    * The core of the initialization work that happens once the browser is ready
    * (after a timer when called during startup).
    *
    * Can be called more than once (e.g. if the initial setup fails at some phase).
    * @param {Deferred} deferredInitialization
    */
   delayedInitialize: Task.async(function*(deferredInitialization) {
     log.debug("delayedInitialize");
--- a/browser/modules/Chat.jsm
+++ b/browser/modules/Chat.jsm
@@ -45,16 +45,48 @@ function getChromeWindow(contentWin) {
                    .getInterface(Ci.nsIDOMWindow);
 }
 
 /*
  * The exported Chat object
  */
 
 let Chat = {
+
+  /**
+   * Iterator of <chatbox> elements from this module in all windows.
+   */
+  get chatboxes() {
+    return function*() {
+      let winEnum = Services.wm.getEnumerator("navigator:browser");
+      while (winEnum.hasMoreElements()) {
+        let win = winEnum.getNext();
+        let chatbar = win.document.getElementById("pinnedchats");
+        if (!chatbar)
+          continue;
+
+        // Make a new array instead of the live NodeList so this iterator can be
+        // used for closing/deleting.
+        let chatboxes = [c for (c of chatbar.children)];
+        for (let chatbox of chatboxes) {
+          yield chatbox;
+        }
+      }
+
+      // include standalone chat windows
+      winEnum = Services.wm.getEnumerator("Social:Chat");
+      while (winEnum.hasMoreElements()) {
+        let win = winEnum.getNext();
+        if (win.closed)
+          continue;
+        yield win.document.getElementById("chatter");
+      }
+    }();
+  },
+
   /**
    * Open a new chatbox.
    *
    * @param contentWindow [optional]
    *        The content window that requested this chat.  May be null.
    * @param origin
    *        The origin for the chat.  This is primarily used as an identifier
    *        to help identify all chats from the same provider.
@@ -103,36 +135,21 @@ let Chat = {
 
   /**
    * Close all chats from the specified origin.
    *
    * @param origin
    *        The origin from which all chats should be closed.
    */
   closeAll: function(origin) {
-    // close all attached chat windows
-    let winEnum = Services.wm.getEnumerator("navigator:browser");
-    while (winEnum.hasMoreElements()) {
-      let win = winEnum.getNext();
-      let chatbar = win.document.getElementById("pinnedchats");
-      if (!chatbar)
+    for (let chatbox of this.chatboxes) {
+      if (chatbox.content.getAttribute("origin") != origin) {
         continue;
-      let chats = [c for (c of chatbar.children) if (c.content.getAttribute("origin") == origin)];
-      [c.close() for (c of chats)];
-    }
-
-    // close all standalone chat windows
-    winEnum = Services.wm.getEnumerator("Social:Chat");
-    while (winEnum.hasMoreElements()) {
-      let win = winEnum.getNext();
-      if (win.closed)
-        continue;
-      let chatOrigin = win.document.getElementById("chatter").content.getAttribute("origin");
-      if (origin == chatOrigin)
-        win.close();
+      }
+      chatbox.close();
     }
   },
 
   /**
    * Focus the chatbar associated with a window
    *
    * @param window
    */