1008436 - Support Firefly multiscreen service

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: netcast.patch

Comment 1

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

Comment on attachment 8420384 [details] [diff] [review]
netcast.patch

Just a quick first pass review. Too tired to do a complete review right now. I'll pick it up tomorrow.


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

>+    SimpleServiceDiscovery.registerTarget(netcastTarget, netcastFactory, ["video/mp4", "video/webm"], ["mp4", "webm"]);

Let's just drop the mimetype and extensions for now. We have a little work to do to get that working right. Let's just pretend we only support h264 for now.

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

naming nit: Do we want to use the XxxApp.jsm or just Xxx.jsm convention? The Roku code uses RokuApp.jsm which is also the JS object which is exported. You use Netcaster.jsm and export NetcasterApp. As long as we are consistent, I don't care.

>+// meta constants

<snip>lot's of constants</snip>

Do we nee all of these?

>+let gWindow;

This seems unused, which is good :) Let's remove it.

>+function NetCastApp(aService) {

You use unprefixed args everywhere else in the file. Might as well use | service |

>+  let ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+    .getService(Components.interfaces.nsIIOService);
>+  let uri = ioService.newURI(aService.location, null, null);

Use: Services.ios.newURI(aService.location, null, null);

>+  this._ip = uri.host;

Is the port always the same, or should we set it via uri.port ?

>+NetCastApp.prototype = {

>+  _info_timer: 0,
>+  _send_meta_cmd: function(cmd, callback, extras) {

Add a blank line before methods for separation

>+    let str = JSON.stringify([HEADER_META, msg ]);
>+    this._send_cmd(str, 0);

Remove the space after "msg"
Don't bother with the local "str" variable. Just pass ther JSON.stringify to _send_cmd

>+  _send_ramp_cmd: function(type, cmd_id, callback, extras) {

>+    let str = JSON.stringify([NAMESPACE_RAMP, msg ]);
>+    this._send_cmd(str, 0);

Remove the space after "msg"
Don't bother with the local "str" variable. Just pass ther JSON.stringify to _send_cmd

>+  _send_cmd: function(str, recursionDepth) {
>+    if(!this._cmd_socket) {

if (!this
nit: add the space

>+      let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket);
>+      this._cmd_socket = baseSocket.open(this._ip, 8888, {useSecureTransport: false, binaryType: 'string'});

Use this._port ?

>+      if (!(this._cmd_socket)) {
>+        Cu.reportError("socket is null");

I'm not sure we want to use Cu.reportError for this. Maybe just make a dump(msg) method that wraps Services.console.logStringMessage(msg)

>+      let self = this;

Using self like this is really old school. I'll help clean it up in the next round.
We typically use function(...) {}.bind(this) or (*choke*) fat arrow functions. We can chat about it.

>+        if (data[1]['app-name'] == PROTO_HOME_APP) {

Use " instead of '

>+            this._handle_meta_callback({status: "OK"});

nit: We typically pad the object { } with spaces: { status: "OK" }

>+          }
>+        }
>+
>+      } catch (ex) {

Remove the trailing blank line

>+  observe: function(subject, data, topic) {
>+    if (data = "timer-callback") {

===

>+  start: function(func) {

>+    this._send_meta_cmd(cmd,func, {"app-name": "Remote Player"});

nit: We typically pad the object { } with spaces: { "app-name": "Remote Player" }
and you should not need quotes around the app-name in this case. JSON.stringify will handle that.

>+  stop: function(func) {
>+    if (func)
>+      func(true);

Use { }

>+  remoteMedia: function(func, listener) {

>+    let self = this;

self is unused

>+    if (listener)
>+      listener.onRemoteMediaStart(this);

Use { }

>+  _handle_meta_response: function(data) {
>+    switch(data.cmd) {
>+    case "create-session":
>+    case "~create-session":
>+      // if we get a response form start-app, assume we have a connection already
>+    case "start-app":
>+    case "~start-app":
>+      this._have_session = data.status == "OK";
>+      break;
>+    case "end-session":
>+    case "~end-session":
>+      this._have_session = data.status != "OK";
>+      break;
>+    }

JS indents the "case" statements

Comment 2

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

Comment on attachment 8420384 [details] [diff] [review]
netcast.patch

>+    }
>+    if (this._mediaListener)
>+      this._mediaListener.onRemoteMediaStatus(this);

nit: Please use a few blank lines to separate the logical chunks of a function.

Use {} around the if statement

>+  play: function(){
>+  pause: function(){

Add a space before the {

>+  shutdown: function() {
>+    if (this._info_timer) {
>+      this._info_timer.clear();
>+      this._info_ttimer = {};

Typo: this._info_ttimer  ->  this._info_timer
Set this._info_timer to null not {} since you set it to null in the "timer-callback"

>+    this.stop(function() {

>+      if (self._mediaListener)
>+        self._mediaListener.onRemoteMediaStop(self);

Use {} around the if statement

r- to see a new patch, but nothing too major. looks pretty good.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: netcast.patch

(In reply to Mark Finkle (:mfinkle) from comment #1)

> >+// meta constants
> 
> <snip>lot's of constants</snip>
> 
> Do we nee all of these?
I figured we might as well document these constants for future use.

> 
> >+  this._ip = uri.host;
> 
> Is the port always the same, or should we set it via uri.port ?
always the same

Comment 4

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

I am building and running locally. I want to see the interaction with Roku when both devices are active on a network since the Roku will respond to DIAL discovery too. I also want to see if we can add some simple filtering into the service. That would be a followup, but I want to check it out now.

Comment 5

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

Attachment: fennec-dial-discovery.png

We need to do some filtering before landing this patch. With this applied, I get Rokus counted twice and Chromecasts showing up too. In the screenshot, only "FinkleCast" is a Netcast device. I don't even have the Chromecast support in this build, but they show up too - and won't work.

I'll start looking at ways to filter.

Comment 6

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

Comment on attachment 8426725 [details] [diff] [review]
netcast.patch

r+ but I have this patch tweaked a little to handle the changes in bug 1016785 which is needed to land before this one.

I will handle landing this patch along with the stuff in bug 1016785.

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/19f0790943e3

Landed with tweaks for bug 1016785 and renamed to Firefly where appropriate.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19f0790943e3