1048425 - Enable support for tab sharing with Roku device.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Randall Barker [:rbarker]]

Fennec should be able to detect the WebRTC player channel and share a tab with the channel.

Comment 1

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8467230 [details] [diff] [review]
Enable tab sharing.

># HG changeset patch
># Parent bffb1d563196a9b38d8749cba3bae257db4f77a0
># User Randall Barker <[e-mail]>
>Bug 1048425 - Enable support for tab sharing with Roku device
>
>diff --git a/mobile/android/modules/RokuApp.jsm b/mobile/android/modules/RokuApp.jsm
>--- a/mobile/android/modules/RokuApp.jsm
>+++ b/mobile/android/modules/RokuApp.jsm
>@@ -6,35 +6,49 @@
> "use strict";
> 
> this.EXPORTED_SYMBOLS = ["RokuApp"];
> 
> const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> 
> Cu.import("resource://gre/modules/Services.jsm");
> 
>+const WEBRTC_PLAYER_NAME = "WebRTC Player";
>+
> function log(msg) {
>   //Services.console.logStringMessage(msg);
> }
> 
>+function createRemoteMirror() {
>+  if (this.mirrorAppID == -1) {
>+    // TODO: Inform user to install Roku WebRTC Player Channel.
>+  }
>+  else {
>+    // TODO: Launch the Roku WebRTC Player Channel
>+    this.mirroror = new RemoteMirror(this.resourceURL);
>+  }
>+}
>+
> const PROTOCOL_VERSION = 1;
> 
> /* RokuApp is a wrapper for interacting with a Roku channel.
>  * The basic interactions all use a REST API.
>  * spec: http://sdkdocs.roku.com/display/sdkdoc/External+Control+Guide
>  */
> function RokuApp(service) {
>   this.service = service;
>   this.resourceURL = this.service.location;
> #ifdef RELEASE_BUILD
>   this.app = "Firefox";
> #else
>   this.app = "Firefox Nightly";
> #endif
>   this.appID = -1;
>+  this.mirrorAppID = -1;
>+  this._createRemoteMirror = createRemoteMirror.bind(this);

I don't exactly follow why you need a separate function to handle creating the RemoteMirror. It's seems overly complicated. Let's try to reduce things down to at least the level of how we handle RemoteMedia, unless there is some reason we can't.

>           if (app.textContent == this.app) {
>             this.appID = app.id;
>           }
>+          else if (app.textContent == WEBRTC_PLAYER_NAME) {
>+            log("reb found: " + app.textContent);
>+            this.mirrorAppID = app.id

* Maybe we rename this.appID -> this.mediaAppID ?
* Bracing style cuddles the else: } else {

>+  mirror: function(callback) {
>+    if (callback) {
>+      callback();
>+    }

We should probably wait to do the callback until we've really created the mirror

>+function RemoteMirror(url) {
>+  this._serverURI = Services.io.newURI(url , null, null);
>+  this._window = Services.wm.getMostRecentWindow("navigator:browser");

Maybe mirror(callback) should take a browser tab too: mirror(tab, callback)
Then we can stop defaulting to the selected tab in this windowless code. We can alwyas pass in the selected tab from CastingApps

>+  this._baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
>+  this._error = RemoteMirrorError.bind(this)

Unused? If used, couldn't it be part of RemoteMirror?

>+  let constraints = {
>+    video: {
>+      mediaSource: "browser",
>+      advanced: [
>+        { width: { min: videoWidth, max: videoWidth },
>+          height: { min: videoHeight, max: videoHeight }
>+        },
>+        { aspectRatio: maxWidth / maxHeight }
>+      ]
>+    }
>+  };
>+
>+  this._window.navigator.mozGetUserMedia(constraints, RemoteMirrorGum.bind(this), this._error);

Maybe we need to pass the window in as well?


>+function RemoteMirrorGum (aStream) {
>+  this._stream = aStream;
>+  this._pc = new this._window.mozRTCPeerConnection;
>+  this._pc.addStream(aStream);
>+  this._pc.createOffer((function(aOffer) {
>+    this._pc.setLocalDescription(new this._window.mozRTCSessionDescription(aOffer), (function() {
>+      this._createSocket(aOffer);
>+    }).bind(this), this._error);
>+  }).bind(this), this._error);
>+}

Any reason this couldn't be part of RemoteMirror?

>+
>+function RemoteMirrorError() {
>+}

Unused

>+  _createSocket: function(aOffer) {
>+    const MIRROR_PORT = 8088;

Put at top of file?

>+    this._socket = this._baseSocket.open(this._serverURI.host, MIRROR_PORT, {useSecureTransport: false, binaryType: 'string'});

Here (and other JS object literals): we like to add one space padding inside the {} and use " instead of '
{ useSecureTransport: false, binaryType: "string" }

>+      this._pc.setRemoteDescription(
>+        new this._window.mozRTCSessionDescription({"type": "answer", "sdp": aResponse.data}),
>+        (function() {
>+        }).bind(this),

Is this an empty function? If so, skip the bind(this)

>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm

>+        if (aService.target == "roku:ecp") {
>+          aService.mirror = true;
>+        }

I'd like to not do this. Can't we add mirror = true to the Roku Target in CastingApps?

f+

Comment 3

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v2

Comment 4

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v3

Comment 5

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v4

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8468141 [details] [diff] [review]
Enable tab sharing. v4

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

These are all style nits I think. Looks good to me!

::: mobile/android/modules/RokuApp.jsm
@@ +52,5 @@
>          let apps = doc.querySelectorAll("app");
>          for (let app of apps) {
>            if (app.textContent == this.app) {
> +            this.mediaAppID = app.id;
> +          } else if (app.textContent == WEBRTC_PLAYER_NAME) {

Use triple equals. I forget to all the time, but we should use it.

@@ +145,5 @@
>        }
>      }
> +  },
> +
> +  mirror: function(callback, aWindow, aViewport) {

We used to always prefix arguments with 'a'. We've stopped doing that... sometimes :) Since they don't in this file, I'd stick with its style.

@@ +156,5 @@
> +  },
> +
> +  _createRemoteMirror: function(callback, aWindow, aViewport) {
> +    if (this.mirrorAppID == -1) {
> +      // TODO: Inform user to install Roku WebRTC Player Channel.

Roku told us the player will automatically launch the store for us. Can we use that here? File a follow up.

@@ +159,5 @@
> +    if (this.mirrorAppID == -1) {
> +      // TODO: Inform user to install Roku WebRTC Player Channel.
> +    } else {
> +      // TODO: Launch the Roku WebRTC Player Channel
> +      this.mirroror = new RemoteMirror(this.resourceURL, aWindow, aViewport);

'mirroror' ?

@@ +161,5 @@
> +    } else {
> +      // TODO: Launch the Roku WebRTC Player Channel
> +      this.mirroror = new RemoteMirror(this.resourceURL, aWindow, aViewport);
> +    }
> +    if (callback) {

Add a blank line between the first if-else and this one. In general, feel free to use more whitespace in here.

@@ +162,5 @@
> +      // TODO: Launch the Roku WebRTC Player Channel
> +      this.mirroror = new RemoteMirror(this.resourceURL, aWindow, aViewport);
> +    }
> +    if (callback) {
> +      callback();

Its strange that you make this mirroror object and then don't hand it to the callback here.

@@ +262,5 @@
>  }
> +
> +function RemoteMirror(url, aWindow, aViewport) {
> +  this._serverURI = Services.io.newURI(url , null, null);
> +  this._window = aWindow;

For these types of API's we're usually careful and only grab WeakRef's to DOM Nodes/Windows/etc. (unless they're only local). That said 1.) This is the XUL browser.xul window, so holding a reference to it probably doesn't matter and 2.) Maybe this wants to keep the window alive anyway, to ensure we keep casting even if the browser is destroyed.

I'll leave it up to you. Since the RemoteMirror app isn't really held by anyone, it probably loses this reference anyway. It looks like it sends a reference to itself to the GUM request, which holds it alive until that returns, and then also passes itself to some socket requests, which also keep it alive. Once the stream is started, the object dies?

@@ +270,5 @@
> +  let maxHeight = Math.max(aViewport.cssHeight, aViewport.height);
> +  let minRatio = Math.sqrt((maxWidth * maxHeight) / (640 * 480));
> +
> +  let screenWidth = 640;
> +  let screenHeight = 480;

Where are these from. We should use them up above in minRatio at least. Maybe the Roku sends us screen dimensions even (but I doubt we can cast at 1080i).

@@ +273,5 @@
> +  let screenWidth = 640;
> +  let screenHeight = 480;
> +  let videoWidth = 0;
> +  let videoHeight = 0;
> +  if (screenWidth/screenHeight > maxWidth / maxHeight) {

The spacing-style here is inconsistent within this line.

@@ +285,5 @@
> +  let constraints = {
> +    video: {
> +      mediaSource: "browser",
> +      advanced: [
> +        { width: { min: videoWidth, max: videoWidth },

Put 'width' on its own line.

@@ +293,5 @@
> +      ]
> +    }
> +  };
> +
> +  this._window.navigator.mozGetUserMedia(constraints, this._remoteMirrorGum.bind(this), function() {});

Log something in the error if it happens. We could probably show a toast or something too. There's a lot of blank error handling here. At least file a bug for that (since it needs strings, it will probably have to be separate). But logging would be helpful if this fails (as long as we're not leaking private information like urls to the log). Cu.reportError("Something something");

@@ +310,5 @@
> +
> +    this._socket.onerror = (function(err) {
> +      this._socket = null;
> +      if (this._stream) {
> +        if (this._stream.stop) { this._stream.stop(); }

Wrap this if onto multiple lines. JSON we'll ocassionally put on a single line. if statements (almost?) never.

@@ +320,5 @@
> +      this._socket.send(aOffer.sdp, aOffer.sdp.length);
> +    }).bind(this);
> +  },
> +
> +  _remoteMirrorGum: function (aStream) {

I think I'd name this something more specific like _onReceivedGUMStream

@@ +327,5 @@
> +    this._pc.addStream(aStream);
> +    this._pc.createOffer((function(aOffer) {
> +      this._pc.setLocalDescription(new this._window.mozRTCSessionDescription(aOffer), (function() {
> +        this._createSocket(aOffer);
> +        }).bind(this), function() {});

We would usually yank this indentation back one level here. You did that above, so I'll just assume brad wrote this.

Comment 7

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v5

carry r+ from wesj forward.

Comment 8

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v6

Carry r+ forward from :wjohnston

Comment 9

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v7

Comment 10

[Randall Barker [:rbarker]]

Comment on attachment 8476235 [details] [diff] [review]
Enable tab sharing. v7

I updated this so that mirroring can be enabled with a flag. Also fixed it so that the stop mirroring option shows up for Roku mirroring and will actually stop the stream. I've noticed that the red border doesn't go away when the stream is stopped, I'm still looking into what I can do to make it go away. With these changes, should probably re-review patch.

Comment 11

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v8

Comment 12

[Randall Barker [:rbarker]]

Comment on attachment 8476989 [details] [diff] [review]
Enable tab sharing. v8

Updated patch to closer match how the chromecast version works.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8476989 [details] [diff] [review]
Enable tab sharing. v8

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

> pref("browser.casting.enabled", false);
>+pref("browser.tab_mirror.enabled", false);
> #else
> pref("browser.casting.enabled", true);
>+pref("browser.tab_mirror.enabled", true);

Just a nit, but I see a pattern forming where we use "casting" and "mirroring". Could you switch to "browser.mirroring.enabled" ?

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js
>+  _mirrorStarted: function(stopMirrorCallback) {
>+

Remove the blank line

>+    this.stopMirrorCallback = stopMirrorCallback;

I'm not thrilled with this cached, but I don't have a better fix, so let's just use it.

>+  isTabMirrorEnabled: function isTabMirrorEnabled() {

Matching isCastingEnabled and the prefs, could you name this isMirroringEnabled ?

>diff --git a/mobile/android/modules/RokuApp.jsm b/mobile/android/modules/RokuApp.jsm

>+const WEBRTC_PLAYER_NAME = "WebRTC Player";
>+const MIRROR_PORT = 8088;

We should make the MEDIA_PLAYER_NAME and MEDIA_PORT consts in a followup patch

>+  _createRemoteMirror: function(callback, win, viewport, mirrorStartedCallback) {

>+    } else {
>+      // TODO: Launch the Roku WebRTC Player Channel
>+      this.remoteMirror = new RemoteMirror(this.resourceURL, win, viewport, mirrorStartedCallback);

Doesn't this launch the WebRTC player channel? If not, what more do we need?

>+RemoteMirror.prototype = {
>+
>+  _createSocket: function(offer) {

Remove the blank line

Wes might have more comments for you (he should still review), but this looks pretty good to me. I'd like to have some instructions for testing (for dev and QA) before we land this though. We should talk through what needs to happen for landing and testing.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8476989 [details] [diff] [review]
Enable tab sharing. v8

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

::: mobile/android/chrome/content/CastingApps.js
@@ +109,4 @@
>        this.mirrorStartMenuId = NativeWindow.menu.add({
>          name: Strings.browser.GetStringFromName("casting.mirrorTab"),
>          callback: function() {
>            function callbackFunc(aService) {

I would kinda rather we declared functions like this as variables I think. callbackFunc = function(aService); No idea why I like that. It just looks more normal to me.

@@ +118,5 @@
>  
>            function filterFunc(aService) {
>              return aService.mirror == true;
>            }
> +          this.prompt(callbackFunc.bind(this), filterFunc);

Fun mozilla JS tidbit, you could use lambda functions combined with big arrows to write:

this.prompt(callbackFunc.bind(this), aService => aService.mirror);

::: mobile/android/modules/RokuApp.jsm
@@ +264,5 @@
> +
> +function RemoteMirror(url, win, viewport, mirrorStartedCallback) {
> +  this._serverURI = Services.io.newURI(url , null, null);
> +  this._window = win;
> +  this._baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);

I would avoid creating this socket until you need it.

@@ +269,5 @@
> +  this.mirrorStarted = mirrorStartedCallback;
> +
> +  // This code insures the generated tab mirror is not wider than 800 nor taller than 600
> +  // Better dimensions should be chosen after the Roku Channel is working.
> +  let windowId = this._window.BrowserApp.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).outerWindowID;

Use win here instead of this._window (since this is so long...)

@@ +283,5 @@
> +  if ((cWidth / MaxWidth) > (cHeight / MaxHeight)) {
> +    tHeight = Math.ceil((MaxWidth / cWidth) * cHeight);
> +    tWidth = MaxWidth;
> +  }
> +  else {

move else up a line.

@@ +313,5 @@
> +    this._socket.ondata = (function(response) {
> +      this._pc.setRemoteDescription(
> +        new this._window.mozRTCSessionDescription({ "type": "answer", "sdp": response.data }),
> +        (function() { this.mirrorStarted(this._stopMirror.bind(this)); }).bind(this),
> +        function() {});

Log an error? You could add an error function somewhere in this file and just pass it in to all of these requests. At the very least, it can have a commented out Cu.reportError() line thats easy to uncomment if you want to enable some debugging.

@@ +334,5 @@
> +    this._pc = new this._window.mozRTCPeerConnection;
> +    this._pc.addStream(stream);
> +    this._pc.createOffer((function(offer) {
> +      this._pc.setLocalDescription(new this._window.mozRTCSessionDescription(offer), (function() {
> +        this._createSocket(offer);

I'd name this _sendOffer

Comment 15

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v9

Comment 16

[Randall Barker [:rbarker]]

Comment on attachment 8488890 [details] [diff] [review]
Enable tab sharing. v9

I had to change the way I communicate with the Roku to work with the latest version of WebRTC. The changes are enough to probably require a re-review.

Comment 17

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v10

Addressed comments in previous review.

Comment 18

[Randall Barker [:rbarker]]

Comment on attachment 8488907 [details] [diff] [review]
Enable tab sharing. v10

Updated patch to address comments from previous review.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8488907 [details] [diff] [review]
Enable tab sharing. v10

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

> var rokuTarget = {
>   target: "roku:ecp",
>   factory: function(aService) {
>     Cu.import("resource://gre/modules/RokuApp.jsm");
>     return new RokuApp(aService);
>   },
>+  mirror: true,
>   types: ["video/mp4"],
>   extensions: ["mp4"]
> };

Can you add the same "mirror" property to mediaPlayerDevice since technically it can mirror too?

>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm

>     if (!this._services.has(service.uuid)) {
>+      let target = this._targets.get(service.target);
>+      if (target && target.mirror) {
>+        service.mirror = true;
>+      }

I bitrotted you a bit here too:
_targets -> _devices
target -> device

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 8488907 [details] [diff] [review]
Enable tab sharing. v10

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

::: mobile/android/chrome/content/CastingApps.js
@@ +117,2 @@
>  
> +          this.prompt(callbackFunc, aService => aService.mirror);

This is so magic! I like it :)

::: mobile/android/modules/RokuApp.jsm
@@ +173,5 @@
> +        }
> +      }).bind(this), false);
> +
> +      xhr.addEventListener("error", (function() {
> +      }).bind(this), false);

Remove this if it doesn't do anything.

@@ +289,5 @@
> +  let cWidth =  Math.max(viewport.cssWidth, viewport.width);
> +  let cHeight = Math.max(viewport.cssHeight, viewport.height);
> +
> +  const MaxWidth = 800;
> +  const MaxHeight = 600;

All caps for constants. I bet we're crazy inconsistent with that or you would have done it :)

@@ +308,5 @@
> +      mediaSource: "browser",
> +      browserWindow: windowId,
> +      scrollWithPage: true,
> +      advanced: [
> +        { width: { min: tWidth, max: tWidth },

Put width on its own line.

@@ +325,5 @@
> +    if (!this._baseSocket) {
> +      this._baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
> +    }
> +    this._socket = this._baseSocket.open(this._serverURI.host, MIRROR_PORT, { useSecureTransport: false, binaryType: "string" });
> +    this._socket.ondata = (function(response) {

Can you move these functions to be methods on RemoteMirror.prototype

@@ +354,5 @@
> +      log("RemoteMirror: Error socket.onerror: " + (err.data ? err.data : "NO DATA"));
> +      this._socket = null;
> +      if (this._pc) {
> +        this._pc.close();
> +        this._pc = null;

_stopMirror() does the same thing. Can we use it here?

@@ +379,5 @@
> +    this._pc = new this._window.mozRTCPeerConnection;
> +    this._pc.addStream(stream);
> +    this._pc.onicecandidate = (function(evt) {
> +      // Usually the last candidate is null, expected?
> +      if (evt.candidate) {

if (!evt.candidate) early return

@@ +383,5 @@
> +      if (evt.candidate) {
> +        let jsonCandidate = JSON.stringify(evt.candidate);
> +        if (!this._open) {
> +          if (!this._iceCandidates) {
> +            this._iceCandidates = [];

I would just define this on the object so you don't need these checks.

@@ +392,5 @@
> +        jsonCandidate = jsonCandidate + JSON_MESSAGE_TERMINATOR;
> +        this._socket.send(jsonCandidate, jsonCandidate.length);
> +      }
> +    }).bind(this);
> +    this._pc.createOffer((function(offer) {

Blank line between lines here.

@@ +401,5 @@
> +    }).bind(this),
> +    function() { log("RemoteMirror: Failed to create offer."); });
> +  },
> +
> +  _stopMirror: function() {

Should this kill the socket?

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +375,5 @@
>      }
>  
>      // Only add and notify if we don't already know about this service
>      if (!this._services.has(service.uuid)) {
> +      let target = this._targets.get(service.target);

There's been some changes to nomenclature here (i.e. service.targetId I think?)

Comment 21

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v11

Address comments.

Comment 22

[Randall Barker [:rbarker]]

Attachment: Enable tab sharing. v12

Comment 23

[Randall Barker [:rbarker]]

Comment on attachment 8492378 [details] [diff] [review]
Enable tab sharing. v12

I added a pref for roku in addition to a master mirror pref. Is this sufficient to land this patch or is there something more/different required first?

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8492378 [details] [diff] [review]
Enable tab sharing. v12

The roku pref is fine with me.

Comment 25

[Randall Barker [:rbarker]]

Comment on attachment 8492378 [details] [diff] [review]
Enable tab sharing. v12

carry forward r+ from :mfinkle and :wesj

Comment 26

[Randall Barker [:rbarker]]

https://tbpl.mozilla.org/?tree=Try&rev=2069b0e228dd

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/4f539eec6a3e

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f539eec6a3e