Bug 1139471 - Fix an issue with trying to update the Loop desktop room view's state whilst already rendering; This could cause items like tab sharing to still look like they were active even though they weren't. r=jaws

browser/components/loop/content/js/roomViews.js

@@ -166,7 +166,8 @@ loop.roomViews = (function(mozL10n) {
       ActiveRoomStoreMixin,
       sharedMixins.DocumentTitleMixin,
       sharedMixins.MediaSetupMixin,
-      sharedMixins.RoomsAudioMixin
+      sharedMixins.RoomsAudioMixin,
+      sharedMixins.WindowCloseMixin
     ],
 
     propTypes: {
@@ -204,14 +205,11 @@ loop.roomViews = (function(mozL10n) {
      * User clicked on the "Leave" button.
      */
     leaveRoom: function() {
-      this.props.dispatcher.dispatch(new sharedActions.LeaveRoom());
-    },
-
-    /**
-     * Closes the window if the cancel button is pressed in the generic failure view.
-     */
-    closeWindow: function() {
-      window.close();
+      if (this.state.used) {
+        this.props.dispatcher.dispatch(new sharedActions.LeaveRoom());
+      } else {
+        this.closeWindow();
+      }
     },
 
     /**
@@ -255,15 +253,9 @@ loop.roomViews = (function(mozL10n) {
           );
         }
         case ROOM_STATES.ENDED: {
-          if (this.state.used)
-            return React.createElement(sharedViews.FeedbackView, {
-              onAfterFeedbackReceived: this.closeWindow}
-            );
-
-          // In case the room was not used (no one was here), we
-          // bypass the feedback form.
-          this.closeWindow();
-          return null;
+          return React.createElement(sharedViews.FeedbackView, {
+            onAfterFeedbackReceived: this.closeWindow}
+          );
         }
         default: {
           return (

browser/components/loop/content/js/roomViews.jsx

@@ -166,7 +166,8 @@ loop.roomViews = (function(mozL10n) {
       ActiveRoomStoreMixin,
       sharedMixins.DocumentTitleMixin,
       sharedMixins.MediaSetupMixin,
-      sharedMixins.RoomsAudioMixin
+      sharedMixins.RoomsAudioMixin,
+      sharedMixins.WindowCloseMixin
     ],
 
     propTypes: {
@@ -204,14 +205,11 @@ loop.roomViews = (function(mozL10n) {
      * User clicked on the "Leave" button.
      */
     leaveRoom: function() {
-      this.props.dispatcher.dispatch(new sharedActions.LeaveRoom());
-    },
-
-    /**
-     * Closes the window if the cancel button is pressed in the generic failure view.
-     */
-    closeWindow: function() {
-      window.close();
+      if (this.state.used) {
+        this.props.dispatcher.dispatch(new sharedActions.LeaveRoom());
+      } else {
+        this.closeWindow();
+      }
     },
 
     /**
@@ -255,15 +253,9 @@ loop.roomViews = (function(mozL10n) {
           />;
         }
         case ROOM_STATES.ENDED: {
-          if (this.state.used)
-            return <sharedViews.FeedbackView
-              onAfterFeedbackReceived={this.closeWindow}
-            />;
-
-          // In case the room was not used (no one was here), we
-          // bypass the feedback form.
-          this.closeWindow();
-          return null;
+          return <sharedViews.FeedbackView
+            onAfterFeedbackReceived={this.closeWindow}
+          />;
         }
         default: {
           return (

browser/components/loop/content/shared/js/activeRoomStore.js

@@ -497,13 +497,6 @@ loop.store.ActiveRoomStore = (function() {
     windowUnload: function() {
       this._leaveRoom(ROOM_STATES.CLOSING);
 
-      // If we're closing the window, then ensure the screensharing state
-      // is cleared. We don't do this on leave room, as we might still be
-      // sharing.
-      this._mozLoop.setScreenShareState(
-        this.getStoreState().windowId,
-        false);
-
       if (!this._onUpdateListener) {
         return;
       }
@@ -520,7 +513,7 @@ loop.store.ActiveRoomStore = (function() {
      * Handles a room being left.
      */
     leaveRoom: function() {
-      this._leaveRoom();
+      this._leaveRoom(ROOM_STATES.ENDED);
     },
 
     /**
@@ -554,14 +547,24 @@ loop.store.ActiveRoomStore = (function() {
      * Handles leaving a room. Clears any membership timeouts, then
      * signals to the server the leave of the room.
      *
-     * @param {ROOM_STATES} nextState Optional; the next state to switch to.
-     *                                Switches to READY if undefined.
+     * @param {ROOM_STATES} nextState The next state to switch to.
      */
     _leaveRoom: function(nextState) {
       if (loop.standaloneMedia) {
         loop.standaloneMedia.multiplexGum.reset();
       }
 
+      this._mozLoop.setScreenShareState(
+        this.getStoreState().windowId,
+        false);
+
+      if (this._browserSharingListener) {
+        // Remove the browser sharing listener as we don't need it now.
+        this._mozLoop.removeBrowserSharingListener(this._browserSharingListener);
+        this._browserSharingListener = null;
+      }
+
+      // We probably don't need to end screen share separately, but lets be safe.
       this._sdkDriver.disconnectSession();
 
       if (this._timeout) {
@@ -577,7 +580,7 @@ loop.store.ActiveRoomStore = (function() {
           this._storeState.sessionToken);
       }
 
-      this.setStoreState({roomState: nextState || ROOM_STATES.ENDED});
+      this.setStoreState({roomState: nextState});
     },
 
     /**

browser/components/loop/content/shared/js/roomStates.js

@@ -28,7 +28,7 @@ loop.store.ROOM_STATES = {
     FAILED: "room-failed",
     // The room is full
     FULL: "room-full",
-    // The room conversation has ended
+    // The room conversation has ended, displays the feedback view.
     ENDED: "room-ended",
     // The window is closing
     CLOSING: "room-closing"

browser/components/loop/test/desktop-local/roomViews_test.js

@@ -22,6 +22,7 @@ describe("loop.roomViews", function () {
     };
 
     fakeWindow = {
+      close: sinon.stub(),
       document: {},
       navigator: {
         mozLoop: fakeMozLoop
@@ -290,6 +291,32 @@ describe("loop.roomViews", function () {
         sinon.match.hasOwn("name", "setMute"));
     });
 
+    it("should dispatch a `LeaveRoom` action when the hangup button is pressed and the room has been used", function() {
+      view = mountTestComponent();
+
+      view.setState({used: true});
+
+      var hangupBtn = view.getDOMNode().querySelector(".btn-hangup");
+
+      React.addons.TestUtils.Simulate.click(hangupBtn);
+
+      sinon.assert.calledOnce(dispatcher.dispatch);
+      sinon.assert.calledWithExactly(dispatcher.dispatch,
+        new sharedActions.LeaveRoom());
+    });
+
+    it("should close the window when the hangup button is pressed and the room has not been used", function() {
+      view = mountTestComponent();
+
+      view.setState({used: false});
+
+      var hangupBtn = view.getDOMNode().querySelector(".btn-hangup");
+
+      React.addons.TestUtils.Simulate.click(hangupBtn);
+
+      sinon.assert.calledOnce(fakeWindow.close);
+    });
+
     describe("#componentWillUpdate", function() {
       function expectActionDispatched(view) {
         sinon.assert.calledOnce(dispatcher.dispatch);
@@ -389,18 +416,6 @@ describe("loop.roomViews", function () {
           TestUtils.findRenderedComponentWithType(view,
             loop.shared.views.FeedbackView);
         });
-
-      it("should NOT render the FeedbackView if the room has not been used",
-        function() {
-          activeRoomStore.setStoreState({
-            roomState: ROOM_STATES.ENDED,
-            used: false
-          });
-
-          view = mountTestComponent();
-
-          expect(view.getDOMNode()).eql(null);
-        });
     });
 
     describe("Mute", function() {

browser/components/loop/test/shared/activeRoomStore_test.js

@@ -142,6 +142,15 @@ describe("loop.store.ActiveRoomStore", function () {
       sinon.assert.calledOnce(fakeMultiplexGum.reset);
     });
 
+    it("should set screen sharing inactive", function() {
+      store.setStoreState({windowId: "1234"});
+
+      store.roomFailure({error: fakeError});
+
+      sinon.assert.calledOnce(fakeMozLoop.setScreenShareState);
+      sinon.assert.calledWithExactly(fakeMozLoop.setScreenShareState, "1234", false);
+    });
+
     it("should disconnect from the servers via the sdk", function() {
       store.roomFailure({error: fakeError});
 
@@ -157,6 +166,18 @@ describe("loop.store.ActiveRoomStore", function () {
       sinon.assert.calledOnce(clearTimeout);
     });
 
+    it("should remove the sharing listener", function() {
+      // Setup the listener.
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
+
+      // Now simulate room failure.
+      store.roomFailure({error: fakeError});
+
+      sinon.assert.calledOnce(fakeMozLoop.removeBrowserSharingListener);
+    });
+
     it("should call mozLoop.rooms.leave", function() {
       store.roomFailure({error: fakeError});
 
@@ -600,6 +621,15 @@ describe("loop.store.ActiveRoomStore", function () {
       sinon.assert.calledOnce(fakeMultiplexGum.reset);
     });
 
+    it("should set screen sharing inactive", function() {
+      store.setStoreState({windowId: "1234"});
+
+      store.connectionFailure(connectionFailureAction);
+
+      sinon.assert.calledOnce(fakeMozLoop.setScreenShareState);
+      sinon.assert.calledWithExactly(fakeMozLoop.setScreenShareState, "1234", false);
+    });
+
     it("should disconnect from the servers via the sdk", function() {
       store.connectionFailure(connectionFailureAction);
 
@@ -623,6 +653,18 @@ describe("loop.store.ActiveRoomStore", function () {
         "fakeToken", "1627384950");
     });
 
+    it("should remove the sharing listener", function() {
+      // Setup the listener.
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
+
+      // Now simulate connection failure.
+      store.connectionFailure(connectionFailureAction);
+
+      sinon.assert.calledOnce(fakeMozLoop.removeBrowserSharingListener);
+    });
+
     it("should set the state to `FAILED`", function() {
       store.connectionFailure(connectionFailureAction);
 
@@ -877,6 +919,18 @@ describe("loop.store.ActiveRoomStore", function () {
           "fakeToken", "1627384950");
       });
 
+    it("should remove the sharing listener", function() {
+      // Setup the listener.
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
+
+      // Now unload the window.
+      store.windowUnload();
+
+      sinon.assert.calledOnce(fakeMozLoop.removeBrowserSharingListener);
+    });
+
     it("should set the state to CLOSING", function() {
       store.windowUnload();
 
@@ -922,6 +976,18 @@ describe("loop.store.ActiveRoomStore", function () {
         "fakeToken", "1627384950");
     });
 
+    it("should remove the sharing listener", function() {
+      // Setup the listener.
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
+
+      // Now leave the room.
+      store.leaveRoom();
+
+      sinon.assert.calledOnce(fakeMozLoop.removeBrowserSharingListener);
+    });
+
     it("should set the state to ENDED", function() {
       store.leaveRoom();