1131493 - [Wifi Direct] Support bonjour/upnp service advertisement

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Wifi, defect)

Description

[Henry Chang [:hchang]]

We need an API like [1] to support register/request upnp/bonjour service.

[1] http://developer.android.com/training/connect-devices-wirelessly/nsd-wifi-direct.html

Comment 1

[Tim Huang[:timhuang]]

Attachment: Part 1 - Add WifiP2pServiceDiscovery.jsm for the implementation of p2p service discovery objects.

Comment 2

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add wifi commands for the p2p service discovery.

Comment 3

[Tim Huang[:timhuang]]

Attachment: Part 3 - Modify the Wifi P2P manager for adding the support of P2P service discovery.

Comment 4

[Tim Huang[:timhuang]]

Attachment: Part 4 - Add webidl and modify the MozWifiP2pManager for P2P service discovery.

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8704582 [details] [diff] [review]
Part 4 - Add webidl and modify the MozWifiP2pManager for P2P service discovery.

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

I can't review the webidls. Could you move webidls to another patch? Thanks!

::: dom/webidl/MozWifiP2pManager.webidl
@@ +214,5 @@
> +   * onsuccess: Command succeeded.
> +   * onerror:   Command failed.
> +   *
> +   */
> +  DOMRequest clearServiceDiscoveryRequests(optional DOMString? addr);

Since addServiceDiscoveryRequest and removeServiceDiscoveryRequest is designed to use a explicit wildcard MAC address for "all devices" semantic, why not take the same approach here? (Actually I like the "optional" approach.)

::: dom/wifi/DOMWifiP2pManager.js
@@ +331,5 @@
> +          let servResps = msg.servResps;
> +
> +          for (let i = 0 ; i < servResps.length; i++) {
> +            let servResp = servResps[i];
> +            if ( servResp._serviceType === P2pServReq.SERVICE_TYPE_BONJOUR ) {

No white spaces between parentheses and the condition.

@@ +333,5 @@
> +          for (let i = 0 ; i < servResps.length; i++) {
> +            let servResp = servResps[i];
> +            if ( servResp._serviceType === P2pServReq.SERVICE_TYPE_BONJOUR ) {
> +              this._fireBonjourDiscoveredEvent(servResp);
> +            } else if ( servResp._serviceType === P2pServReq.SERVICE_TYPE_UPNP  ) {

should be: "if (servResp._serviceType === P2pServReq.SERVICE_TYPE_UPNP)"

@@ +470,5 @@
> +  },
> +
> +  addServiceDiscoveryRequest: function(aAddr, aServReq) {
> +    let request = this.createRequest();
> +    let servReq = this.convertToJSOM(aServReq);

covertToJSON?

@@ +542,5 @@
>        }
>      });
>    },
> +
> +  convertToJSOM: function(obj) {

The function name is a little confusing. According to the implementation, is it a shallow copy function? I'd like it to be just "clone" or "copy".

Comment 6

[Tim Huang[:timhuang]]

(In reply to Henry Chang [:henry] from comment #5)
> Comment on attachment 8704582 [details] [diff] [review]
> Part 4 - Add webidl and modify the MozWifiP2pManager for P2P service
> discovery.
> 
> Review of attachment 8704582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't review the webidls. Could you move webidls to another patch? Thanks!
> 
> ::: dom/webidl/MozWifiP2pManager.webidl
> @@ +214,5 @@
> > +   * onsuccess: Command succeeded.
> > +   * onerror:   Command failed.
> > +   *
> > +   */
> > +  DOMRequest clearServiceDiscoveryRequests(optional DOMString? addr);
> 
> Since addServiceDiscoveryRequest and removeServiceDiscoveryRequest is
> designed to use a explicit wildcard MAC address for "all devices" semantic,
> why not take the same approach here? (Actually I like the "optional"
> approach.)
> 

That's a bit confusing here. The "00:00:00:00:00:00", the wild card address, also represents one device. But the difference from other MAC address is that the wild card address will be received by all devices. And if we leave addr blank here will clean all devices, in other words, the device of the wild card address and other devices. Thus, this is the reason of your question.

> @@ +470,5 @@
> > +  },
> > +
> > +  addServiceDiscoveryRequest: function(aAddr, aServReq) {
> > +    let request = this.createRequest();
> > +    let servReq = this.convertToJSOM(aServReq);
> 
> covertToJSON?
> 
> @@ +542,5 @@
> >        }
> >      });
> >    },
> > +
> > +  convertToJSOM: function(obj) {
> 
> The function name is a little confusing. According to the implementation, is
> it a shallow copy function? I'd like it to be just "clone" or "copy".

Yes, I agree your suggestion, I would change the name into cloneToJSON.

Comment 7

[Tim Huang[:timhuang]]

Attachment: Part 5 - Add webidls for the WiFi P2P service discovery.

Comment 8

[Tim Huang[:timhuang]]

Attachment: Part 4 - Modify the MozWifiP2pManager for P2P service discovery.

Comment 9

[Tim Huang[:timhuang]]

Attachment: Part 4 - Modify the MozWifiP2pManager for P2P service discovery.

Comment 10

[Henry Chang [:hchang]]

Comment on attachment 8708232 [details] [diff] [review]
Part 4 - Modify the MozWifiP2pManager for P2P service discovery.

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

Good job, Tim! Only one question in the comment. Thanks!

::: dom/wifi/DOMWifiP2pManager.js
@@ +470,5 @@
> +  },
> +
> +  addServiceDiscoveryRequest: function(aAddr, aServReq) {
> +    let request = this.createRequest();
> +    let servReq = this.cloneToJSON(aServReq);

Only one question: why do you need to clone it but not just pass is to this._sendMessageForRequest? Is it any IPC or security issue?

Comment 11

[Tim Huang[:timhuang]]

(In reply to Henry Chang [:henry] from comment #10)
> Comment on attachment 8708232 [details] [diff] [review]
> Part 4 - Modify the MozWifiP2pManager for P2P service discovery.
> 
> Review of attachment 8708232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good job, Tim! Only one question in the comment. Thanks!
> 
> ::: dom/wifi/DOMWifiP2pManager.js
> @@ +470,5 @@
> > +  },
> > +
> > +  addServiceDiscoveryRequest: function(aAddr, aServReq) {
> > +    let request = this.createRequest();
> > +    let servReq = this.cloneToJSON(aServReq);
> 
> Only one question: why do you need to clone it but not just pass is to
> this._sendMessageForRequest? Is it any IPC or security issue?

Because the aServReq is a XPCOM object, and a XPCOM object cannot directly pass through frame message manager due to the security issue.

Comment 12

[Henry Chang [:hchang]]

Comment on attachment 8704575 [details] [diff] [review]
Part 1 - Add WifiP2pServiceDiscovery.jsm for the implementation of p2p service discovery objects.

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

It generally looks very good to me. Good job!

Please address the comments and also

1) Use |let| rather than |var|
2) It should be queries but not querys
3) Some function names are not self-explained enough and the spec of the args are not simple enough. I'd suggest add comments for those spots.

::: dom/wifi/WifiP2pServiceDiscovery.jsm
@@ +71,5 @@
> +    this._transId = aTransId;
> +  }
> +
> +  function validateQuery(aQuery) {
> +    if (!aQuery)

Maybe add a comment to describe the use the aQuery when it's falsy.

@@ +72,5 @@
> +  }
> +
> +  function validateQuery(aQuery) {
> +    if (!aQuery)
> +      return;

if (!aQuery) {
  return;
}

@@ +129,5 @@
> +    if (this._protocolType !== aCompareReq._protocolType) {
> +      return false;
> +    }
> +
> +    if (!this._query && !aCompareReq._query) {

Can it be written as

return this._query === aCompareReq._query?

@@ +229,5 @@
> +
> +// The constructor of P2pServDesc.
> +//
> +// @param The array of service querys.
> +this.P2pServDesc = function(aQuerys) {

It should be "queries".

@@ +312,5 @@
> +
> +    var vmpack = Object.create(this._vmPack);
> +
> +    if (this._dnsQueryName) {
> +      vmpack[0x27] = this._dnsQueryName;

How does 0x27 come from?

@@ +519,5 @@
> +    throw "The instance name of service type should not be empty.";
> +  }
> +
> +  let querys = [];
> +

Need a comment to explain why we have two queries in one service description.

@@ +553,5 @@
> +                                     SERVICE_BONJOUR_VERSION_1);
> +  query += " ";
> +
> +  let isEmpty = true;
> +

Maybe just return "00" when aTxtMap is empty here to get rid of |isEmpty| flag.

@@ +556,5 @@
> +  let isEmpty = true;
> +
> +  for (let key in aTxtMap) {
> +    if (aTxtMap.hasOwnProperty(key)) {
> +      let keyValStr = key +"="+ aTxtMap[key];

let keyValStr = key + "=" + aTxtMap[key];

@@ +687,5 @@
> +  var len = aHex.length / 2;
> +  var data = [];
> +
> +  for (let i =0; i < len; i++) {
> +    data.push(parseInt(aHex.substring(i*2, i*2+2),16));

data.push(parseInt(aHex.substring(i*2, i*2+2), 16));

@@ +702,5 @@
> +  }
> +
> +  return str;
> +}
> +

Need a short description to explain what it does since the function name is not explicit enough.

Comment 13

[Tim Huang[:timhuang]]

Attachment: Part 4 - Add webidl and modify the MozWifiP2pManager for P2P service discovery.

Comment 14

[Tim Huang[:timhuang]]

Attachment: Part 3 - Modify the Wifi P2P manager for adding the support of P2P service discovery.

Comment 15

[Tim Huang[:timhuang]]

Attachment: Part 2 - Add wifi commands for the p2p service discovery.

Comment 16

[Tim Huang[:timhuang]]

Remove attachments that are attached incorrectly.

Comment 17

[Henry Chang [:hchang]]

No longer working on this one.

Comment 18

[Ethan Tseng [:ethan]]

Comment on attachment 8704581 [details] [diff] [review]
Part 3 -  Modify the Wifi P2P manager for adding the support of P2P service discovery.

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

Cancel the review request since we are not working on this bug now.

Comment 19

[BMO Automation]

Firefox OS is not being worked on