Switch to a unique ID when registering targets

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
Screencasting
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Trunk
Firefox 35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8478789 [details] [diff] [review]
target-id v0.1

Currently, we use the SSDP "target" as a unique ID when registering device targets (Roku, Chromecast, etc), but this creates a problem: What if two or more devices have the same target?

It turns out that DIAL is used by multiple devices, but we can only register one.

This patch switches to use a non-SSDP unique ID for each device and uses the ID when registering the targets.

We need to land this before switching Roku to use DIAL (bug 1054056) or add any new DIAL devices.


This patch bitrots my patch in bug 1020564 but I'll update it and ask for a new review.
Attachment #8478789 - Flags: review?(wjohnston)
Comment on attachment 8478789 [details] [diff] [review]
target-id v0.1

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

r- until the naming is fixed.

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

Can we just generate this id? AFAICT, its only use it to let us map service <---> target which is all internal work. Then no one outside needs to know it exists...

@@ +272,5 @@
>      }
>  
>      // Only add if we don't already know about this target
> +    if (!this._targets.has(aTarget.id)) {
> +      this._targets.set(aTarget.id, aTarget);

We should probably log or throw something if we hit this.

@@ +278,5 @@
>    },
>  
>    unregisterTarget: function unregisterTarget(aTarget) {
>      // We must have "target" and "factory" defined
> +    if (!("id" in aTarget) || !("target" in aTarget) || !("factory" in aTarget)) {

And here

@@ +289,5 @@
>      }
>    },
>  
>    findAppForService: function findAppForService(aService) {
> +    if (!aService || !aService.device) {

? service.device == target.id? We need to fix this naming. Since you mentioned in the meeting last week, my complaints about this code aren't the code's format (although its bad in some places). My complaints are mostly to do with the naming making it hard to follow, which makes it hard for anyone to contribute to or review (i.e. we're not shipping stuff because I'm wasting time trying to re-learn that target in place A is an object and in place B its a string). These "target" and "service" objects are confusing and ill-defined. In addition, we just shove properties on them wherever we want, making them monster objects to do everything instead of one thing (i.e. we have no interfaces). AFAICT from very slowly ripping the code apart they look something like:

// Service is basically a list of info about a particular device. It gets passed to the factory to make things like RokuApp.
service = {
  location // ip to send requests to
  target   // reference to the ssdp query that was used to find this device
  server   // No idea. Not used anywhere I can see. Just storing extra info? Is this just a duplicate of location?
  friendlyName:    // A name to show in the ui
  uuid             // a unique id for this service.
  manufacturer     // details for uniquely identifying this device. not used
  modelName        // details for uniquely identifying this device. used in one filter
  mirror           // does this support mirroring
  lastPing         // time we last pinged this.
  extensions       // file extensions this supports, copied from the target
  types            // mimetypes this supports. compied from the target
  appsURL          // url to query for available apps. (I really want to combine this with things like mirror into just an "apps" getter than returns
                   // "apps" we support on this device, but we'd still need to cache this url somewhere.)
  device           // NEW IN THIS PATCH - maps to target.id
}

// Target is a smaller list of info to help discovering devices
target = {
  id       // NEW IN THIS PATCH, uniquely identifies a target. Note that one target may map to several services
  target   // string to use when querying ssid service
  location // only used for fixed targets. Passed to service
  factory  // builds an App() object based on the service (note the App doesn't control an App, it controls the device and launches apps on it).
  extensions // File extensions this supports
  types    // mime types this supports
  filters  // used to identify devices types. filters must match whats in the service
}

I think this targets->services work was probably an over-optimization done at the start, and probably gives negligible win (since our chrome JS is compiled and cached and we've never see appreciable wins when we try and optimize its loading). But its not going away, so I'd like to at least make sure the naming is consistent and makes sense so that these reviews are easier. i.e. Now that I have a description of what these look like, lets rename! How about something like:

target.target => target.ssdpQuery
service.target => service.ssdpQuery
service.device => service.targetID
service => deviceInfo/routeInfo? (RouteInfo matches Android, but we aren't really building a media router here, so I prefer device)
target => deviceFactory? This holds more than that, but its a larger bug to fix that.

All the targetId/ssdpQuery renaming are related to this bug. I'd like at least to see them done here.

@@ +339,5 @@
> +          if (filter in aService && aService[filter] != filters[filter]) {
> +            failed = true;
> +          }
> +        }
> +        if (!failed) {

white space between if-else
Attachment #8478789 - Flags: review?(wjohnston) → review-
(In reply to Wesley Johnston (:wesj) from comment #1)

> ? service.device == target.id? We need to fix this naming. Since you
> mentioned in the meeting last week, my complaints about this code aren't the
> code's format (although its bad in some places). My complaints are mostly to
> do with the naming making it hard to follow, which makes it hard for anyone
> to contribute to or review (i.e. we're not shipping stuff because I'm
> wasting time trying to re-learn that target in place A is an object and in
> place B its a string). These "target" and "service" objects are confusing
> and ill-defined. In addition, we just shove properties on them wherever we
> want, making them monster objects to do everything instead of one thing
> (i.e. we have no interfaces). AFAICT from very slowly ripping the code apart
> they look something like:

Remember bug 1020564. That will change "target" to "device". I will rebase this patch on top of it.

> // Service is basically a list of info about a particular device. It gets
> passed to the factory to make things like RokuApp.
> service = {
>   location // ip to send requests to
>   target   // reference to the ssdp query that was used to find this device

SSDP refers to this as the "search target" so I'd like to stick with "target" especially since we'll start using "device" for the "other target" object.

>   server   // No idea. Not used anywhere I can see. Just storing extra info?
> Is this just a duplicate of location?

No. "server" is the UA of the server on the device. Sadly, we use it to tell the Firefly from the Chromecast, since the FIrefly tries to act like a clone of a Chromecast. We should remove this when Firefly stops goofing around. So it's used in a filter.

>   device           // NEW IN THIS PATCH - maps to target.id

This will make more sense when bug 1020564 lands (or this patch is rebased on to it)

> 
> // Target is a smaller list of info to help discovering devices
> target = {

Becoming "device" in bug 1020564

> I think this targets->services work was probably an over-optimization done
> at the start, and probably gives negligible win (since our chrome JS is
> compiled and cached and we've never see appreciable wins when we try and
> optimize its loading). But its not going away, so I'd like to at least make
> sure the naming is consistent and makes sense so that these reviews are
> easier. i.e. Now that I have a description of what these look like, lets
> rename! How about something like:
> 
> target.target => target.ssdpQuery

This will become device.target after bug 1020564

> service.target => service.ssdpQuery

I want to keep "target" for both device and service

> service.device => service.targetID

service.device makes sense after bug 1020564, although I would be OK with service.deviceID

> service => deviceInfo/routeInfo? (RouteInfo matches Android, but we aren't
> really building a media router here, so I prefer device)

I'm not sold on this one

> target => deviceFactory? This holds more than that, but its a larger bug to
> fix that.

I like "device" and it has a device.factory

> All the targetId/ssdpQuery renaming are related to this bug. I'd like at
> least to see them done here.

Agreed, but I will rebase this on bug 1020564

I will also update the patch for your other code comments
Created attachment 8483049 [details] [diff] [review]
target-id v0.2

Updated with some naming changes:
* service.device -> service.deviceID

Code comments addressed:
* Log if device is already registered in registerDevice
* Log is device is not registered in unregisterDevice
* if block spacing

I did not add auto ID generation. We could go back and add it if we want. I am also using the ID in the above Logs.
Assignee: nobody → mark.finkle
Attachment #8478789 - Attachment is obsolete: true
Attachment #8483049 - Flags: review?(wjohnston)
Comment on attachment 8483049 [details] [diff] [review]
target-id v0.2

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

Thanks :)

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +328,5 @@
>    _filterService: function _filterService(aService) {
> +    // Loop over all the devices, looking for one that matches the service
> +    for (let [key, device] of this._devices) {
> +      // First level of match is on the target itself
> +      if (device.target == aService.target) {

I would probably flip this condition:

if (device.target != aService.target) {
  continue;
}
Attachment #8483049 - Flags: review?(wjohnston) → review+
I think you could probably add a test for this pretty easily too?
I flipped the condition as suggested. Good idea.

https://hg.mozilla.org/integration/fx-team/rev/881e56c64995

(In reply to Wesley Johnston (:wesj) from comment #5)
> I think you could probably add a test for this pretty easily too?

We have two tests that need the code to work or they fail, so we have some coverage. I have ideas for more.
https://hg.mozilla.org/mozilla-central/rev/881e56c64995
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.