From 7f5f655174e9632f52e749328381dda944cb3a5f Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Fri, 24 Jul 2015 17:25:34 +0100 Subject: [PATCH] Bug 1187309 - Simplify the no-camera work around for Loop that was put in place when we didn't have device enumeration - avoid unnecessary exceptions from the sdk. r=mikedeboer --- .../loop/content/shared/js/activeRoomStore.js | 15 ------ .../content/shared/js/conversationStore.js | 15 ------ .../loop/content/shared/js/otSdkDriver.js | 48 ++++++++----------- .../loop/test/shared/activeRoomStore_test.js | 20 -------- .../test/shared/conversationStore_test.js | 20 -------- .../loop/test/shared/otSdkDriver_test.js | 37 -------------- 6 files changed, 20 insertions(+), 135 deletions(-) diff --git a/browser/components/loop/content/shared/js/activeRoomStore.js b/browser/components/loop/content/shared/js/activeRoomStore.js index 62eb8bbd3390..835c64ee7092 100644 --- a/browser/components/loop/content/shared/js/activeRoomStore.js +++ b/browser/components/loop/content/shared/js/activeRoomStore.js @@ -580,21 +580,6 @@ loop.store.ActiveRoomStore = (function() { * @param {sharedActions.ConnectionFailure} actionData */ connectionFailure: function(actionData) { - /** - * XXX This is a workaround for desktop machines that do not have a - * camera installed. As we don't yet have device enumeration, when - * we do, this can be removed (bug 1138851), and the sdk should handle it. - */ - if (this._isDesktop && - actionData.reason === FAILURE_DETAILS.UNABLE_TO_PUBLISH_MEDIA && - this.getStoreState().videoMuted === false) { - // We failed to publish with media, so due to the bug, we try again without - // video. - this.setStoreState({videoMuted: true}); - this._sdkDriver.retryPublishWithoutVideo(); - return; - } - var exitState = this._storeState.roomState === ROOM_STATES.FAILED ? this._storeState.failureExitState : this._storeState.roomState; diff --git a/browser/components/loop/content/shared/js/conversationStore.js b/browser/components/loop/content/shared/js/conversationStore.js index 211972c055b5..6d99673b1847 100644 --- a/browser/components/loop/content/shared/js/conversationStore.js +++ b/browser/components/loop/content/shared/js/conversationStore.js @@ -146,21 +146,6 @@ loop.store = loop.store || {}; * @param {sharedActions.ConnectionFailure} actionData The action data. */ connectionFailure: function(actionData) { - /** - * XXX This is a workaround for desktop machines that do not have a - * camera installed. As we don't yet have device enumeration, when - * we do, this can be removed (bug 1138851), and the sdk should handle it. - */ - if (this._isDesktop && - actionData.reason === FAILURE_DETAILS.UNABLE_TO_PUBLISH_MEDIA && - this.getStoreState().videoMuted === false) { - // We failed to publish with media, so due to the bug, we try again without - // video. - this.setStoreState({videoMuted: true}); - this.sdkDriver.retryPublishWithoutVideo(); - return; - } - this._endSession(); this.setStoreState({ callState: CALL_STATES.TERMINATED, diff --git a/browser/components/loop/content/shared/js/otSdkDriver.js b/browser/components/loop/content/shared/js/otSdkDriver.js index 994d2e2ab137..9b337dd70fd1 100644 --- a/browser/components/loop/content/shared/js/otSdkDriver.js +++ b/browser/components/loop/content/shared/js/otSdkDriver.js @@ -61,15 +61,26 @@ loop.OTSdkDriver = (function() { /** * XXX This is a workaround for desktop machines that do not have a - * camera installed. As we don't yet have device enumeration, when - * we do, this can be removed (bug 1138851), and the sdk should handle it. + * camera installed. The SDK doesn't currently do use the new device + * enumeration apis, when it does (bug 1138851), we can drop this part. */ - if (this._isDesktop && !window.MediaStreamTrack.getSources) { + if (this._isDesktop) { // If there's no getSources function, the sdk defines its own and caches - // the result. So here we define the "normal" one which doesn't get cached, so - // we can change it later. + // the result. So here we define our own one which wraps around the + // real device enumeration api. window.MediaStreamTrack.getSources = function(callback) { - callback([{kind: "audio"}, {kind: "video"}]); + navigator.mediaDevices.enumerateDevices().then(function(devices) { + var result = []; + devices.forEach(function(device) { + if (device.kind === "audioinput") { + result.push({kind: "audio"}); + } + if (device.kind === "videoinput") { + result.push({kind: "video"}); + } + }); + callback(result); + }); }; } }; @@ -109,21 +120,13 @@ loop.OTSdkDriver = (function() { this.sdk.on("exception", this._onOTException.bind(this)); - // At this state we init the publisher, even though we might be waiting for - // the initial connect of the session. This saves time when setting up - // the media. - this._publishLocalStreams(); - }, - - /** - * Internal function to publish a local stream. - * XXX This can be simplified when bug 1138851 is actioned. - */ - _publishLocalStreams: function() { // We expect the local video to be muted automatically by the SDK. Hence // we don't mute it manually here. this._mockPublisherEl = document.createElement("div"); + // At this state we init the publisher, even though we might be waiting for + // the initial connect of the session. This saves time when setting up + // the media. this.publisher = this.sdk.initPublisher(this._mockPublisherEl, _.extend(this._getDataChannelSettings, this._getCopyPublisherConfig)); @@ -135,17 +138,6 @@ loop.OTSdkDriver = (function() { this._onAccessDialogOpened.bind(this)); }, - /** - * Forces the sdk into not using video, and starts publishing again. - * XXX This is part of the work around that will be removed by bug 1138851. - */ - retryPublishWithoutVideo: function() { - window.MediaStreamTrack.getSources = function(callback) { - callback([{kind: "audio"}]); - }; - this._publishLocalStreams(); - }, - /** * Handles the setMute action. Informs the published stream to mute * or unmute audio as appropriate. diff --git a/browser/components/loop/test/shared/activeRoomStore_test.js b/browser/components/loop/test/shared/activeRoomStore_test.js index 1b8d11d7cfda..55f7bb96b3a3 100644 --- a/browser/components/loop/test/shared/activeRoomStore_test.js +++ b/browser/components/loop/test/shared/activeRoomStore_test.js @@ -916,26 +916,6 @@ describe("loop.store.ActiveRoomStore", function () { }); }); - it("should retry publishing if on desktop, and in the videoMuted state", function() { - store._isDesktop = true; - - store.connectionFailure(new sharedActions.ConnectionFailure({ - reason: FAILURE_DETAILS.UNABLE_TO_PUBLISH_MEDIA - })); - - sinon.assert.calledOnce(fakeSdkDriver.retryPublishWithoutVideo); - }); - - it("should set videoMuted to try when retrying publishing", function() { - store._isDesktop = true; - - store.connectionFailure(new sharedActions.ConnectionFailure({ - reason: FAILURE_DETAILS.UNABLE_TO_PUBLISH_MEDIA - })); - - expect(store.getStoreState().videoMuted).eql(true); - }); - it("should store the failure reason", function() { store.connectionFailure(connectionFailureAction); diff --git a/browser/components/loop/test/shared/conversationStore_test.js b/browser/components/loop/test/shared/conversationStore_test.js index d696e784970c..f8dbbc9a9f75 100644 --- a/browser/components/loop/test/shared/conversationStore_test.js +++ b/browser/components/loop/test/shared/conversationStore_test.js @@ -147,26 +147,6 @@ describe("loop.store.ConversationStore", function () { store.setStoreState({windowId: "42"}); }); - it("should retry publishing if on desktop, and in the videoMuted state", function() { - store._isDesktop = true; - - store.connectionFailure(new sharedActions.ConnectionFailure({ - reason: FAILURE_DETAILS.UNABLE_TO_PUBLISH_MEDIA - })); - - sinon.assert.calledOnce(sdkDriver.retryPublishWithoutVideo); - }); - - it("should set videoMuted to try when retrying publishing", function() { - store._isDesktop = true; - - store.connectionFailure(new sharedActions.ConnectionFailure({ - reason: FAILURE_DETAILS.UNABLE_TO_PUBLISH_MEDIA - })); - - expect(store.getStoreState().videoMuted).eql(true); - }); - it("should disconnect the session", function() { store.connectionFailure( new sharedActions.ConnectionFailure({reason: "fake"})); diff --git a/browser/components/loop/test/shared/otSdkDriver_test.js b/browser/components/loop/test/shared/otSdkDriver_test.js index 0706e83edd84..c31febdc2fd9 100644 --- a/browser/components/loop/test/shared/otSdkDriver_test.js +++ b/browser/components/loop/test/shared/otSdkDriver_test.js @@ -133,43 +133,6 @@ describe("loop.OTSdkDriver", function () { }); }); - describe("#retryPublishWithoutVideo", function() { - beforeEach(function() { - sdk.initPublisher.returns(publisher); - - driver.setupStreamElements(new sharedActions.SetupStreamElements({ - publisherConfig: publisherConfig - })); - }); - - it("should make MediaStreamTrack.getSources return without a video source", function(done) { - driver.retryPublishWithoutVideo(); - - window.MediaStreamTrack.getSources(function(sources) { - expect(sources.some(function(src) { - return src.kind === "video"; - })).eql(false); - - done(); - }); - }); - - it("should call initPublisher", function() { - driver.retryPublishWithoutVideo(); - - var expectedConfig = _.extend({ - channels: { - text: {} - } - }, publisherConfig); - - sinon.assert.calledTwice(sdk.initPublisher); - sinon.assert.calledWith(sdk.initPublisher, - sinon.match.instanceOf(HTMLDivElement), - expectedConfig); - }); - }); - describe("#setMute", function() { beforeEach(function() { sdk.initPublisher.returns(publisher);