Integrate roku support into Firefox

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 31
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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.
Posted patch Roku support v0.1 (obsolete) — Splinter Review
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.
Assignee: nobody → mark.finkle
Posted file Main code for Roku app (obsolete) —
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.
Posted patch ruko-support (obsolete) — Splinter Review
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.
Attachment #811828 - Attachment is obsolete: true
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.
Attachment #811829 - Attachment is obsolete: true
Depends on: 938571
Posted patch Roku support v0.2 (obsolete) — Splinter Review
Updated control code for Roku
Attachment #832200 - Attachment is obsolete: true
Posted patch ruko-app v0.3 (obsolete) — Splinter Review
Updated to current code
Attachment #8359618 - Attachment is obsolete: true
Posted patch ruko-app v0.4 (obsolete) — Splinter Review
Let's start the review process. Wes has spent some time looking at this.
Attachment #8396699 - Attachment is obsolete: true
Attachment #8399718 - Flags: review?(wjohnston)
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 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?
Attachment #8399718 - Flags: review?(wjohnston) → review-
(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
Posted patch ruko-app v0.5Splinter Review
With most comments addressed
Attachment #8399718 - Attachment is obsolete: true
Attachment #8400893 - Flags: review?(wjohnston)
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...
Attachment #8400893 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/b4cf96fc38ec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Depends on: 993633
Added with the description "Roku support". Let me know if you prefer something else.
shoot, I think we need to un-rel note for now.
You need to log in before you can comment on or make changes to this bug.