[Wifi Direct] Support bonjour/upnp service advertisement

NEW
Unassigned

Status

Firefox OS
Wifi
3 years ago
2 years ago

People

(Reporter: hchang, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

20.31 KB, patch
hchang
: review+
Details | Diff | Splinter Review
1.57 KB, patch
hchang
: review+
Details | Diff | Splinter Review
21.08 KB, patch
Details | Diff | Splinter Review
10.07 KB, patch
Details | Diff | Splinter Review
18.96 KB, patch
hchang
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Assignee: nobody → hchang
(Reporter)

Updated

3 years ago
Blocks: 1131632

Comment 1

2 years ago
Created attachment 8704575 [details] [diff] [review]
Part 1 - Add WifiP2pServiceDiscovery.jsm for the implementation of p2p service discovery objects.

Comment 2

2 years ago
Created attachment 8704576 [details] [diff] [review]
Part 2 - Add wifi commands for the p2p service discovery.

Comment 3

2 years ago
Created attachment 8704581 [details] [diff] [review]
Part 3 -  Modify the Wifi P2P manager for adding the support of P2P service discovery.

Comment 4

2 years ago
Created attachment 8704582 [details] [diff] [review]
Part 4 - Add webidl and modify the MozWifiP2pManager for P2P service discovery.

Updated

2 years ago
Attachment #8704575 - Flags: review?(hchang)

Updated

2 years ago
Attachment #8704576 - Flags: review?(hchang)

Updated

2 years ago
Attachment #8704581 - Flags: review?(hchang)

Updated

2 years ago
Attachment #8704582 - Flags: review?(hchang)
(Reporter)

Updated

2 years ago
Attachment #8704576 - Flags: review?(hchang) → review+
(Reporter)

Comment 5

2 years ago
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".
Attachment #8704582 - Flags: review?(hchang) → feedback+

Comment 6

2 years ago
(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

2 years ago
Created attachment 8708227 [details] [diff] [review]
Part 5 - Add webidls for the WiFi P2P service discovery.

Comment 8

2 years ago
Created attachment 8708228 [details] [diff] [review]
Part 4 - Modify the MozWifiP2pManager for P2P service discovery.
Attachment #8708228 - Flags: review?(hchang)

Updated

2 years ago
Attachment #8704582 - Attachment is obsolete: true

Comment 9

2 years ago
Created attachment 8708232 [details] [diff] [review]
Part 4 - Modify the MozWifiP2pManager for P2P service discovery.
Attachment #8708232 - Flags: review?(hchang)

Updated

2 years ago
Attachment #8708228 - Attachment is obsolete: true
Attachment #8708228 - Flags: review?(hchang)
(Reporter)

Comment 10

2 years ago
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?
Attachment #8708232 - Flags: review?(hchang) → review+
(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.
(Reporter)

Comment 12

2 years ago
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.
Attachment #8704575 - Flags: review?(hchang) → review+
Created attachment 8731084 [details] [diff] [review]
Part 4 - Add webidl and modify the MozWifiP2pManager for P2P service discovery.
Created attachment 8731085 [details] [diff] [review]
Part 3 -  Modify the Wifi P2P manager for adding the support of P2P service discovery.
Created attachment 8731086 [details] [diff] [review]
Part 2 - Add wifi commands for the p2p service discovery.
Remove attachments that are attached incorrectly.

Updated

2 years ago
Attachment #8731084 - Attachment is obsolete: true
Attachment #8731085 - Attachment is obsolete: true
Attachment #8731086 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Assignee: hchang → nobody
(Reporter)

Comment 17

2 years ago
No longer working on this one.
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.
Attachment #8704581 - Flags: review?(hchang)
You need to log in before you can comment on or make changes to this bug.