Bug 1106934 Opening a Loop room can show an unexpected error due to race conditions. r=nperriault a=sylvestre
authorMark Banner <standard8@mozilla.com>
Tue, 02 Dec 2014 18:04:58 -0800
changeset 242329 fdb517499757cd03016d1ee40e9d8b99f99e1906
parent 242328 005e8f9938000f4de7e85d16b206a2a1462cca7f
child 242330 7365fec8ba5c848c3c52da67495b8b2fcbe9047a
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)
reviewersnperriault, sylvestre
bugs1106934
milestone36.0a2
Bug 1106934 Opening a Loop room can show an unexpected error due to race conditions. r=nperriault a=sylvestre
browser/components/loop/content/js/roomViews.js
browser/components/loop/content/js/roomViews.jsx
browser/components/loop/test/desktop-local/roomViews_test.js
--- a/browser/components/loop/content/js/roomViews.js
+++ b/browser/components/loop/content/js/roomViews.js
@@ -157,25 +157,30 @@ loop.roomViews = (function(mozL10n) {
       /**
        * OT inserts inline styles into the markup. Using a listener for
        * resize events helps us trigger a full width/height on the element
        * so that they update to the correct dimensions.
        * XXX: this should be factored as a mixin.
        */
       window.addEventListener('orientationchange', this.updateVideoContainer);
       window.addEventListener('resize', this.updateVideoContainer);
+    },
 
+    componentWillUpdate: function(nextProps, nextState) {
       // The SDK needs to know about the configuration and the elements to use
       // for display. So the best way seems to pass the information here - ideally
       // the sdk wouldn't need to know this, but we can't change that.
-      this.props.dispatcher.dispatch(new sharedActions.SetupStreamElements({
-        publisherConfig: this._getPublisherConfig(),
-        getLocalElementFunc: this._getElement.bind(this, ".local"),
-        getRemoteElementFunc: this._getElement.bind(this, ".remote")
-      }));
+      if (this.state.roomState !== ROOM_STATES.MEDIA_WAIT &&
+          nextState.roomState === ROOM_STATES.MEDIA_WAIT) {
+        this.props.dispatcher.dispatch(new sharedActions.SetupStreamElements({
+          publisherConfig: this._getPublisherConfig(),
+          getLocalElementFunc: this._getElement.bind(this, ".local"),
+          getRemoteElementFunc: this._getElement.bind(this, ".remote")
+        }));
+      }
     },
 
     _getPublisherConfig: function() {
       // height set to 100%" to fix video layout on Google Chrome
       // @see https://bugzilla.mozilla.org/show_bug.cgi?id=1020445
       return {
         insertMode: "append",
         width: "100%",
--- a/browser/components/loop/content/js/roomViews.jsx
+++ b/browser/components/loop/content/js/roomViews.jsx
@@ -157,25 +157,30 @@ loop.roomViews = (function(mozL10n) {
       /**
        * OT inserts inline styles into the markup. Using a listener for
        * resize events helps us trigger a full width/height on the element
        * so that they update to the correct dimensions.
        * XXX: this should be factored as a mixin.
        */
       window.addEventListener('orientationchange', this.updateVideoContainer);
       window.addEventListener('resize', this.updateVideoContainer);
+    },
 
+    componentWillUpdate: function(nextProps, nextState) {
       // The SDK needs to know about the configuration and the elements to use
       // for display. So the best way seems to pass the information here - ideally
       // the sdk wouldn't need to know this, but we can't change that.
-      this.props.dispatcher.dispatch(new sharedActions.SetupStreamElements({
-        publisherConfig: this._getPublisherConfig(),
-        getLocalElementFunc: this._getElement.bind(this, ".local"),
-        getRemoteElementFunc: this._getElement.bind(this, ".remote")
-      }));
+      if (this.state.roomState !== ROOM_STATES.MEDIA_WAIT &&
+          nextState.roomState === ROOM_STATES.MEDIA_WAIT) {
+        this.props.dispatcher.dispatch(new sharedActions.SetupStreamElements({
+          publisherConfig: this._getPublisherConfig(),
+          getLocalElementFunc: this._getElement.bind(this, ".local"),
+          getRemoteElementFunc: this._getElement.bind(this, ".remote")
+        }));
+      }
     },
 
     _getPublisherConfig: function() {
       // height set to 100%" to fix video layout on Google Chrome
       // @see https://bugzilla.mozilla.org/show_bug.cgi?id=1020445
       return {
         insertMode: "append",
         width: "100%",
--- a/browser/components/loop/test/desktop-local/roomViews_test.js
+++ b/browser/components/loop/test/desktop-local/roomViews_test.js
@@ -201,25 +201,16 @@ describe("loop.roomViews", function () {
           dispatcher: dispatcher,
           roomStore: roomStore,
           feedbackStore: new loop.store.FeedbackStore(dispatcher, {
             feedbackClient: {}
           })
         }));
     }
 
-    it("should dispatch a setupStreamElements action when the view is created",
-      function() {
-        view = mountTestComponent();
-
-        sinon.assert.calledOnce(dispatcher.dispatch);
-        sinon.assert.calledWithMatch(dispatcher.dispatch,
-          sinon.match.hasOwn("name", "setupStreamElements"));
-    });
-
     it("should dispatch a setMute action when the audio mute button is pressed",
       function() {
         view = mountTestComponent();
 
         view.setState({audioMuted: true});
 
         var muteBtn = view.getDOMNode().querySelector('.btn-mute-audio');
 
@@ -266,16 +257,54 @@ describe("loop.roomViews", function () {
 
       view.setState({audioMuted: true});
 
       var muteBtn = view.getDOMNode().querySelector('.btn-mute-audio');
 
       expect(muteBtn.classList.contains("muted")).eql(true);
     });
 
+    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() ===
+                   view.getDOMNode().querySelector(".local");
+          }));
+        sinon.assert.calledWithExactly(dispatcher.dispatch,
+          sinon.match(function(value) {
+            return value.getRemoteElementFunc() ===
+                   view.getDOMNode().querySelector(".remote");
+          }));
+      }
+
+      it("should dispatch a `SetupStreamElements` action when the MEDIA_WAIT state " +
+        "is entered", function() {
+          activeRoomStore.setStoreState({roomState: ROOM_STATES.READY});
+          var view = mountTestComponent();
+
+          activeRoomStore.setStoreState({roomState: ROOM_STATES.MEDIA_WAIT});
+
+          expectActionDispatched(view);
+        });
+
+      it("should dispatch a `SetupStreamElements` action on MEDIA_WAIT state is " +
+        "re-entered", function() {
+          activeRoomStore.setStoreState({roomState: ROOM_STATES.ENDED});
+          var view = mountTestComponent();
+
+          activeRoomStore.setStoreState({roomState: ROOM_STATES.MEDIA_WAIT});
+
+          expectActionDispatched(view);
+        });
+    });
+
     describe("#render", function() {
       it("should set document.title to store.serverData.roomName", function() {
         mountTestComponent();
 
         activeRoomStore.setStoreState({roomName: "fakeName"});
 
         expect(fakeWindow.document.title).to.equal("fakeName");
       });