Closed Bug 938571 Opened 11 years ago Closed 10 years ago

Add discovery support for second-screen devices

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(2 files, 5 obsolete files)

Too support discovering second-screen devices on the local network, we need to add some code to implement Simple Service Discovery Protocol (SSDP) which is a UPnP based discovery system.

Both Chromecast and Roku use SSDP, for example.

http://en.wikipedia.org/wiki/Simple_Service_Discovery_Protocol
Attached patch ssdp-broadcast (obsolete) — Splinter Review
This patch adds a simple SSDP implementation to Firefox for Android using Java classes, since I don't believe Gecko has the necessary networking primitives.

The code will search for a given SSDP target, listening for responses from the network for a fixed period. Any responses are passed to Gecko.
Assignee: nobody → mark.finkle
Attachment #832192 - Attachment filename: dial-broadcast → ssdp-broadcast
Attachment #832192 - Attachment description: dial-broadcast → ssdp-broadcast
Attached patch ssdp-jsm (obsolete) — Splinter Review
This patch adds a JavaScript module to manage SSDP requests and responses. The SimpleServiceDiscovery module allows you to register targets with the class. You can search in one-shot or continuous modes.

As responses are found, server information is stored and a notification is sent out. If the class finds that a once active server is no longer responding, we assume it is offline or out-of-range, and we send a notification.
Blocks: 921948
Blocks: 901803
Blocks: 921924
Depends on: 939680
Attached patch ssdp-jsm v0.2 (obsolete) — Splinter Review
Updated:
* Use SSDP detection in JavaScript! Thanks to Wes for finding the nsIUDPSocket a few days after it landed in m-c. With the recent landing, we no longer need to use Java to do the SSDP discovery.
* Uses the nsINetowrkLinkService to detect wifi or ethernet, otherwise we ignore the SSDP search request.
* Fixes a few bugs
Attachment #832197 - Attachment is obsolete: true
Attachment #832192 - Attachment is obsolete: true
Attached patch ssdp-jsm v0.3 (obsolete) — Splinter Review
I think this is close enough for review. In addition to the code itself, I'm looking for feedback on what is missing. I'm not sure we need any configuration preferences in this code, but I am open to suggestions.

The service allows outside code to be registered for specific SSDP services. Chromecast (DAIL) and Roku (Custom) are two cases, but others might appear. I want to keep this service fairly focused on discovery and nothing more.

I expect we'll find issues or new things we need to add as we integrate devices, but that should not stop us from landing the core service.
Attachment #8333702 - Attachment is obsolete: true
Attachment #8344454 - Flags: feedback?(wjohnston)
Attachment #8344454 - Flags: feedback?(rnewman)
Comment on attachment 8344454 [details] [diff] [review]
ssdp-jsm v0.3

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

I would like to see some tests for this -- even if only an httpd test for the XHR, and a couple of unit tests for the response parsing.

I'm also happy to spend the time to do most of the cleanup of this patch if you cannae be bothered.

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +1,1 @@
> +// -*- Mode: js2; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-

This is a brand new file, so let's use modern style rather than the obsolete toolkit style:

* Braces on every conditional.
* No named methods.
* No pseudo-Hungarian notation (aFoo, sBar, mNoo).

It's what a bunch of module owners recommend, and it also whitens teeth and eliminates heartburn.

@@ +6,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["SimpleServiceDiscovery"];
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

You never use Cc.

@@ +28,5 @@
> +  _localAddress: null,
> +  _searchInterval: 0,
> +  _searchTimestamp: 0,
> +  _searchTimeout: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
> +  _searchRepeat: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),

Do we need two timers, or can we use the same one, resetting it when we switch from a search to waiting to repeat?

@@ +32,5 @@
> +  _searchRepeat: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
> +
> +  // nsIUDPSocketListener implementation
> +  onPacketReceived: function(aSocket, aMessage) {
> +    // Listen for repsonses from specific targets. There could be more than one

responses

@@ +39,5 @@
> +    let location;
> +    let target;
> +    let valid = false;
> +    response.forEach(function(row) {
> +      log("response: " + row);

Don't routinely log here. You're printing arbitrary data from the network.

@@ +47,5 @@
> +      } else if (header.startsWith("ST")) {
> +        target = row.substr(4).trim();
> +        if (target in this._targets)
> +          valid = true;
> +      }

I think this is a reasonable use of a neat regex, particularly because of the expensive split/uppercase/substring/trim combo. You can do this in nearly one line.

Note also that you don't exit from the loop, so you're processing every row -- you'll take the last target and the last location. Is that intentional?

@@ +50,5 @@
> +          valid = true;
> +      }
> +    }.bind(this));
> +
> +    if (valid) {

What do we do if we never receive a valid packet?

@@ +55,5 @@
> +      // When we find a valid response, package up the server information
> +      // and pass it on.
> +      log("found[" + target + "]: " + location);
> +      let server = {
> +        location: location,

location can be undefined at this point -- you only set `valid` on encountering a target. Is this meaningful without a location?

@@ +63,5 @@
> +    }
> +  },
> +
> +  onStopListening: function(aSocket, aStatus) {
> +    log("socket stop: " + aStatus);

Should this be doing something, like stopping a timer or such?

@@ +67,5 @@
> +    log("socket stop: " + aStatus);
> +  },
> +
> +  // Start a search. Make it continuous by passing an interval (in seconds)
> +  search: function search(aInterval) {

search: function search(intervalSeconds) {

Including units is a lifesaver.

@@ +69,5 @@
> +
> +  // Start a search. Make it continuous by passing an interval (in seconds)
> +  search: function search(aInterval) {
> +    this._searchInterval = aInterval * 1000 || 0;
> +    if (this._searchInterval > 0)

if (aInterval > 0) {
  this._searchIntervalMillis = aInterval * 1000;

@@ +71,5 @@
> +  search: function search(aInterval) {
> +    this._searchInterval = aInterval * 1000 || 0;
> +    if (this._searchInterval > 0)
> +      this._searchRepeat.initWithCallback(this._search.bind(this), this._searchInterval, Ci.nsITimer.TYPE_REPEATING_SLACK);
> +    this._search();

Braces would make this clearer!

@@ +83,5 @@
> +  _search: function _search() {
> +    // We only search if on Wifi/Ethernet
> +    let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> +    if (network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_WIFI && network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET) {
> +      log("no wifi/ethernet: " + network.linkType);

Split out these lines, so you can do this:

  if (!isOnFooKindOfNetwork()) {
    return;
  }

which will also force you to put a name to the concept.

@@ +89,5 @@
> +    }
> +
> +    // Update the timestamp so we can use it to clean out stale servers the
> +    // next time we search.
> +    this._searchTimestamp = Date.now();

Clock skew will screw you here. Remember that Android clocks hop around a lot.

Use uptime or realtime if this doesn't need to be persisted to disk.

@@ +94,5 @@
> +
> +    // Perform a UDP broadcast to search for SSDP devices
> +    // spec: http://www.dial-multiscreen.org/dial-protocol-specification
> +    let port = 1900;
> +    let address = "239.255.255.250";

Lift magic constant, explain that it comes from the spec.

@@ +97,5 @@
> +    let port = 1900;
> +    let address = "239.255.255.250";
> +    let socket = Cc["@mozilla.org/network/udp-socket;1"].createInstance(Ci.nsIUDPSocket);
> +    socket.init(port, false);
> +    socket.asyncListen(this);

Are you ready for either of these to throw?

@@ +100,5 @@
> +    socket.init(port, false);
> +    socket.asyncListen(this);
> +
> +    let data = "M-SEARCH * HTTP/1.1\r\n" +
> +        "HOST: 239.255.255.250:1900\r\n" +

Shouldn't you be using your `address` constant here?

@@ +103,5 @@
> +    let data = "M-SEARCH * HTTP/1.1\r\n" +
> +        "HOST: 239.255.255.250:1900\r\n" +
> +        "MAN: \"ssdp:discover\"\r\n" +
> +        "MX: 2\r\n" +
> +        "ST: %SEARCH_TARGET%\r\n\r\n";

Lift magic.

@@ +107,5 @@
> +        "ST: %SEARCH_TARGET%\r\n\r\n";
> +
> +    this._searchTimeout.initWithCallback(function() {
> +      socket.close();
> +    }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);

this._searchTimeout.initWithCallback(socket.close, ...)

@@ +113,5 @@
> +    for each(let target in this._targets) {
> +      let msgString = data.replace("%SEARCH_TARGET%", target.target);
> +      let msgArray = Array.prototype.map.apply(msgString, [function(c) {
> +        return c.charCodeAt(0);
> +      }]);

Have you encountered

  https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIScriptableUnicodeConverter#convertToByteArray%28%29

? It turns a Unicode string of whichever encoding you like into a byte array, efficiently.

@@ +120,5 @@
> +  },
> +
> +  registerTarget: function registerTarget(aTarget, aAppFactory) {
> +    // Only add if we don't already know about this target
> +    if (this._targets.hasOwnProperty(aTarget) == false) {

if (!this.targets.hasOwnProperty(target)) {

@@ +140,5 @@
> +  get servers() {
> +    let array = [];
> +    for each (let server in this._servers)
> +      array.push(server);
> +    return array;

get servers() {
  return this._servers.slice();
},

@@ +164,5 @@
> +        aServer.localAddress = this._localAddress;
> +
> +        let doc = xhr.responseXML;
> +        aServer.appsURL = xhr.getResponseHeader("Application-URL");
> +        if (aServer.appsURL && aServer.appsURL.substr(-1) != "/")

...endsWith('/')

@@ +173,5 @@
> +        aServer.modelName = doc.querySelector("modelName").textContent;
> +
> +        // Only add and notify if we don't already know about this server
> +        if (this._servers.hasOwnProperty(aServer.location) == false) {
> +          this._servers[aServer.location] = aServer;

We can check this without making a request, right?
Attachment #8344454 - Flags: feedback?(rnewman) → feedback+
Status: NEW → ASSIGNED
OS: Linux → Android
Hardware: x86_64 → All
(In reply to Richard Newman [:rnewman] from comment #5)

> I would like to see some tests for this -- even if only an httpd test for
> the XHR, and a couple of unit tests for the response parsing.

Agreed

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

> This is a brand new file, so let's use modern style rather than the obsolete
> toolkit style:
> 
> * Braces on every conditional.

OK

> * No named methods.

OK

> * No pseudo-Hungarian notation (aFoo, sBar, mNoo).

Holding off on this one

> > +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> 
> You never use Cc.

Sure I do. Look below. 

> > +  _searchTimeout: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
> > +  _searchRepeat: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
> 
> Do we need two timers, or can we use the same one, resetting it when we
> switch from a search to waiting to repeat?

It is probably possible to use a single timer at the cost of complexity. I'd rather keep it simple.

> > +      log("response: " + row);
> 
> Don't routinely log here. You're printing arbitrary data from the network.

Indeed. Just debugging. I'll remove.

> > +      } else if (header.startsWith("ST")) {
> > +        target = row.substr(4).trim();
> > +        if (target in this._targets)
> > +          valid = true;
> > +      }
> 
> I think this is a reasonable use of a neat regex, particularly because of
> the expensive split/uppercase/substring/trim combo. You can do this in
> nearly one line.

I believe you, but I am easily confused by regex and this works. Sounds like a good followup.

> Note also that you don't exit from the loop, so you're processing every row
> -- you'll take the last target and the last location. Is that intentional?

Not intentional. We could break after finding both a LOCATION and a valid ST. The few devices I have seen only send back <10 headers, and never duplicates (which is against spec). Again, I didn't feel like adding the extra code/complexity for the check given the number of headers.

> What do we do if we never receive a valid packet?

We never send out the "we found a server" notification and life goes on.

> 
> @@ +55,5 @@
> > +      // When we find a valid response, package up the server information
> > +      // and pass it on.
> > +      log("found[" + target + "]: " + location);
> > +      let server = {
> > +        location: location,
> 
> location can be undefined at this point -- you only set `valid` on
> encountering a target. Is this meaningful without a location?

It's not meaningful. I'll change the code to:

  if (location && valid) { ... }

which means I can now move this to inside the loop and return early, addressing your feedback above.

> > +  onStopListening: function(aSocket, aStatus) {
> > +    log("socket stop: " + aStatus);
> 
> Should this be doing something, like stopping a timer or such?

This fires when we close the socket and we should be cancelling timers when we close it. I'll remove the log.

> > +  // Start a search. Make it continuous by passing an interval (in seconds)
> > +  search: function search(aInterval) {
> 
> search: function search(intervalSeconds) {
> 
> Including units is a lifesaver.

It's in the comment.

> 
> @@ +69,5 @@
> > +
> > +  // Start a search. Make it continuous by passing an interval (in seconds)
> > +  search: function search(aInterval) {
> > +    this._searchInterval = aInterval * 1000 || 0;
> > +    if (this._searchInterval > 0)
> 
> if (aInterval > 0) {
>   this._searchIntervalMillis = aInterval * 1000;

Done

> > +    if (this._searchInterval > 0)
> > +      this._searchRepeat.initWithCallback(this._search.bind(this), this._searchInterval, Ci.nsITimer.TYPE_REPEATING_SLACK);
> > +    this._search();
> 
> Braces would make this clearer!

Added

> > +  _search: function _search() {
> > +    // We only search if on Wifi/Ethernet
> > +    let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> > +    if (network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_WIFI && network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET) {
> > +      log("no wifi/ethernet: " + network.linkType);
> 
> Split out these lines, so you can do this:
> 
>   if (!isOnFooKindOfNetwork()) {
>     return;
>   }
> 
> which will also force you to put a name to the concept.

Done

> > +    this._searchTimestamp = Date.now();
> 
> Clock skew will screw you here. Remember that Android clocks hop around a
> lot.
> 
> Use uptime or realtime if this doesn't need to be persisted to disk.

Not sure how critical it is, or how easy it is to get uptime from JS.


> > +    // spec: http://www.dial-multiscreen.org/dial-protocol-specification
> > +    let port = 1900;
> > +    let address = "239.255.255.250";
> 
> Lift magic constant, explain that it comes from the spec.
> 
> @@ +97,5 @@
> > +    let port = 1900;
> > +    let address = "239.255.255.250";
> > +    let socket = Cc["@mozilla.org/network/udp-socket;1"].createInstance(Ci.nsIUDPSocket);
> > +    socket.init(port, false);
> > +    socket.asyncListen(this);
> 
> Are you ready for either of these to throw?

Good point. Changed to return, but not kill the interval timer since it might work the next time.

> > +    let data = "M-SEARCH * HTTP/1.1\r\n" +
> > +        "HOST: 239.255.255.250:1900\r\n" +
> 
> Shouldn't you be using your `address` constant here?

Done

> > +    this._searchTimeout.initWithCallback(function() {
> > +      socket.close();
> > +    }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> this._searchTimeout.initWithCallback(socket.close, ...)

I'm old school. I find the desire for braces around control statements, but lack of structure around functions an interesting dichotomy.

> > +    for each(let target in this._targets) {
> > +      let msgString = data.replace("%SEARCH_TARGET%", target.target);
> > +      let msgArray = Array.prototype.map.apply(msgString, [function(c) {
> > +        return c.charCodeAt(0);
> > +      }]);
> 
> Have you encountered
> 
>  
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIScriptableUnicodeConverter#convertToByteArray%28%29
> 
> ? It turns a Unicode string of whichever encoding you like into a byte
> array, efficiently.

Good find. Added.

> > +  registerTarget: function registerTarget(aTarget, aAppFactory) {
> > +    // Only add if we don't already know about this target
> > +    if (this._targets.hasOwnProperty(aTarget) == false) {
> 
> if (!this.targets.hasOwnProperty(target)) {

Done

> > +  get servers() {
> > +    let array = [];
> > +    for each (let server in this._servers)
> > +      array.push(server);
> > +    return array;
> 
> get servers() {
>   return this._servers.slice();
> },

Done

> > +        if (aServer.appsURL && aServer.appsURL.substr(-1) != "/")
> 
> ...endsWith('/')

Done

> > +        // Only add and notify if we don't already know about this server
> > +        if (this._servers.hasOwnProperty(aServer.location) == false) {
> > +          this._servers[aServer.location] = aServer;
> 
> We can check this without making a request, right?

Yes, but I want the server information to be available when the notification is sent out.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> > * No named methods.
> 
> OK

I was running the profiler last night and it still doesn't show names for anonymous functions. I don't think we should do this (I'm not really sure why its become popular all of the sudden).
(In reply to Mark Finkle (:mfinkle) from comment #6)

> > > +  get servers() {
> > > +    let array = [];
> > > +    for each (let server in this._servers)
> > > +      array.push(server);
> > > +    return array;
> > 
> > get servers() {
> >   return this._servers.slice();
> > },
> 
> Done

Undone. _servers is an object, not an array.
(In reply to Mark Finkle (:mfinkle) from comment #8)

> Undone. _servers is an object, not an array.

Object.keys(this._servers);
(In reply to Wesley Johnston (:wesj) from comment #7)

> I was running the profiler last night and it still doesn't show names for
> anonymous functions. I don't think we should do this (I'm not really sure
> why its become popular all of the sudden).

The debugger (and other stack-related tools) understands this syntax:

  { myMethod: function () { ... }, }

and names the method appropriately. It doesn't do that for all anonymous functions. If you're seeing the profiler not show names for methods, file a bug?

It became popular because our tools grew this ability about two years ago, and thus the hack of verbosely naming methods twice (often incorrectly, with the names drifting out of agreement!) was no longer necessary.
> > +    this._searchTimeout.initWithCallback(function() {
> > +      socket.close();
> > +    }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> this._searchTimeout.initWithCallback(socket.close, ...)

> I'm old school. I find the desire for braces around control statements,
> but lack of structure around functions an interesting dichotomy.

It's not structure; it's important to think about the differences here.

  function () {
    somefunction();
  }

and

  somefunction

are strictly equivalent, but the first version allocates an additional closure. That's not good -- "we are all on the Memshrink team!".

Note that

  function () {
    foo.bar();
  }

and

  foo.bar

are *not* strictly equivalent, because `this` will be bound differently. That might be important, depending on the method, but because of the additional cost of allocating that closure, you should know the difference.

The former is approximately equivalent to

  foo.bar.bind(foo);

which again is somewhat better style, but less readable than using a double-arrow.

You might consider using CommonUtils.namedTimer, which includes some handling of `this`.
(In reply to Richard Newman [:rnewman] from comment #10)
> (In reply to Wesley Johnston (:wesj) from comment #7)
> 
> > I was running the profiler last night and it still doesn't show names for
> > anonymous functions. I don't think we should do this (I'm not really sure
> > why its become popular all of the sudden).
> 
> The debugger (and other stack-related tools) understands this syntax:
> 
>   { myMethod: function () { ... }, }
> 
> and names the method appropriately. It doesn't do that for all anonymous
> functions. If you're seeing the profiler not show names for methods, file a
> bug?
> 
> It became popular because our tools grew this ability about two years ago,
> and thus the hack of verbosely naming methods twice (often incorrectly, with
> the names drifting out of agreement!) was no longer necessary.

Yes, I know about this. I don't think we should do it until support is better, but I'll file a bug for the profiler.
(In reply to Mark Finkle (:mfinkle) from comment #6)

> Sure I do. Look below. 

Huh. Find-in-page lets me down again.

> I believe you, but I am easily confused by regex and this works. Sounds like
> a good followup.

Please file, and I'll snag it! (Assuming that you get some test coverage on this.)

> It's in the comment.

Comments don't help you from getting units wrong in arithmetic inside the methods, or when you're calling it and not reading the comment. Be kind to those of us who are slow! We have found bugs in patches this way.

> > Use uptime or realtime if this doesn't need to be persisted to disk.
> 
> Not sure how critical it is, or how easy it is to get uptime from JS.

https://developer.mozilla.org/en-US/docs/Web/API/Performance.now() ?
Comment on attachment 8344454 [details] [diff] [review]
ssdp-jsm v0.3

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

I like this :)

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +16,5 @@
> +}
> +
> +/*
> + * SimpleServiceDiscovery manages any discovered SSDP servers.
> + * Since Gecko does not support UDP client sockets yet, we use a Java helper

This ain't true anymore :)

@@ +66,5 @@
> +  onStopListening: function(aSocket, aStatus) {
> +    log("socket stop: " + aStatus);
> +  },
> +
> +  // Start a search. Make it continuous by passing an interval (in seconds)

I think I'd rather just use ms here. Almost every other API we have uses them. It seems like using seconds here would just lead to confusion.

@@ +109,5 @@
> +    this._searchTimeout.initWithCallback(function() {
> +      socket.close();
> +    }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);
> +
> +    for each(let target in this._targets) {

for each... of

@@ +154,5 @@
> +    xhr.open("GET", aServer.location, true);
> +    xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +    xhr.overrideMimeType("text/xml");
> +
> +    xhr.addEventListener("load", (function() {

You could fat arrow this and remove the bind (the only good use for fat arrow I've found, but one I kinda love!)

@@ +173,5 @@
> +        aServer.modelName = doc.querySelector("modelName").textContent;
> +
> +        // Only add and notify if we don't already know about this server
> +        if (this._servers.hasOwnProperty(aServer.location) == false) {
> +          this._servers[aServer.location] = aServer;

Do we want to update this data, even if we had it before. Is there any chance that something changed?
Attachment #8344454 - Flags: feedback?(wjohnston) → feedback+
Attached patch ssdp-jsm v0.4 (obsolete) — Splinter Review
This patch addresses the previous feedback comments:
* Switches to milliseconds
* Removes the closure for the socket close. We use a bound callback and cache the socket.
  * We manage the socket and timer so that we only have a single search active at any time
* Uses new Map() instead of objects
* Uses for ( of ) for iterating
* No easy way of converting a Map to an Array, so we still need to keep that code [get servers()]
Attachment #8344454 - Attachment is obsolete: true
Attachment #8348994 - Flags: review?(wjohnston)
Comment on attachment 8348994 [details] [diff] [review]
ssdp-jsm v0.4

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

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +58,5 @@
> +    let response = aMessage.data.split("\n");
> +    let location;
> +    let target;
> +    let valid = false;
> +    response.some(function(row) {

I really like fat-arrows for these anonymous function-bind situations.

@@ +87,5 @@
> +  onStopListening: function(aSocket, aStatus) {
> +    // This is fired when the socket is closed expectedly or unexpectedly.
> +    // nsITimer.cancel() is a no-op if the timer is not active.
> +    this._searchTimeout.cancel();
> +    this._searchSocket = null;

Do you need to close the socket? Can we push all this into a _closeSocket function below?

@@ +102,5 @@
> +  },
> +
> +  // Stop the current continuous search
> +  stopSearch: function stopSearch() {
> +    this._searchRepeat.cancel();

The differences between stopSearch and _searchStop aren't exactly clear by their names...

@@ +109,5 @@
> +  _usingLAN: function() {
> +    let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> +    if (network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_WIFI && network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET) {
> +      return false;
> +    }

You could just 'return network.linkType == A || network.linkType == B'

@@ +116,5 @@
> +
> +  _search: function _search() {
> +    // We only search if on local network
> +    if (!this._usingLAN()) {
> +      log("no wifi/ethernet");

Probably don't want to log this all the time. You probably want a distinction between log() and debug/info here. i.e. error should always appear. debugging messages only if some flag is set.

@@ +137,5 @@
> +    // next time we search.
> +    this._searchTimestamp = Date.now();
> +
> +    // If a search is already active, shut it down.
> +    this._searchStop();

I'd probably do this at the top of this function. Is there some benefit to keeping the old socket around if this one failed to be created?

@@ +153,5 @@
> +      }
> +    }
> +  },
> +
> +  _searchStop: function _searchStop(aSocket) {

Maybe we can name this something like _closeSocket()

@@ +163,5 @@
> +
> +  registerTarget: function registerTarget(aTarget, aAppFactory) {
> +    // Only add if we don't already know about this target
> +    if (!this._targets.has(aTarget)) {
> +      this._targets.set(aTarget, { target: aTarget, factory: aAppFactory });

It might be nice to return something here so that callers can know if this succeeded?

@@ +179,5 @@
> +    }
> +    return null;
> +  },
> +
> +  get servers() {

Lets add a note that we're intentionally returning a copy. Hopefully, callers will be smart and not call this over and over.

@@ +187,5 @@
> +    }
> +    return array;
> +  },
> +
> +  get localAddress() {

Do you really need a getter here? Do we need this to be read-only?

@@ +226,5 @@
> +        this._servers.get(aServer.location).lastPing = this._searchTimestamp;
> +
> +        // Clean out any stale servers
> +        for (let [key, server] of this._servers) {
> +          if (server.lastPing != this._searchTimestamp) {

Hmm... Does this need a longer timeout? Is it reasonable to expect them to reply on every request? Sometimes on desktop I see things pop in rather slowly, but I'm not sure why that is.
Attachment #8348994 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #16)

> > +    response.some(function(row) {
> 
> I really like fat-arrows for these anonymous function-bind situations.

I know you do but, I am not there yet.

> > +  onStopListening: function(aSocket, aStatus) {
> > +    // This is fired when the socket is closed expectedly or unexpectedly.
> > +    // nsITimer.cancel() is a no-op if the timer is not active.
> > +    this._searchTimeout.cancel();
> > +    this._searchSocket = null;
> 
> Do you need to close the socket? Can we push all this into a _closeSocket
> function below?

If the network connection drops, or some other disaster strikes, this is called by the socket itself. I want to try to be sure that we have expected and unexpected shutdowns covered.

> > +  // Stop the current continuous search
> > +  stopSearch: function stopSearch() {
> > +    this._searchRepeat.cancel();
> 
> The differences between stopSearch and _searchStop aren't exactly clear by
> their names...

I changed _searchStop to _searchShutdown

> > +  _usingLAN: function() {
> > +    let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> > +    if (network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_WIFI && network.linkType != Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET) {
> > +      return false;
> > +    }
> 
> You could just 'return network.linkType == A || network.linkType == B'

Done

> > +    if (!this._usingLAN()) {
> > +      log("no wifi/ethernet");
> 
> Probably don't want to log this all the time. You probably want a
> distinction between log() and debug/info here. i.e. error should always
> appear. debugging messages only if some flag is set.

I removed it for now

> > +    // If a search is already active, shut it down.
> > +    this._searchStop();
> 
> I'd probably do this at the top of this function. Is there some benefit to
> keeping the old socket around if this one failed to be created?

I moved it to the top. Cleaning up the existing socket seems reasonable.

> > +  _searchStop: function _searchStop(aSocket) {
> 
> Maybe we can name this something like _closeSocket()

Renamed to _searchShutdown since it's not only about closing the socket. It's also about saying the search cycle timed out.

> > +  registerTarget: function registerTarget(aTarget, aAppFactory) {
> > +    // Only add if we don't already know about this target
> > +    if (!this._targets.has(aTarget)) {
> > +      this._targets.set(aTarget, { target: aTarget, factory: aAppFactory });
> 
> It might be nice to return something here so that callers can know if this
> succeeded?

I'm not opposed to returning something. I don't have any ideas for now. Maybe we'll think of something as we use it.

> > +  get servers() {
> 
> Lets add a note that we're intentionally returning a copy. Hopefully,
> callers will be smart and not call this over and over.

Done

> > +  get localAddress() {
> 
> Do you really need a getter here? Do we need this to be read-only?

This is a leftover bit of code. I can remove it now that a service has a localAddress property.

> > +        // Clean out any stale servers
> > +        for (let [key, server] of this._servers) {
> > +          if (server.lastPing != this._searchTimestamp) {
> 
> Hmm... Does this need a longer timeout? Is it reasonable to expect them to
> reply on every request? Sometimes on desktop I see things pop in rather
> slowly, but I'm not sure why that is.

I moved this to _searchShutdown, which is normally fired when the search cycle times out... after 10 seconds. That should be enough time. We can always tweak this if we see strange behavior.
Attached patch ssdp-jsm v0.4Splinter Review
Updated with most of Wes' suggestions. Carrying r+.
Attachment #8348994 - Attachment is obsolete: true
Attachment #8351531 - Flags: review+
A simple test. The code forces a discovery of a hard coded service. The service XML file is pulled by the SimpleServiceDiscovery code and the test is notified.

We then check that the service meta data is correct.
Attachment #8351532 - Flags: review?(rnewman)
Comment on attachment 8351532 [details] [diff] [review]
Simple discovery test v0.1

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

r+ with nits.

::: mobile/android/base/tests/testSimpleDiscovery.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

Why don't we have licenses in some of these test files?

::: mobile/android/base/tests/testSimpleDiscovery.js
@@ +6,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/SimpleServiceDiscovery.jsm");
> +
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;

"use strict";

const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Cu.import(…);

@@ +8,5 @@
> +
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;
> +
> +function discovery_observer(aSubject, aTopic, aData) {

Hurrrghhhhhh aFoo

@@ +30,5 @@
> +  });
> +
> +  Services.obs.addObserver(discovery_observer, "ssdp-service-found", false);
> +
> +  do_print("Located a simple service");

At this point, this is inaccurate -- you haven't, you've only registered the observer.

@@ +42,5 @@
> +  // Poke the service directly to get the discovery to happen
> +  SimpleServiceDiscovery._found(service);
> +});
> +
> +run_next_test();

Instead of this, put this:

  function run_test() {
    run_next_test();
  }

before your add_test call, so the file reads:

  run_test;
  add_test(...);
  add_test(...);

That's usual style.
Attachment #8351532 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #21)

> ::: mobile/android/base/tests/testSimpleDiscovery.java
> @@ +1,1 @@
> > +package org.mozilla.gecko.tests;
> 
> Why don't we have licenses in some of these test files?

No idea, but every .java test file I opened had no license. Perhaps a nice intern project?

> ::: mobile/android/base/tests/testSimpleDiscovery.js

> > +Components.utils.import("resource://gre/modules/Services.jsm");
> > +Components.utils.import("resource://gre/modules/SimpleServiceDiscovery.jsm");
> > +
> > +var Cc = Components.classes;
> > +var Ci = Components.interfaces;
> 
> "use strict";
> 
> const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

Done

> > +function discovery_observer(aSubject, aTopic, aData) {
> 
> Hurrrghhhhhh aFoo

It must be the festive mood, but I changed these for you.

> > +  do_print("Located a simple service");
> 
> At this point, this is inaccurate -- you haven't, you've only registered the
> observer.

Fixed

> > +run_next_test();
> 
> Instead of this, put this:
> 
>   function run_test() {
>     run_next_test();
>   }

Done

> before your add_test call, so the file reads:
> 
>   run_test;
>   add_test(...);
>   add_test(...);

I see run_test defined after the add_test calls in the files I opened. In any case, I added the run_test call as you pointed out. The test framework seems to call it.
(In reply to Mark Finkle (:mfinkle) from comment #22)

> > Why don't we have licenses in some of these test files?
> 
> No idea, but every .java test file I opened had no license. Perhaps a nice
> intern project?

nalexander has a script :D
(In reply to Mark Finkle (:mfinkle) from comment #22)
> (In reply to Richard Newman [:rnewman] from comment #21)

> > > +run_next_test();
> > 
> > Instead of this, put this:
> > 
> >   function run_test() {
> >     run_next_test();
> >   }
> 
> Done
> 
> > before your add_test call, so the file reads:
> > 
> >   run_test;
> >   add_test(...);
> >   add_test(...);
> 
> I see run_test defined after the add_test calls in the files I opened. In
> any case, I added the run_test call as you pointed out. The test framework
> seems to call it.

Actually, this doesn't seem to work in our harness. I needed to use the run_next_test() call like all other JavaScript harness tests, including the ones Nick and Richard added. Might be worth looking into.
(In reply to Mark Finkle (:mfinkle) from comment #24)

> Actually, this doesn't seem to work in our harness. I needed to use the
> run_next_test() call like all other JavaScript harness tests, including the
> ones Nick and Richard added. Might be worth looking into.

Huh, good to know!
https://hg.mozilla.org/mozilla-central/rev/f90dc8337952
https://hg.mozilla.org/mozilla-central/rev/b6a3eb865983
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Blocks: 953346
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: