921948 - Integrate roku support into Firefox

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Using the core patches in bug 901803, we can also add support for Roku. We can create a Roku app (called a channel) designed to work with Firefox and support viewing images, videos and tab mirroring.

Comment 1

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

Attachment: Roku support v0.1

This patch adds a JSM for RokuApp support. The Ruko does use SSDP for discovery, so the code in the other patches for SSDP works fine, if we send the Roku target.

The Roku REST API for controlling applications is different, so the RokuApp tries to encapsulate what we can. It uses the same "interface" as the ChromecastApp.

The JSM also has a RemoteMedia class that wraps the Roku "INPUT" protocol, which is a simple HTTP POST + query params system. The Roku and Chromecast systems are different enough to make trying to support *everything* both systems can do a bit of a losing game. Let's not block on trying to do that.

Comment 2

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

Attachment: Main code for Roku app

This is the source code to a Roku app that supports tab mirroring. Pretty simple stuff, but gives an idea of what's possible. The Roku platform supports much more than this. We can explore doing more as we get ideas/designs.

Comment 3

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

Attachment: ruko-support

The biggest change in this patch is moving away from Roku's INPUT style external control protocol and moving to a simple TCP socket system. The INPUT system did not allow for two-way communication, which we really want for status updates.

Comment 4

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

Attachment: Main code for Roku app v0.2

Updated the code to add the TCP socket server support. The app now sends status back to Firefox. The app is now designed to play videos too. In fact, this is the primary role now.

Comment 5

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

Attachment: Roku support v0.2

Updated control code for Roku

Comment 6

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

Attachment: ruko-app v0.3

Updated to current code

Comment 7

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

Attachment: ruko-app v0.4

Let's start the review process. Wes has spent some time looking at this.

Comment 8

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

For a whole-system overview, we'll need to review the Roku client too:
http://hg.mozilla.org/users/mfinkle_mozilla.com/firefox-roku/summary

(before Taras shuts down the HG user repos)

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8399718 [details] [diff] [review]
ruko-app v0.4

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

Mostly nits. Curious about the aCallback({ state: "unknown" }); stuff though.

::: mobile/android/chrome/content/CastingApps.js
@@ +13,5 @@
>  
> +    // Register a service target
> +    SimpleServiceDiscovery.registerTarget("roku:ecp", function(aService, aApp) {
> +      Cu.import("resource://gre/modules/RokuApp.jsm");
> +      return new RokuApp(aService, "FirefoxTest");

Probably want to remove the Test bit here, but I assume its required for the app for now...

::: mobile/android/modules/RokuApp.jsm
@@ +11,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function log(msg) {
> +  Services.console.logStringMessage(msg);

Disable before landing?

@@ +35,5 @@
> +    let url = this.resourceURL + "query/apps";
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
> +    xhr.open("GET", url, true);
> +    xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +    xhr.overrideMimeType("text/xml");

All of the functions here do XHR requests. I wonder if we should wrap them. OTOH, This one is a GET request for text/xml and most of the rest are PUT's with text/plain. Not sure how helpful it will be....

@@ +52,5 @@
> +        if (aCallback)
> +          aCallback({ state: "unknown" });
> +      } else {
> +        if (aCallback)
> +          aCallback({ state: "unknown" });

We return the same state regardless of what xhr.status is, or if we got an id. Is this a bug? If not, I'd yank it out of the if-else block.

@@ +58,5 @@
> +    }).bind(this), false);
> +
> +    xhr.addEventListener("error", (function() {
> +      if (aCallback)
> +        aCallback({ state: "unknown" });

Same with errors?

@@ +70,5 @@
> +    if (this.appID == -1) {
> +      this.status(function() {
> +        // If we found the appID, use it to make a new start call
> +        if (this.appID != -1)
> +          this.start(aCallback);

aCallback(false) should be called if this failed?

@@ +77,5 @@
> +    }
> +
> +    // Start a given app with any extra query data. Each app uses it's own data scheme.
> +    // NOTE: Roku will also pass "source=external-control" as a param
> +    let url = this.resourceURL + "launch/" + this.appID + "?version=" + parseInt(PROTOCOL_VERSION);

What's this parseInt for?

@@ +86,5 @@
> +
> +    xhr.addEventListener("load", (function() {
> +      if (xhr.status == 200) {
> +        if (aCallback)
> +          aCallback(true);

I'd probably just simplify this to if (aCallback) aCallback(xhr.status === 200);

@@ +115,5 @@
> +        if (aCallback)
> +          aCallback(true);
> +      } else {
> +        if (aCallback)
> +          aCallback(false);

Same as before. These two xhr's could definately go in a helper. They're basically identical.

@@ +130,5 @@
> +
> +  remoteMedia: function remoteMedia(aCallback, aListener) {
> +    if (this.appID != -1) {
> +      if (aCallback)
> +        aCallback(new RemoteMedia(this.resourceURL, aListener));

If its really optional, I'm tempted to just set a default for aCallback and remove all these checks. i.e. in the function signature put remoteMedia(aCallback = function() {}, aListener)

@@ +141,5 @@
> +
> +/* RemoteMedia provides a wrapper for using TCP socket to control Roku apps.
> + * The server implementation must be built into the Roku receiver app.
> + */
> +function RemoteMedia(aURL, aListener) {

Remove 'a' prefixes on arguments.

@@ +166,5 @@
> +
> +  onDataAvailable: function(request, context, stream, offset, count) {
> +    this._scriptableStream.init(stream);
> +    let data = this._scriptableStream.read(count);
> +    log("XXXX log socket: " + data)

Remove

@@ +172,5 @@
> +      return;
> +    }
> +
> +    let msg = JSON.parse(data);
> +    if (this._status != msg._s) {

I prefer early returns.
if (this._status === msg._s) {
  return;
}

@@ +189,5 @@
> +    }
> +  },
> +
> +  onStopRequest: function(request, context, result) {
> +    log("XXX server stop request")

Remove

@@ +213,5 @@
> +
> +  play: function play() {
> +    // TODO: add position support
> +    let data = { type: "PLAY", _v: PROTOCOL_VERSION };
> +    this._sendMsg(JSON.stringify(data));

Might be better to have _sendMsg take an object and append the protocol version in there?

Comment 10

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

(In reply to Wesley Johnston (:wesj) from comment #9)

> ::: mobile/android/chrome/content/CastingApps.js
> @@ +13,5 @@
> >  
> > +    // Register a service target
> > +    SimpleServiceDiscovery.registerTarget("roku:ecp", function(aService, aApp) {
> > +      Cu.import("resource://gre/modules/RokuApp.jsm");
> > +      return new RokuApp(aService, "FirefoxTest");
> 
> Probably want to remove the Test bit here, but I assume its required for the
> app for now...

Right. We need a followup patch before we enable this code. We also need to "ship" the Nightly version of the Roku App too. We'll probably use "FennecNightly" and "Firefox" for the real names, associated with Nightly/Aurora and Beta/Release on Android.

> ::: mobile/android/modules/RokuApp.jsm

> > +function log(msg) {
> > +  Services.console.logStringMessage(msg);
> 
> Disable before landing?

Yeah

> > +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
> > +    xhr.open("GET", url, true);
> > +    xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> > +    xhr.overrideMimeType("text/xml");
> 
> All of the functions here do XHR requests. I wonder if we should wrap them.
> OTOH, This one is a GET request for text/xml and most of the rest are PUT's
> with text/plain. Not sure how helpful it will be....

I did not wrap any XHR's in this patch. I am "wait and see" on it.

> > +        if (aCallback)
> > +          aCallback({ state: "unknown" });
> > +      } else {
> > +        if (aCallback)
> > +          aCallback({ state: "unknown" });
> 
> We return the same state regardless of what xhr.status is, or if we got an
> id. Is this a bug? If not, I'd yank it out of the if-else block.

Roku has no way to tell us if an app is running yet, so I play it safe for now. I did add a comment about it. We might be able to "fix" this when Roku starts supporting DIAL on more of it's models.

> > +    xhr.addEventListener("error", (function() {
> > +      if (aCallback)
> > +        aCallback({ state: "unknown" });
> 
> Same with errors?

An error still means that we don't know if it's running.

> > +        // If we found the appID, use it to make a new start call
> > +        if (this.appID != -1)
> > +          this.start(aCallback);
> 
> aCallback(false) should be called if this failed?

Good catch. Yes.

> > +    // Start a given app with any extra query data. Each app uses it's own data scheme.
> > +    // NOTE: Roku will also pass "source=external-control" as a param
> > +    let url = this.resourceURL + "launch/" + this.appID + "?version=" + parseInt(PROTOCOL_VERSION);
> 
> What's this parseInt for?

Honestly, I forget the details, but it wasn't working unless I added the parseInt. I think margaret was hit by this somewhere too.

> > +      if (xhr.status == 200) {
> > +        if (aCallback)
> > +          aCallback(true);
> 
> I'd probably just simplify this to if (aCallback) aCallback(xhr.status ===
> 200);

Done

> > +        if (aCallback)
> > +          aCallback(true);
> > +      } else {
> > +        if (aCallback)
> > +          aCallback(false);
> 
> Same as before. These two xhr's could definately go in a helper. They're
> basically identical.

I collasped the if-block, but did not wrap the XHR. The helper didn't feel that valuable. Maybe I have this "don't make jQuery" thing in my head.

> > +  remoteMedia: function remoteMedia(aCallback, aListener) {
> > +    if (this.appID != -1) {
> > +      if (aCallback)
> > +        aCallback(new RemoteMedia(this.resourceURL, aListener));
> 
> If its really optional, I'm tempted to just set a default for aCallback and
> remove all these checks. i.e. in the function signature put
> remoteMedia(aCallback = function() {}, aListener)

I don't want to default to a function() {} yet. That seems wasteful. I almost ripped them all out, but I want to wait for a followup to see how this code evolves on it's own first.

> > +function RemoteMedia(aURL, aListener) {
> 
> Remove 'a' prefixes on arguments.

Done

> > +  onDataAvailable: function(request, context, stream, offset, count) {
> > +    this._scriptableStream.init(stream);
> > +    let data = this._scriptableStream.read(count);
> > +    log("XXXX log socket: " + data)
> 
> Remove

Done

> > +    let msg = JSON.parse(data);
> > +    if (this._status != msg._s) {
> 
> I prefer early returns.
> if (this._status === msg._s) {
>   return;
> }

Done

> > +  onStopRequest: function(request, context, result) {
> > +    log("XXX server stop request")
> 
> Remove

Done

> > +    let data = { type: "PLAY", _v: PROTOCOL_VERSION };
> > +    this._sendMsg(JSON.stringify(data));
> 
> Might be better to have _sendMsg take an object and append the protocol
> version in there?

Done

Comment 11

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

Attachment: ruko-app v0.5

With most comments addressed

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8400893 [details] [diff] [review]
ruko-app v0.5

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

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

I still find this a bit weird. If we get an error or a non-200 response, we know something then (i.e. the Roku seems busted or the app isn't installed?). I guess our callers just check for the appId to get that info...

Comment 13

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

Still pref'd off, but landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cf96fc38ec

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4cf96fc38ec

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Added with the description "Roku support". Let me know if you prefer something else.

Comment 16

[Erin Lancaster [:elan]]

shoot, I think we need to un-rel note for now.