Closed Bug 1131574 Opened 9 years ago Closed 9 years ago

Replace the stream when the active tab switches

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
5

Tracking

(firefox38 verified, firefox39 verified)

VERIFIED FIXED
mozilla39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified
backlog backlog+

People

(Reporter: mikedeboer, Assigned: standard8)

References

(Depends on 1 open bug)

Details

(Whiteboard: [screensharing])

Attachments

(1 file, 3 obsolete files)

Whilst sharing a browser window, aka tab sharing, the user may switch between tabs. When this happens, the current stream should be replaced with a new one obtained from the tab-source-provider that captures the window inside the newly activated tab.

To prevent flickering and excessive stream switching, possibly causing lags or hangs, a short timeout should be implemented to make sure we may only replace a stream every X milliseconds.
triaging to blocking-loop:Fx38+ for tracking this release, all bugs in 38+ and 38? now, will move to "backlog+" for Fx39.  After that - we won't triage to a specific release any longer.  we will work from an ordered backlog.
backlog: --- → Fx38+
Target Milestone: mozilla38 → ---
Flags: qe-verify+
Flags: firefox-backlog+
Jan-Ivar, I'd like to go through the implementation of this bug with you!

I'd like to make sure that the stream switching happens in (privileged) chrome code, not relying on content. So what needs to happen is that when the user switches tabs, the tab-source-service component - which will implement nsIObserver - will be notified and it can swap out the contentWindow objects by itself. Can it do that currently? Do we need any additional steps to make this work?

I'd also be more than happy to discuss this over IRC - mikedeboer in #media - and I'll record the outcome here after that. Thanks in advance for your help!
Flags: needinfo?(jib)
See Also: → 1121043
Covered on irc. In short, sounds like the path for using replaceTrack is being paved, so that approach should work and line up with Hello UX needs.
Flags: needinfo?(jib)
Whiteboard: [screensharing]
backlog: Fx38+ → backlog+
Rank: 2
Assignee: nobody → standard8
Iteration: --- → 39.1 - 9 Mar
So this bug depends on bug 1133493. In my view, a couple of things should change for this to work:

 - `getActiveTabWindowId` in MozLoopAPI.jsm should be replaced by two methods: `startScreenShare` and `endScreenshare`. `startScreenShare` will lookup the foremost browser window and call a new method `window.LoopUI.startScreenShare`. A weak reference to that window should be kept to make `endScreenShare` work correctly.
 - `LoopUI.startScreenShare` will accept a callback to which it'll pass the window ID retrieved via the existing async content script API. It will also start listening to tab visibility changes once the initial window ID is retrieved.
 - Each time the tab switches, a notification will be fired with the new window ID. It's important to implement a debounce mechanism using `setTimeout()` to prevent jarring UX or perhaps even an unresponsive script when the user switches tabs erratically. This event will be passed down to the conversation window, which can then invoke `switchAcquiredWindow` on the SDK to replace the current track.
 - NOTE: We could also skip the callback mechanism and rely on the notifications only. This might simplify the code somewhat.
 - `endScreenShare` in MozLoopAPI.jsm should check for a valid WeakRef to the window that's sharing its tabs. On that window, `window.LoopUI.endScreenShare()` should be called so that it can clean up its reference to the active tab and stop listening to tab visibility changes.

This flow on does something extra for a videoSource of 'browser'. 'window' sharing stays deliciously simple.

What do you think, Mark?
Status: NEW → ASSIGNED
Depends on: 1133493
Flags: needinfo?(standard8)
Blocks: 1135045
Here's the majority of the patch, its fairly tidy now, so I think its time for some feedback. There's a few bits todo still:

- Write a mechanism in MozLoopAPI to properly remove the listener with the correct window in the case that someone's opened a new one or two.
- Finish writing tests
- I'm considering adding an extra state to SCREEN_SHARE_STATES - the patch introduces a period of time where a screen share might be starting up, but hasn't quite completed and a new windowId comes in. I think this has a risk of getting lost or causing errors.
- We probably also want some form of buffering (as mentioned previously), to prevent lots of tab switches from being queued up. I think the easiest way to do this might be to listen to the replaceTrack response (once exposed), and not accept new windowIds until we get that response back - at which time, we use the latest windowId. I might split this out to a separate bug.
Attachment #8570486 - Flags: feedback?(mdeboer)
Flags: needinfo?(standard8)
Comment on attachment 8570486 [details] [diff] [review]
WIP In Loop's tab sharing, make the shared tab follow the active tab.

Review of attachment 8570486 [details] [diff] [review]:
-----------------------------------------------------------------

First part of feedback. Rest comes after lunch! :)

::: browser/base/content/browser-loop.js
@@ +349,5 @@
> +     *
> +     * @param {Function} callback The callback to call once the window id is obtained.
> +     */
> +    _getWindowIdForSharing: function(callback) {
> +      let browser = gBrowser.selectedTab.linkedBrowser;

Ok so now that we're splitting this out, it's a good time to simply do the following (per Mike Conley's suggestion):

```js
let windowId = gBrowser.selectedTab.linkedBrowser.outerWindowID;
```

This will also cancel out the need for an additional screen sharing state while waiting until the window ID gets back from content.

@@ +373,5 @@
> +     *                            windowId changes.
> +     */
> +    addBrowserSharingListener: function(listener) {
> +      if (!this.tabChangeListeners) {
> +        this.tabChangeListeners = [];

Please make this a Set - this ensures that you only add unique listener Functions and makes it easy to iterate.

@@ +410,5 @@
> +    /**
> +     * Handles events from gBrowser.
> +     */
> +    handleEvent: function(event) {
> +      // We only should get pageshow events.

nit: this is now 'select'

@@ +417,5 @@
> +      }
> +
> +      // We've changed the tab, so get the new window id.
> +      this._getWindowIdForSharing(windowId => {
> +        this.tabChangeListeners.forEach(listener => {

With this being a Set, please use for...of
Attachment #8570486 - Flags: feedback?(mdeboer) → feedback+
WIP In Loop's tab sharing, make the shared tab follow the active tab v2
Attachment #8570486 - Attachment is obsolete: true
Comment on attachment 8571300 [details] [diff] [review]
In Loop's tab sharing, make the shared tab follow the active tab.

Review of attachment 8571300 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-loop.js
@@ +355,5 @@
> +     *                            windowId changes.
> +     */
> +    addBrowserSharingListener: function(listener) {
> +      if (!this.tabChangeListeners) {
> +        this.tabChangeListeners = new Set();

Please prefix this with an underscore to indicate it's a private member.

@@ +379,5 @@
> +      if (this.tabChangeListeners.has(listener)) {
> +        this.tabChangeListeners.delete(listener);
> +      }
> +
> +      if (this.tabChangeListeners == 0) {

.size

::: browser/components/loop/MozLoopAPI.jsm
@@ +273,5 @@
> +     * time there is a change of tab.
> +     *
> +     * Listener parameters:
> +     * - {Object}  err      If there is a error this will be defined, null otherwise.
> +     * - {Integer} windowId The new windowId after a change of tab.

This does not seem correct anymore, as you consistently pass in one parameter to the listener function.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +454,5 @@
>       * Ends an active screenshare session.
>       */
>      endScreenShare: function() {
>        if (this._sdkDriver.endScreenShare()) {
> +        if (this.getStoreState().screenSharingVideoSource === "browser") {

I think it'd be simpler to just change this check to `if (this._browserSharingListener) {`

You can even move it out of the conditional for the call into the SDK driver.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +102,5 @@
>      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;

Please prefix this with an underscore to indicate it's a private member.

::: browser/components/loop/test/mochitest/browser_sharingListeners.js
@@ +99,5 @@
> +
> +  let newWindowId0 = handlers[0].windowId;
> +  let newWindowId1 = handlers[1].windowId;
> +
> +  Assert.equal(newWindowId0, newWindowId1, "Listeners should have got the same windowId");

nit: message doesn't really 'flow'.

@@ +100,5 @@
> +  let newWindowId0 = handlers[0].windowId;
> +  let newWindowId1 = handlers[1].windowId;
> +
> +  Assert.equal(newWindowId0, newWindowId1, "Listeners should have got the same windowId");
> +  Assert.notEqual(initialWindowId0, newWindowId0, "Tab contentWindow IDs for shouldn't be the same");

nit: message doesn't really 'flow'.

@@ +114,5 @@
> +
> +  Assert.equal(newWindowId0, nextWindowId0, "First listener shouldn't have updated");
> +  Assert.notEqual(newWindowId1, nextWindowId1, "Second listener should have updated");
> +
> +  // Cleanup

nit: missing dot.
One additional thing to keep in mind is tearing a tab; a tab can be torn off a window to its own _new_ window, which would break all references in mozLoopAPI to the window object it _assumes_ the tab to be part of.
This is good fodder to break off to its own bug, but still something to consider! I say 'own bug', because tab tearing was already rather difficult to get working with e10s enabled, so I assume that the amount of work for this is considerable. Additionally, I consider tearing a tab off during browser sharing an edge case, so there's room for debate wrt priority.
Updated to include comments and the missing tests.
Attachment #8571300 - Attachment is obsolete: true
Attachment #8571348 - Flags: review?(mdeboer)
Comment on attachment 8571348 [details] [diff] [review]
In Loop's tab sharing, make the shared tab follow the active tab.

Review of attachment 8571348 [details] [diff] [review]:
-----------------------------------------------------------------

Getting close! Good stuff :)

There are a couple of things that could be improved, please take a look at my comments below:

::: browser/components/loop/MozLoopAPI.jsm
@@ +298,5 @@
> +          savedWindowListeners = [];
> +        }
> +
> +        savedWindowListeners.push({
> +          listener: listener,

Please use a Map here... you can use the `listener` function as the key!

@@ +319,5 @@
>          });
> +
> +        if (index == -1) {
> +          return;
> +        }

By using a Map you can just do:

```js
if (!savedWindowListeners.has(listener)) {
  return;
}

let win = savedWindowListeners.get(listener).get();

savedWindowListeners.delete(listener);

...
```

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +73,5 @@
>          used: false,
>          localVideoDimensions: {},
>          remoteVideoDimensions: {},
>          screenSharingState: SCREEN_SHARE_STATES.INACTIVE,
> +        screenSharingVideoSource: "",

It doesn't look like you're using this store property... can you remove it?

@@ +416,5 @@
> +          videoSource: "browser",
> +          constraints: {
> +            browserWindow: windowId,
> +            scrollWithPage: true,
> +          },

nit: trailing comma

@@ +449,5 @@
> +        // 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);

nit: I think this'll fit on one line, since the longer one in `endScreenShare` fits too.

::: browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js
@@ +1,5 @@
> +/* 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

nit: missing dot.

@@ +52,5 @@
> +  return Promise.all(promiseHandlers);
> +};
> +
> +function removeTabs() {
> +  createdTabs.forEach(createdTab => {

Please use for...of

@@ +73,5 @@
> +  let newWindowId = handlers[0].windowId;
> +
> +  Assert.notEqual(initialWindowId, newWindowId, "Tab contentWindow IDs shouldn't be the same");
> +
> +  // Now remove the listener

nit: missing dot.

@@ +102,5 @@
> +
> +  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

nit: missing dot.

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +754,5 @@
> +        type: "browser"
> +      }));
> +
> +      // Listener is the first argument of the first call.
> +      listener = fakeMozLoop.addBrowserSharingListener.args[0][0];

What are you going to do with this reference to the listener?
Attachment #8571348 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> ::: browser/components/loop/test/shared/activeRoomStore_test.js
> @@ +754,5 @@
> > +        type: "browser"
> > +      }));
> > +
> > +      // Listener is the first argument of the first call.
> > +      listener = fakeMozLoop.addBrowserSharingListener.args[0][0];
> 
> What are you going to do with this reference to the listener?

It gets used in the test below it (this code is in the beforeEach section).
Updated for comments.
Attachment #8571348 - Attachment is obsolete: true
Attachment #8571417 - Flags: review?(mdeboer)
Comment on attachment 8571417 [details] [diff] [review]
In Loop's tab sharing, make the shared tab follow the active tab.

Review of attachment 8571417 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! LGTM.

Can you also file the follow-up bugs for tab switching de-bounce mechanism and tab tearing whilst browser sharing?

::: browser/base/content/browser-loop.js
@@ +357,5 @@
> +     */
> +    addBrowserSharingListener: function(listener) {
> +      if (!this._tabChangeListeners) {
> +        this._tabChangeListeners = new Set();
> +        gBrowser.addEventListener("select", this, false);

`false` is the default, so there's no need to specify it here.

@@ +380,5 @@
> +      if (this._tabChangeListeners.has(listener)) {
> +        this._tabChangeListeners.delete(listener);
> +      }
> +
> +      if (this._tabChangeListeners.size == 0) {

nit: you can simplify this to `if (!this._tabChangeListeners.size) {`

::: browser/components/loop/MozLoopAPI.jsm
@@ +313,5 @@
> +
> +        let win = savedWindowListeners.get(listener).get();
> +
> +        // Remove the element, regardless of if the window exists or not so
> +        // that we clean the array.

nit: s/array/map

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +398,5 @@
>      /**
> +     * Handles switching browser (aka tab) sharing to a new window. Should
> +     * only be used for browser sharing.
> +     *
> +     * @param {Integer} windowId  The new windowId to start sharing.

s/Integer/Number

::: browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Please add "use strict";
Attachment #8571417 - Flags: review?(mdeboer) → review+
Landed with comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/aebb5e9855dd
Target Milestone: --- → mozilla39
There was an issue with the test in e10s mode, so I've disabled it for now and filed bug 1138638 to handle it:

https://hg.mozilla.org/integration/fx-team/rev/aed265aff73d
QA Contact: bogdan.maris
Depends on: 1139471
Comment on attachment 8571417 [details] [diff] [review]
In Loop's tab sharing, make the shared tab follow the active tab.

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello screensharing milestone
[User impact if declined]: User will see a new option that allows her/ him to share a window or Firefox tabs inside a room (aka. conversation).
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a
Attachment #8571417 - Flags: approval-mozilla-aurora?
Attachment #8571417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1143623
Depends on: 1144782
Verified that during a tab sharing stream, switching the tab also switches tabs on stream. Tested on latest Nightly and latest Aurora across all platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: