Bug 1131574 - In Loop's tab sharing, make the shared tab follow the active tab. r=mikedeboer

browser/base/content/browser-loop.js

@@ -343,5 +343,63 @@ XPCOMUtils.defineLazyModuleGetter(this, "PanelFrame", "resource:///modules/Panel
 
       this.activeSound.addEventListener("ended", () => this.activeSound = undefined, false);
     },
+
+    /**
+     * Adds a listener for browser sharing. It will inform the listener straight
+     * away for the current windowId, and then on every tab change.
+     *
+     * Listener parameters:
+     * - {Object}  err       If there is a error this will be defined, null otherwise.
+     * - {Integer} windowId  The new windowId for the browser.
+     *
+     * @param {Function} listener The listener to receive information on when the
+     *                            windowId changes.
+     */
+    addBrowserSharingListener: function(listener) {
+      if (!this._tabChangeListeners) {
+        this._tabChangeListeners = new Set();
+        gBrowser.addEventListener("select", this);
+      }
+
+      this._tabChangeListeners.add(listener);
+
+      // Get the first window Id for the listener.
+      listener(null, gBrowser.selectedTab.linkedBrowser.outerWindowID);
+    },
+
+    /**
+     * Removes a listener from browser sharing.
+     *
+     * @param {Function} listener The listener to remove from the list.
+     */
+    removeBrowserSharingListener: function(listener) {
+      if (!this._tabChangeListeners) {
+        return;
+      }
+
+      if (this._tabChangeListeners.has(listener)) {
+        this._tabChangeListeners.delete(listener);
+      }
+
+      if (!this._tabChangeListeners.size) {
+        gBrowser.removeEventListener("select", this);
+        delete this._tabChangeListeners;
+      }
+    },
+
+    /**
+     * Handles events from gBrowser.
+     */
+    handleEvent: function(event) {
+      // We only should get "select" events.
+      if (event.type != "select") {
+        return;
+      }
+
+      // We've changed the tab, so get the new window id.
+      for (let listener of this._tabChangeListeners) {
+        listener(null, gBrowser.selectedTab.linkedBrowser.outerWindowID);
+      };
+    },
   };
 })();

browser/components/loop/MozLoopAPI.jsm

@@ -196,6 +196,7 @@ function injectLoopAPI(targetWindow) {
   let contactsAPI;
   let roomsAPI;
   let callsAPI;
+  let savedWindowListeners = new Map();
 
   let api = {
     /**
@@ -266,10 +267,21 @@ function injectLoopAPI(targetWindow) {
       }
     },
 
-    getActiveTabWindowId: {
+    /**
+     * Adds a listener to the most recent window for browser/tab sharing. The
+     * listener will be notified straight away of the current tab id, then every
+     * time there is a change of tab.
+     *
+     * Listener parameters:
+     * - {Object}  err      If there is a error this will be defined, null otherwise.
+     * - {Number} windowId The new windowId after a change of tab.
+     *
+     * @param {Function} listener The listener to handle the windowId changes.
+     */
+    addBrowserSharingListener: {
       enumerable: true,
       writable: true,
-      value: function(callback) {
+      value: function(listener) {
         let win = Services.wm.getMostRecentWindow("navigator:browser");
         let browser = win && win.gBrowser.selectedTab.linkedBrowser;
         if (!win || !browser) {
@@ -277,16 +289,39 @@ function injectLoopAPI(targetWindow) {
           // window left.
           let err = new Error("No tabs available to share.");
           MozLoopService.log.error(err);
-          callback(cloneValueInto(err, targetWindow));
+          listener(cloneValueInto(err, targetWindow));
+          return;
+        }
+        win.LoopUI.addBrowserSharingListener(listener);
+
+        savedWindowListeners.set(listener, Cu.getWeakReference(win));
+      }
+    },
+
+    /**
+     * Removes a listener that was previously added.
+     *
+     * @param {Function} listener The listener to handle the windowId changes.
+     */
+    removeBrowserSharingListener: {
+      enumerable: true,
+      writable: true,
+      value: function(listener) {
+        if (!savedWindowListeners.has(listener)) {
           return;
         }
 
-        let mm = browser.messageManager;
-        mm.addMessageListener("webrtc:response:StartBrowserSharing", function listener(message) {
-          mm.removeMessageListener("webrtc:response:StartBrowserSharing", listener);
-          callback(null, message.data.windowID);
-        });
-        mm.sendAsyncMessage("webrtc:StartBrowserSharing");
+        let win = savedWindowListeners.get(listener).get();
+
+        // Remove the element, regardless of if the window exists or not so
+        // that we clean the map.
+        savedWindowListeners.delete(listener);
+
+        if (!win) {
+          return;
+        }
+
+        win.LoopUI.removeBrowserSharingListener(listener);
       }
     },
 

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

@@ -395,6 +395,38 @@ loop.store.ActiveRoomStore = (function() {
       this.setStoreState({receivingScreenShare: actionData.receiving});
     },
 
+    /**
+     * Handles switching browser (aka tab) sharing to a new window. Should
+     * only be used for browser sharing.
+     *
+     * @param {Number} windowId  The new windowId to start sharing.
+     */
+    _handleSwitchBrowserShare: function(err, windowId) {
+      if (err) {
+        console.error("Error getting the windowId: " + err);
+        return;
+      }
+
+      var screenSharingState = this.getStoreState().screenSharingState;
+
+      if (screenSharingState === SCREEN_SHARE_STATES.INACTIVE) {
+        // Screen sharing is still pending, so assume that we need to kick it off.
+        var options = {
+          videoSource: "browser",
+          constraints: {
+            browserWindow: windowId,
+            scrollWithPage: true
+          },
+        };
+        this._sdkDriver.startScreenShare(options);
+      } else if (screenSharingState === SCREEN_SHARE_STATES.ACTIVE) {
+        // Just update the current share.
+        this._sdkDriver.switchAcquiredWindow(windowId);
+      } else {
+        console.error("Unexpectedly received windowId for browser sharing when pending");
+      }
+    },
+
     /**
      * Initiates a screen sharing publisher.
      *
@@ -409,19 +441,12 @@ loop.store.ActiveRoomStore = (function() {
         videoSource: actionData.type
       };
       if (options.videoSource === "browser") {
-        this._mozLoop.getActiveTabWindowId(function(err, windowId) {
-          if (err || !windowId) {
-            this.dispatchAction(new sharedActions.ScreenSharingState({
-              state: SCREEN_SHARE_STATES.INACTIVE
-            }));
-            return;
-          }
-          options.constraints = {
-            browserWindow: windowId,
-            scrollWithPage: true
-          };
-          this._sdkDriver.startScreenShare(options);
-        }.bind(this));
+        this._browserSharingListener = this._handleSwitchBrowserShare.bind(this);
+
+        // Set up a listener for watching screen shares. This will get notified
+        // with the first windowId when it is added, so we start off the sharing
+        // from within the listener.
+        this._mozLoop.addBrowserSharingListener(this._browserSharingListener);
       } else {
         this._sdkDriver.startScreenShare(options);
       }
@@ -431,6 +456,12 @@ loop.store.ActiveRoomStore = (function() {
      * Ends an active screenshare session.
      */
     endScreenShare: function() {
+      if (this._browserSharingListener) {
+        // Remove the browser sharing listener as we don't need it now.
+        this._mozLoop.removeBrowserSharingListener(this._browserSharingListener);
+        this._browserSharingListener = null;
+      }
+
       if (this._sdkDriver.endScreenShare()) {
         this.dispatchAction(new sharedActions.ScreenSharingState({
           state: SCREEN_SHARE_STATES.INACTIVE

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

@@ -100,6 +100,12 @@ loop.OTSdkDriver = (function() {
      * @param {Object} options Hash containing options for the SDK
      */
     startScreenShare: function(options) {
+      // For browser sharing, we store the window Id so that we can avoid unnecessary
+      // re-triggers.
+      if (options.videoSource === "browser") {
+        this._windowId = options.constraints.browserWindow;
+      }
+
       var config = _.extend(this._getCopyPublisherConfig(), options);
 
       this.screenshare = this.sdk.initPublisher(this.getScreenShareElementFunc(),
@@ -108,6 +114,20 @@ loop.OTSdkDriver = (function() {
       this.screenshare.on("accessDenied", this._onScreenShareDenied.bind(this));
     },
 
+    /**
+     * Initiates switching the browser window that is being shared.
+     *
+     * @param {Integer} windowId  The windowId of the browser.
+     */
+    switchAcquiredWindow: function(windowId) {
+      if (windowId === this._windowId) {
+        return;
+      }
+
+      this._windowId = windowId;
+      this.screenshare._.switchAcquiredWindow(windowId);
+    },
+
     /**
      * Ends an active screenshare session. Return `true` when an active screen-
      * sharing session was ended or `false` when no session is active.
@@ -123,6 +143,7 @@ loop.OTSdkDriver = (function() {
       this.screenshare.off("accessAllowed accessDenied");
       this.screenshare.destroy();
       delete this.screenshare;
+      delete this._windowId;
       return true;
     },
 

browser/components/loop/test/mochitest/browser.ini

@@ -19,8 +19,8 @@ skip-if = e10s
 [browser_mozLoop_prefs.js]
 [browser_mozLoop_doNotDisturb.js]
 skip-if = buildapp == 'mulet'
-[browser_toolbarbutton.js]
 [browser_mozLoop_pluralStrings.js]
-[browser_mozLoop_tabSharing.js]
+[browser_mozLoop_sharingListeners.js]
 [browser_mozLoop_telemetry.js]
 skip-if = e10s
+[browser_toolbarbutton.js]

browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js

@@ -0,0 +1,125 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/*
+ * This file contains tests for the window.LoopUI active tab trackers.
+ */
+"use strict";
+
+const {injectLoopAPI} = Cu.import("resource:///modules/loop/MozLoopAPI.jsm");
+gMozLoopAPI = injectLoopAPI({});
+
+let handlers = [
+  {
+    resolve: null,
+    windowId: null,
+    listener: function(err, windowId) {
+      handlers[0].windowId = windowId;
+      handlers[0].resolve();
+    }
+  },
+  {
+    resolve: null,
+    windowId: null,
+    listener: function(err, windowId) {
+      handlers[1].windowId = windowId;
+      handlers[1].resolve();
+    }
+  }
+];
+
+function promiseWindowIdReceivedOnAdd(handler) {
+  return new Promise(resolve => {
+    handler.resolve = resolve;
+    gMozLoopAPI.addBrowserSharingListener(handler.listener);
+  });
+};
+
+let createdTabs = [];
+
+function promiseWindowIdReceivedNewTab(handlers) {
+  let promiseHandlers = [];
+
+  handlers.forEach(handler => {
+    promiseHandlers.push(new Promise(resolve => {
+      handler.resolve = resolve;
+    }));
+  });
+
+  let createdTab = gBrowser.selectedTab = gBrowser.addTab();
+  createdTabs.push(createdTab);
+
+  promiseHandlers.push(promiseTabLoadEvent(createdTab, "about:mozilla"));
+
+  return Promise.all(promiseHandlers);
+};
+
+function removeTabs() {
+  for (let createdTab of createdTabs) {
+    gBrowser.removeTab(createdTab);
+  }
+
+  createdTabs = [];
+}
+
+add_task(function* test_singleListener() {
+  yield promiseWindowIdReceivedOnAdd(handlers[0]);
+
+  let initialWindowId = handlers[0].windowId;
+
+  Assert.notEqual(initialWindowId, null, "window id should be valid");
+
+  // Check that a new tab updates the window id.
+  yield promiseWindowIdReceivedNewTab([handlers[0]]);
+
+  let newWindowId = handlers[0].windowId;
+
+  Assert.notEqual(initialWindowId, newWindowId, "Tab contentWindow IDs shouldn't be the same");
+
+  // Now remove the listener.
+  gMozLoopAPI.removeBrowserSharingListener(handlers[0].listener);
+
+  removeTabs();
+});
+
+add_task(function* test_multipleListener() {
+  yield promiseWindowIdReceivedOnAdd(handlers[0]);
+
+  let initialWindowId0 = handlers[0].windowId;
+
+  Assert.notEqual(initialWindowId0, null, "window id should be valid");
+
+  yield promiseWindowIdReceivedOnAdd(handlers[1]);
+
+  let initialWindowId1 = handlers[1].windowId;
+
+  Assert.notEqual(initialWindowId1, null, "window id should be valid");
+  Assert.equal(initialWindowId0, initialWindowId1, "window ids should be the same");
+
+  // Check that a new tab updates the window id.
+  yield promiseWindowIdReceivedNewTab(handlers);
+
+  let newWindowId0 = handlers[0].windowId;
+  let newWindowId1 = handlers[1].windowId;
+
+  Assert.equal(newWindowId0, newWindowId1, "Listeners should have the same windowId");
+  Assert.notEqual(initialWindowId0, newWindowId0, "Tab contentWindow IDs shouldn't be the same");
+
+  // Now remove the first listener.
+  gMozLoopAPI.removeBrowserSharingListener(handlers[0].listener);
+
+  // Check that a new tab updates the window id.
+  yield promiseWindowIdReceivedNewTab([handlers[1]]);
+
+  let nextWindowId0 = handlers[0].windowId;
+  let nextWindowId1 = handlers[1].windowId;
+
+  Assert.equal(newWindowId0, nextWindowId0, "First listener shouldn't have updated");
+  Assert.notEqual(newWindowId1, nextWindowId1, "Second listener should have updated");
+
+  // Cleanup.
+  gMozLoopAPI.removeBrowserSharingListener(handlers[1].listener);
+
+  removeTabs();
+});
+

browser/components/loop/test/mochitest/browser_mozLoop_tabSharing.js

@@ -1,41 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-/**
- * This is an integration test to make sure that passing window IDs is working as
- * expected, with or without e10s enabled - rather than just testing MozLoopAPI
- * alone.
- */
-
-const {injectLoopAPI} = Cu.import("resource:///modules/loop/MozLoopAPI.jsm");
-gMozLoopAPI = injectLoopAPI({});
-
-let promiseTabWindowId = function() {
-  return new Promise(resolve => {
-    gMozLoopAPI.getActiveTabWindowId((err, windowId) => {
-      Assert.equal(null, err, "No error should've occurred.");
-      Assert.equal(typeof windowId, "number", "We should have a window ID");
-      resolve(windowId);
-    });
-  });
-};
-
-add_task(function* test_windowIdFetch_simple() {
-  Assert.ok(gMozLoopAPI, "mozLoop should exist");
-
-  yield promiseTabWindowId();
-});
-
-add_task(function* test_windowIdFetch_multipleTabs() {
-  let previousTab = gBrowser.selectedTab;
-  let previousTabId = yield promiseTabWindowId();
-
-  let tab = gBrowser.selectedTab = gBrowser.addTab();
-  yield promiseTabLoadEvent(tab, "about:mozilla");
-  let tabId = yield promiseTabWindowId();
-  Assert.ok(tabId !== previousTabId, "Tab contentWindow IDs shouldn't be the same");
-  gBrowser.removeTab(tab);
-
-  tabId = yield promiseTabWindowId();
-  Assert.equal(previousTabId, tabId, "Window IDs should be back to what they were");
-});

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

@@ -21,8 +21,10 @@ describe("loop.store.ActiveRoomStore", function () {
     sandbox.stub(dispatcher, "dispatch");
 
     fakeMozLoop = {
-      setLoopPref: sandbox.stub(),
-      addConversationContext: sandbox.stub(),
+      setLoopPref: sinon.stub(),
+      addConversationContext: sinon.stub(),
+      addBrowserSharingListener: sinon.stub(),
+      removeBrowserSharingListener: sinon.stub(),
       rooms: {
         get: sinon.stub(),
         join: sinon.stub(),
@@ -36,11 +38,12 @@ describe("loop.store.ActiveRoomStore", function () {
     };
 
     fakeSdkDriver = {
-      connectSession: sandbox.stub(),
-      disconnectSession: sandbox.stub(),
-      forceDisconnectAll: sandbox.stub().callsArg(0),
-      startScreenShare: sandbox.stub(),
-      endScreenShare: sandbox.stub().returns(true)
+      connectSession: sinon.stub(),
+      disconnectSession: sinon.stub(),
+      forceDisconnectAll: sinon.stub().callsArg(0),
+      startScreenShare: sinon.stub(),
+      switchAcquiredWindow: sinon.stub(),
+      endScreenShare: sinon.stub().returns(true)
     };
 
     fakeMultiplexGum = {
@@ -717,12 +720,20 @@ describe("loop.store.ActiveRoomStore", function () {
       });
     });
 
-    it("should invoke the SDK driver with the correct options for tab sharing", function() {
+    it("should add a browser sharing listener for tab sharing", function() {
       store.startScreenShare(new sharedActions.StartScreenShare({
         type: "browser"
       }));
 
-      sinon.assert.calledOnce(fakeMozLoop.getActiveTabWindowId);
+      sinon.assert.calledOnce(fakeMozLoop.addBrowserSharingListener);
+    });
+
+    it("should invoke the SDK driver with the correct options for tab sharing", function() {
+      fakeMozLoop.addBrowserSharingListener.callsArgWith(0, null, 42);
+
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
 
       sinon.assert.calledOnce(fakeSdkDriver.startScreenShare);
       sinon.assert.calledWith(fakeSdkDriver.startScreenShare, {
@@ -732,7 +743,31 @@ describe("loop.store.ActiveRoomStore", function () {
           scrollWithPage: true
         }
       });
-    })
+    });
+  });
+
+  describe("Screen share Events", function() {
+    var listener;
+
+    beforeEach(function() {
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
+
+      // Listener is the first argument of the first call.
+      listener = fakeMozLoop.addBrowserSharingListener.args[0][0];
+
+      store.setStoreState({
+        screenSharingState: SCREEN_SHARE_STATES.ACTIVE
+      });
+    });
+
+    it("should update the SDK driver when a new window id is received", function() {
+      listener(null, 72);
+
+      sinon.assert.calledOnce(fakeSdkDriver.switchAcquiredWindow);
+      sinon.assert.calledWithExactly(fakeSdkDriver.switchAcquiredWindow, 72);
+    });
   });
 
   describe("#endScreenShare", function() {
@@ -745,6 +780,18 @@ describe("loop.store.ActiveRoomStore", function () {
           state: SCREEN_SHARE_STATES.INACTIVE
         }));
     });
+
+    it("should remove the sharing listener", function() {
+      // Setup the listener.
+      store.startScreenShare(new sharedActions.StartScreenShare({
+        type: "browser"
+      }));
+
+      // Now stop the screen share.
+      store.endScreenShare();
+
+      sinon.assert.calledOnce(fakeMozLoop.removeBrowserSharingListener);
+    });
   });
 
   describe("#remotePeerConnected", function() {

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

@@ -46,7 +46,10 @@ describe("loop.OTSdkDriver", function () {
     publisher = _.extend({
       destroy: sinon.stub(),
       publishAudio: sinon.stub(),
-      publishVideo: sinon.stub()
+      publishVideo: sinon.stub(),
+      _: {
+        switchAcquiredWindow: sinon.stub()
+      }
     }, Backbone.Events);
 
     sdk = {
@@ -143,8 +146,10 @@ describe("loop.OTSdkDriver", function () {
       // has multiple options.
       var options = {
         videoSource: "browser",
-        browserWindow: 42,
-        scrollWithPage: true
+        constraints: {
+          browserWindow: 42,
+          scrollWithPage: true
+        }
       };
       driver.startScreenShare(options);
 
@@ -153,6 +158,37 @@ describe("loop.OTSdkDriver", function () {
     });
   });
 
+  describe("#switchAcquiredWindow", function() {
+    beforeEach(function() {
+      var options = {
+        videoSource: "browser",
+        constraints: {
+          browserWindow: 42,
+          scrollWithPage: true
+        }
+      };
+      driver.getScreenShareElementFunc = function() {
+        return fakeScreenElement;
+      };
+      sandbox.stub(dispatcher, "dispatch");
+
+      driver.startScreenShare(options);
+    });
+
+    it("should switch to the acquired window", function() {
+      driver.switchAcquiredWindow(72);
+
+      sinon.assert.calledOnce(publisher._.switchAcquiredWindow);
+      sinon.assert.calledWithExactly(publisher._.switchAcquiredWindow, 72);
+    });
+
+    it("should not switch if the window is the same as the currently selected one", function() {
+      driver.switchAcquiredWindow(42);
+
+      sinon.assert.notCalled(publisher._.switchAcquiredWindow);
+    });
+  });
+
   describe("#endScreenShare", function() {
     beforeEach(function() {
       driver.getScreenShareElementFunc = function() {};