Closed Bug 1016785 Opened 6 years ago Closed 5 years ago

Provide better ways to filter duplicate or unwanted services

Categories

(Firefox for Android :: Screencasting, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(3 files)

Some devices support multiple services: Roku supports ECP and DIAL
Some devices act like clones of other devices: Netcast is a mimic of Chromecast

These means that we can easily end up with duplicate devices and services listed in the "Cast to Screen" list.

We should try to do better at filtering out duplicate devices and we should find a way to be more explicit when registering a target.
This patch is a first step. It uses the device's UUID as a unique ID and stops using the location. The location for a roku could be:
* http://192.168.1.100:8060 (for ECP)
* http://192.168.1.100:8008/ssdp/dd.xml (for DIAL)

So we can't use the location for uniqueness. The UUID is the same for both ECP and DIAL.
Assignee: nobody → mark.finkle
Attachment #8429785 - Flags: review?(wjohnston)
This patch goes further. It allows targets to be filtered themselves. This means that we can block the Roku DIAL responses from ever being added to the list of services. With the last patch, a Roku DIAL response could race an ECP response and we'd have a mix of some DIAL and some ECP Rokus listed. No dupes, but the DIAL services would still not work if selected.

The patch pulls the target "helpers" into globals and changes SimpleServiceDiscovery to register the helper objects. An unregister was added too, mainly to cleanup the tests, but might be useful for Add-ons.

I put the helper objects in CastingApps.js for now to keep the changes simple and to keep the lazy JSM loading. We might decide to move these helpers later and put them in the JSM files.

The helper objects add an optional "filters" object which allows SSDP to filter the incoming service using JS properties. Right now, it uses simple string matches, but we could change that to Regex later, if needed.

Using this patch, everything looks good, except we still see Chromecasts appear. I have a patch that builds on this one to handle that.
Attachment #8430361 - Flags: review?(wjohnston)
This patch adds support for filtering dongles that claim to be a Chromecast but are not.
1. Spin through all the headers
2. Add the SERVER header to the service object
3. Use the service.server property to filter Chromecasts out of the list of devices, since the code does not support them yet.
Attachment #8430971 - Flags: review?(wjohnston)
Attachment #8429785 - Flags: review?(wjohnston) → review+
Comment on attachment 8430361 [details] [diff] [review]
target-filtering v0.1

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

Code seems fine. Names need to be fixed up.

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +227,5 @@
>  
> +  registerTarget: function registerTarget(aTarget) {
> +    // We must have "target" and "factory" defined
> +    if (!("target" in aTarget) || !("factory" in aTarget)) {
> +      return;

This should throw. A silent return is useless.

@@ +234,2 @@
>      // Only add if we don't already know about this target
> +    if (!this._targets.has(aTarget.target)) {

aTarget.target? Something needs to be renamed here. aTarget.name/id?

@@ +280,5 @@
> +  // Returns false if the service does not match the target's filters
> +  _filterService: function _filterService(aService) {
> +    let target = this._targets.get(aService.target);
> +    if (!target) {
> +      log("Did not find a target");

Probably somethign like "Target " + aService.targetId + " is not registered" would be more descriptive. Its confusing that service.target is an id here and target is an object.

@@ +286,5 @@
> +    }
> +
> +    // If we have no filter, everything passes
> +    if (!("filters" in target)) {
> +      log("No filters specified");

No need for this logging.

@@ +295,5 @@
> +    let filters = target.filters;
> +    for (let filter in filters) {
> +      if (filter in aService && aService[filter] != filters[filter]) {
> +        log("Failed filter: " + filter + "=" + filters[filter]);
> +        log(" " + aService[filter]);

Or this. If you want it, it should be behind a flag.
Attachment #8430361 - Flags: review?(wjohnston) → review+
Attachment #8430971 - Flags: review?(wjohnston) → review+
You need to log in before you can comment on or make changes to this bug.