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, a=Sylvestre
authorMark Banner <standard8@mozilla.com>
Fri, 06 Mar 2015 08:28:15 +0000
changeset 248118 f8089ba15c56aab0536d154efeb2dfdfd3e8e031
parent 248117 e6b04fc354b3feffe262e6cda635e66850357380
child 248119 4037672fcc2adb6af5e821560bdb8defd62afe60
push id7762
push usermdeboer@mozilla.com
push dateMon, 16 Mar 2015 15:07:09 +0000
treeherdermozilla-aurora@9b59f3a2743d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws, Sylvestre
bugs1139471
milestone38.0a2
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, a=Sylvestre
browser/components/loop/content/js/roomViews.js
browser/components/loop/content/js/roomViews.jsx
browser/components/loop/content/shared/js/activeRoomStore.js
browser/components/loop/content/shared/js/roomStates.js
browser/components/loop/test/desktop-local/roomViews_test.js
browser/components/loop/test/shared/activeRoomStore_test.js
--- a/browser/components/loop/content/js/roomViews.js
+++ b/browser/components/loop/content/js/roomViews.js
@@ -161,17 +161,18 @@ loop.roomViews = (function(mozL10n) {
   /**
    * Desktop room conversation view.
    */
   var DesktopRoomConversationView = React.createClass({displayName: "DesktopRoomConversationView",
     mixins: [
       ActiveRoomStoreMixin,
       sharedMixins.DocumentTitleMixin,
       sharedMixins.MediaSetupMixin,
-      sharedMixins.RoomsAudioMixin
+      sharedMixins.RoomsAudioMixin,
+      sharedMixins.WindowCloseMixin
     ],
 
     propTypes: {
       dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired
     },
 
     _renderInvitationOverlay: function() {
       if (this.state.roomState !== ROOM_STATES.HAS_PARTICIPANTS) {
@@ -199,24 +200,21 @@ 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();
+      }
     },
 
     /**
      * Used to control publishing a stream - i.e. to mute a stream
      *
      * @param {String} type The type of stream, e.g. "audio" or "video".
      * @param {Boolean} enabled True to enable the stream, false otherwise.
      */
@@ -250,25 +248,19 @@ loop.roomViews = (function(mozL10n) {
         case ROOM_STATES.FULL: {
           // Note: While rooms are set to hold a maximum of 2 participants, the
           //       FULL case should never happen on desktop.
           return React.createElement(loop.conversationViews.GenericFailureView, {
             cancelCall: this.closeWindow}
           );
         }
         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 (
             React.createElement("div", {className: "room-conversation-wrapper"}, 
               this._renderInvitationOverlay(), 
               React.createElement("div", {className: "video-layout-wrapper"}, 
                 React.createElement("div", {className: "conversation room-conversation"}, 
                   React.createElement("div", {className: "media nested"}, 
--- a/browser/components/loop/content/js/roomViews.jsx
+++ b/browser/components/loop/content/js/roomViews.jsx
@@ -161,17 +161,18 @@ loop.roomViews = (function(mozL10n) {
   /**
    * Desktop room conversation view.
    */
   var DesktopRoomConversationView = React.createClass({
     mixins: [
       ActiveRoomStoreMixin,
       sharedMixins.DocumentTitleMixin,
       sharedMixins.MediaSetupMixin,
-      sharedMixins.RoomsAudioMixin
+      sharedMixins.RoomsAudioMixin,
+      sharedMixins.WindowCloseMixin
     ],
 
     propTypes: {
       dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired
     },
 
     _renderInvitationOverlay: function() {
       if (this.state.roomState !== ROOM_STATES.HAS_PARTICIPANTS) {
@@ -199,24 +200,21 @@ 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();
+      }
     },
 
     /**
      * Used to control publishing a stream - i.e. to mute a stream
      *
      * @param {String} type The type of stream, e.g. "audio" or "video".
      * @param {Boolean} enabled True to enable the stream, false otherwise.
      */
@@ -250,25 +248,19 @@ loop.roomViews = (function(mozL10n) {
         case ROOM_STATES.FULL: {
           // Note: While rooms are set to hold a maximum of 2 participants, the
           //       FULL case should never happen on desktop.
           return <loop.conversationViews.GenericFailureView
             cancelCall={this.closeWindow}
           />;
         }
         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 (
             <div className="room-conversation-wrapper">
               {this._renderInvitationOverlay()}
               <div className="video-layout-wrapper">
                 <div className="conversation room-conversation">
                   <div className="media nested">
--- a/browser/components/loop/content/shared/js/activeRoomStore.js
+++ b/browser/components/loop/content/shared/js/activeRoomStore.js
@@ -509,40 +509,33 @@ loop.store.ActiveRoomStore = (function()
     },
 
     /**
      * Handles the window being unloaded. Ensures the room is left.
      */
     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;
       }
 
       // If we're closing the window, we can stop listening to updates.
       var roomToken = this.getStoreState().roomToken;
       this._mozLoop.rooms.off("update:" + roomToken, this._onUpdateListener);
       this._mozLoop.rooms.off("delete:" + roomToken, this._onDeleteListener);
       delete this._onUpdateListener;
       delete this._onDeleteListener;
     },
 
     /**
      * Handles a room being left.
      */
     leaveRoom: function() {
-      this._leaveRoom();
+      this._leaveRoom(ROOM_STATES.ENDED);
     },
 
     /**
      * Handles setting of the refresh timeout callback.
      *
      * @param {Integer} expireTime The time until expiry (in seconds).
      */
     _setRefreshTimeout: function(expireTime) {
@@ -566,40 +559,50 @@ loop.store.ActiveRoomStore = (function()
           this._setRefreshTimeout(responseData.expires);
         }.bind(this));
     },
 
     /**
      * 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) {
         clearTimeout(this._timeout);
         delete this._timeout;
       }
 
       if (this._storeState.roomState === ROOM_STATES.JOINING ||
           this._storeState.roomState === ROOM_STATES.JOINED ||
           this._storeState.roomState === ROOM_STATES.SESSION_CONNECTED ||
           this._storeState.roomState === ROOM_STATES.HAS_PARTICIPANTS) {
         this._mozLoop.rooms.leave(this._storeState.roomToken,
           this._storeState.sessionToken);
       }
 
-      this.setStoreState({roomState: nextState || ROOM_STATES.ENDED});
+      this.setStoreState({roomState: nextState});
     },
 
     /**
      * When feedback is complete, we reset the room to the initial state.
      */
     feedbackComplete: function() {
       // Note, that we want some values, such as the windowId, so we don't
       // do a full reset here.
--- a/browser/components/loop/content/shared/js/roomStates.js
+++ b/browser/components/loop/content/shared/js/roomStates.js
@@ -23,13 +23,13 @@ loop.store.ROOM_STATES = {
     // The room is connected to the sdk server.
     SESSION_CONNECTED: "room-session-connected",
     // There are participants in the room.
     HAS_PARTICIPANTS: "room-has-participants",
     // There was an issue with the room
     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"
 };
--- a/browser/components/loop/test/desktop-local/roomViews_test.js
+++ b/browser/components/loop/test/desktop-local/roomViews_test.js
@@ -17,16 +17,17 @@ describe("loop.roomViews", function () {
     dispatcher = new loop.Dispatcher();
 
     fakeMozLoop = {
       getAudioBlob: sinon.stub(),
       getLoopPref: sinon.stub()
     };
 
     fakeWindow = {
+      close: sinon.stub(),
       document: {},
       navigator: {
         mozLoop: fakeMozLoop
       },
       addEventListener: function() {},
       removeEventListener: function() {}
     };
     loop.shared.mixins.setRootObject(fakeWindow);
@@ -285,16 +286,42 @@ describe("loop.roomViews", function () {
       var muteBtn = view.getDOMNode().querySelector('.btn-mute-video');
 
       React.addons.TestUtils.Simulate.click(muteBtn);
 
       sinon.assert.calledWithMatch(dispatcher.dispatch,
         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);
         sinon.assert.calledWithExactly(dispatcher.dispatch,
           sinon.match.instanceOf(sharedActions.SetupStreamElements));
         sinon.assert.calledWithExactly(dispatcher.dispatch,
           sinon.match(function(value) {
             return value.getLocalElementFunc() ===
@@ -384,28 +411,16 @@ describe("loop.roomViews", function () {
             used: true
           });
 
           view = mountTestComponent();
 
           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() {
       it("should render local media as audio-only if video is muted",
         function() {
           activeRoomStore.setStoreState({
             roomState: ROOM_STATES.SESSION_CONNECTED,
             videoMuted: true
--- a/browser/components/loop/test/shared/activeRoomStore_test.js
+++ b/browser/components/loop/test/shared/activeRoomStore_test.js
@@ -138,31 +138,52 @@ describe("loop.store.ActiveRoomStore", f
       });
 
     it("should reset the multiplexGum", function() {
       store.roomFailure({error: fakeError});
 
       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});
 
       sinon.assert.calledOnce(fakeSdkDriver.disconnectSession);
     });
 
     it("should clear any existing timeout", function() {
       sandbox.stub(window, "clearTimeout");
       store._timeout = {};
 
       store.roomFailure({error: fakeError});
 
       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});
 
       sinon.assert.calledOnce(fakeMozLoop.rooms.leave);
       sinon.assert.calledWithExactly(fakeMozLoop.rooms.leave,
         "fakeToken", "1627384950");
     });
   });
@@ -616,16 +637,25 @@ describe("loop.store.ActiveRoomStore", f
     });
 
     it("should reset the multiplexGum", function() {
       store.connectionFailure(connectionFailureAction);
 
       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);
 
       sinon.assert.calledOnce(fakeSdkDriver.disconnectSession);
     });
 
     it("should clear any existing timeout", function() {
       sandbox.stub(window, "clearTimeout");
@@ -639,16 +669,28 @@ describe("loop.store.ActiveRoomStore", f
     it("should call mozLoop.rooms.leave", function() {
       store.connectionFailure(connectionFailureAction);
 
       sinon.assert.calledOnce(fakeMozLoop.rooms.leave);
       sinon.assert.calledWithExactly(fakeMozLoop.rooms.leave,
         "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);
 
       expect(store.getStoreState().roomState).eql(ROOM_STATES.FAILED);
     });
   });
 
   describe("#setMute", function() {
@@ -893,16 +935,28 @@ describe("loop.store.ActiveRoomStore", f
 
         store.windowUnload();
 
         sinon.assert.calledOnce(fakeMozLoop.rooms.leave);
         sinon.assert.calledWithExactly(fakeMozLoop.rooms.leave,
           "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();
 
       expect(store._storeState.roomState).eql(ROOM_STATES.CLOSING);
     });
   });
 
   describe("#leaveRoom", function() {
@@ -938,16 +992,28 @@ describe("loop.store.ActiveRoomStore", f
     it("should call mozLoop.rooms.leave", function() {
       store.leaveRoom();
 
       sinon.assert.calledOnce(fakeMozLoop.rooms.leave);
       sinon.assert.calledWithExactly(fakeMozLoop.rooms.leave,
         "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();
 
       expect(store._storeState.roomState).eql(ROOM_STATES.ENDED);
     });
   });
 
   describe("Events", function() {