811635 - B2G Wifi: Support Wifi Direct

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Provide DOM API and related implementation for Wifi Direct.

Comment 1

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Current progress
1. Create connection with Galaxy SII by command line.
2. Create connection between otoro/unagi by command line.
3. API draft

Behaviors on Android system(Galaxy SII, Android 4.0.3)
1. Android supports only 1-to-1 connection, although Wifi Direct spec supports group operation and compatibility with normal client.
   Not sure if limited by wpa_supplicant.
2. Require successful DHCP IP discovery to enter connected state, although Wifi Direct is a MAC layer protocol.
3. On receiving Wifi Direct connect request, no matter what app is currently running, a system window requesting WPS operation appears on top.

Comment 2

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Wifi Direct API v0.1

Draft of Wifi Direct API

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Wifi Direct API v0.2

1. Create wifiDirectManager for wifi direct API.
2. Refine function names.

Comment 4

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Wifi Direct API v0.3

1. Proposed API is tested with a workable prototype being able to perform following operations:
   (1) Scan for wifi direct devices.
   (2) Being scanned as wifi direct device.
   (3) Create wifi direct connection with wifi-direct-enabled Galaxy SII as station or group owner, with DHCP support.
   (4) Create wifi direct connection between two wifi-direct-enabled otoro as station or group owner, with DHCP support.
   (5) Disconnect a wifi direct connection
2. TODOs
   (1) Report IP for connected peer to create connection.
       Currently station device can get IP for Group Owner, but Group Owner can't get IP of station.
       Take Tethering into consideration, maybe we need a daemon for controlling DHCP client/server service?
   (2) Handling WPS event.
       PIN input/display or PBC windows should be shown whenever WPS request is received and wait for user to response.
       Lack of such mechanism currently, and need to figure out how to send user response to back to wifi worker.
   (3) Seperate event handling for wifi and wifi direct more cleanly.
   (4) API for Service add/remove/search
       This is supported in android API, but lack of reference data. Failed to bring up such functionality by command for now.

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gecko WIP : 0001.IDL

Comment 6

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gecko WIP : 0002.DOM Implementation

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gecko WIP : 0003 Add DOM to tree

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gecko WIP : 0004.Support Wifi Direct in WifiWorker

It seems to be necessary to use different state handler for wifi/wifi direct.
Needs refactory.

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gecko WIP : 0005.Add new libnetutils

Add API to control dhcp server.
It's better provide a service for controlling dhcp server/client, and provide dhcp status and data.

Comment 10

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gaia Test App WIP : 0001.Add config

Comment 11

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gaia Test App WIP : 0002.Add Wifi Direct UI entry in settings

Comment 12

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gaia Test App WIP : 0003.Get Wifi Direct DOM

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gaia Test App WIP : 0004.Wifi Direct control app

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: System Core WIP : 0001.Add dhcp server control

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: System Core WIP : 0002.Add dhcp server init entry

Not really affect system now because it is not included into image.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Chuck, what's the target milestone for this bug and why are we working on it now? Is this for a partner or something?

Comment 17

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

It's for C2, as I know.
I did this mainly for a mighe-be-useful practice while studying the whole wifi structure.
I was assigned with other tasks few days ago and might not be able to focus on wifi for a certain time.
So I think it's better to expose/backup my current progress, so others to can take over/review/refine/whatever if they want.

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

s/C2/V2, sorry for typo

Comment 19

[Henry Chang [:hchang]]

Attachment: Gecko WIP: 0001.DOM_IDL.patch (rebased)

Comment 20

[Henry Chang [:hchang]]

Attachment: Gecko WIP: 0002.DOM_Implementation (rebased)

Comment 21

[Henry Chang [:hchang]]

Attachment: Gecko WIP: 0003.Add_DOM_to_tree (rebased)

Comment 22

[Henry Chang [:hchang]]

Attachment: Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker (rebased)

Comment 23

[Henry Chang [:hchang]]

Attachment: Gecko WIP: 0005.Add_new_libnetutils (rebased)

Comment 24

[Henry Chang [:hchang]]

Attachment: Gaia Test App WIP: 0001.Addconfig (rebased)

Comment 25

[Henry Chang [:hchang]]

Attachment: Gaia Test App WIP: 0002.AddwifidirectUIentryinsettings (rebased)

Comment 26

[Henry Chang [:hchang]]

Attachment: Gaia Test App WIP: 0003.Get WifiDirect DOM (rebased)

Comment 27

[Henry Chang [:hchang]]

Attachment: Gaia Test App WIP: 0004.WifiDirect Control App (rebased)

Comment 28

[Henry Chang [:hchang]]

I've attached all the rebased and conflict-resolved patches for Gecko and Gaia. The hardcoded PIN is 11111111.

Comment 29

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 688138 [details] [diff] [review]
System Core WIP : 0001.Add dhcp server control

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

To prevent modifying non-Gecko part, this patches should be dropped and implement DHCP Server control in bug 900847 instead.

Comment 30

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 688139 [details] [diff] [review]
System Core WIP : 0002.Add dhcp server init entry

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

To prevent modifying non-Gecko part, this patches should be dropped and implement DHCP Server control in bug 900847 instead.

Comment 31

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 784745 [details] [diff] [review]
Gecko WIP: 0005.Add_new_libnetutils (rebased)

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

To prevent modifying non-Gecko part, this patches should be dropped and implement DHCP Server control in bug 900847 instead.

Comment 32

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 784744 [details] [diff] [review]
Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker (rebased)

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

::: dom/wifi/WifiWorker.js
@@ +801,5 @@
> +    controlMessage({ cmd: "dhcp_server_stop"}, function(data) {
> +      callback(!data.status);
> +                  });
> +  }
> +

Use control function provided by bug 900847 instead.

Comment 33

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 784744 [details] [diff] [review]
Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker (rebased)

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

Modified required for this patch:
1. Use control function provided by bug 900847 instead.
2. Use a system message to replace currently hard-coded user response.
   These hard-coded part will move to Gaia test app.
3. PBC seems to be a special case that we use different command while request connection to others and being requested for connection from others.
   We have to find a way to distinguish that in single connect() API.

Comment 34

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Gaia Test app - Experimental settings UI for Wifi Direct.

Receive wifi direct pairing system message and show corresponding UI.
Only valid when settings app is in foreground.

Reference github : https://github.com/chuck-lee/gaia/tree/bug-811635_Wifi_Direct

Comment 35

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 784740 [details] [diff] [review]
Gecko WIP: 0001.DOM_IDL.patch (rebased)

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

Side note:
Maybe use p2p_stop_find to disable all p2p functionality.

::: dom/wifi/nsIDOMWifiDirectManager.idl
@@ +56,5 @@
> +   * Current Status
> +   * - Supported
> +   *
> +   */
> +  nsIDOMDOMRequest enableScan();

use scan();

@@ +73,5 @@
> +   * Current Status
> +   * - Supported
> +   *
> +   */
> +  nsIDOMDOMRequest disableScan();

use listen();

@@ +83,5 @@
> +   *    {
> +   *      .address:           Mac of peer
> +   *      .groupOwnerIntent:  Intention of this device becoming a group owner
> +   *                          range from 1 to 15, larger indicates greater
> +   *                          intention always set to 7?

Add new id:
       .wps : {        // optional
          .method: "KEYPAD", "DISPLAY", or "PBC"    // Method used to connect 
          .pin:                                     // optional, PIN code for KEYPAD or DISPLAY
       }

@@ +124,5 @@
> +   * Current Status
> +   * - Supported
> +   *
> +   */
> +  nsIDOMDOMRequest disconnect();

maybe use disconnect(peerInfo);
in case we support multiple peers.

@@ +172,5 @@
> +   * - Supported
> +   *
> +   */
> +  readonly attribute jsval groupOwner;
> +

add
readonly attribute bool isGroupOwner;

@@ +184,5 @@
> +   * Current Status
> +   * - Supported
> +   *
> +   */
> +  readonly attribute jsval currentPeer;

maybe use coonectedPeers[], for supporting multiple peers.

Comment 36

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Mandatory:
1. 1-to-1 connection.
2. Wifi Direct in settings.
3. UI for WPS action.
   - By system message, like bluetooth pairing.

Optional:
1. Get peer IP
   - Read /data/misc/dhcp/dnsmasq.leases by File API.
   - Discover by app itself.
2. Support coexist of Wifi(STA or AP) and Wifi Direct
   - Might not supported on all platform, backward compatibility must be taken into consideration.
   - Supported in Galaxy S III.

Future:
1. Support multiple peer.
2. Support UPnP service register/handler.

Comment 37

[Henry Chang [:hchang]]

Attachment: Gecko-0001: Adding DOMWifiDirectManager.idl

Comment 38

[Henry Chang [:hchang]]

Attachment: Gecko-0002: Adding DOMWifiDirectManager.js

Comment 39

[Henry Chang [:hchang]]

Attachment: Gecko-0003: Adding WifiWorker support.patch

Comment 40

[Henry Chang [:hchang]]

Attachment: Gecko-0004: Adding NetworkManager support

Comment 41

[Henry Chang [:hchang]]

Attachment: Gecko-0005: Adding installer/manifest/build tree

Comment 42

[[:fabrice] Fabrice Desré]

Henri, great doc in the idl! Can we just use webidl now and not have to convert later?

Comment 43

[Henry Chang [:hchang]]

Removed dependencies on system/core/libnetutil and fixed some wifi regression

Comment 44

[Henry Chang [:hchang]]

Hi Fabrice: Sure! But we are still revising the web APIs. We will in the end use webidl rather than idl. Thanks.

Comment 45

[Henry Chang [:hchang]]

Attachment: 0001_DOMWifiDirectManager_idl.patch

Comment 46

[Henry Chang [:hchang]]

Attachment: 0002_DOMWifiDirectManager_impl.patch

Comment 47

[Henry Chang [:hchang]]

Attachment: 0003_Wifi_backend_support.patch

Comment 48

[Henry Chang [:hchang]]

Attachment: 0004_NetworkManager_backend_support.patch

Comment 49

[Henry Chang [:hchang]]

Attachment: 0005_Build.patch

Comment 50

[Henry Chang [:hchang]]

Attachment: 0006_System_message_for_pairing_request.patch

Comment 51

[Henry Chang [:hchang]]

Attachment: 0001-Bug-811635-Gaia-Test-App.patch

Comment 52

[Henry Chang [:hchang]]

Attached new patches with the following changes:
1. Throwing out system message: "wifidirect-pairing-request" when necessary
2. Gaia prompting appropriate dialog when receiving "wifidirect-pairing-request"
3. Adding WifiDirectManager::setPairingConfirmation to DOM idl
4. Refactoring WifiWorker.js: Move some independent stuff to WifiDirectEventHandler.jsm

Verified:
1. Inviting peer with pbc/display/keypad method (Both GO/Client)
2. Being invited with display method (Both GO/Client

To do:
1. Making the least use of literal strings, including states, wps methods, ...
2. Refactoring WifiWorker: Do we really need WifiManager::onwifirectxxxx to be defined in WifiWorker object?

Known issues:
1. After a couple of connecting/disconnecting, wifi direct can no longer work

Comment 53

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part1: idl/webidl for WifiP2pManager

Comment 54

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part2: Implementation of webidl/idls

Comment 55

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part3: Adding system message "wifip2p-pairing-request"

Comment 56

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part4: p2p core implementation

Comment 57

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part5: Starting wpa_supplicant with proper p2pSupported flag

This patch bases on bug 912015 and additionally add  WifiUtil.cpp::GetWifiP2pSupported() and its reference by do_wifi_start_supplicant(). I will update this patch until bug 912015 lands.

Comment 58

[Henry Chang [:hchang]]

Attachment: The state transition of the p2p state machine defined in WifiP2pManager.jsm

Comment 59

[Henry Chang [:hchang]]

Attachment: Class diagram of wifi direct implementation

Comment 60

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part4: p2p core implementation

Added the support of p2p invitation and persistent group

Comment 61

[Henry Chang [:hchang]]

Attachment: Gaia part for simple testing

Comment 62

[Henry Chang [:hchang]]

See for clearer diff on bitbucket:

https://bitbucket.org/changhenry/mozilla-central/compare/bug%2F811635-wifip2p..default#diff

Comment 63

[_auzias]

IMO, Wi-Fi direct is not better than bluetooth. However having a "*create* an ad hoc network" ability would be great !

Here is an explanation why : https://bugzilla.mozilla.org/show_bug.cgi?id=928362

Comment 64

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 818220 [details] [diff] [review]
Bug 811635 - Part2: Implementation of webidl/idls

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

The patch looks really good. Thank you. r=me with comment addressed.

::: js/xpconnect/src/event_impl_gen.conf.in
@@ +36,1 @@
>      'MozWifiStatusChangeEvent',

This should be removed outside of MOZ_B2G_RIL flag after Bug 920551 is landed.

Comment 65

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 818224 [details] [diff] [review]
Bug 811635 - Part5: Starting wpa_supplicant with proper p2pSupported flag

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

Can you update the patch since bug 912015 is landed ?

Comment 66

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #65)
> Comment on attachment 818224 [details] [diff] [review]
> Bug 811635 - Part5: Starting wpa_supplicant with proper p2pSupported flag
> 
> Review of attachment 818224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you update the patch since bug 912015 is landed ?

Sure it will be done soon!

Comment 67

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #66)
> (In reply to Vincent Chang[:vchang] from comment #65)
> > Comment on attachment 818224 [details] [diff] [review]
> > Bug 811635 - Part5: Starting wpa_supplicant with proper p2pSupported flag
> > 
> > Review of attachment 818224 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can you update the patch since bug 912015 is landed ?
> 
> Sure it will be done soon!

btw, I will also update part 4 since WifiWorker.js was also updated in bug 912015.
Thanks.

Comment 68

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 818217 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager

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

The patch looks good. Please address the comments and ask mrbkap to do superreview.

::: dom/webidl/MozWifiP2pStatusChangeEvent.webidl
@@ +6,5 @@
> +
> +[Constructor(DOMString type, optional MozWifiP2pStatusChangeEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
> +interface MozWifiP2pStatusChangeEvent : Event
> +{
> +  readonly attribute DOMString peerAddress;

Please add comments about peerAddress.

::: dom/webidl/WifiP2pManager.webidl
@@ +15,5 @@
> +
> +dictionary ConnectionConfig {
> +  WPSMethod  wpsMethod;
> +  DOMString  address;
> +  byte?      goIntent;

Can we have some comments on address/goIntent ?

@@ +46,5 @@
> +   *   "connected",
> +   *   "disconnecting"
> +   * };
> +   *
> +   */

Nit: remove unused comment.

@@ +80,5 @@
> +   * =========================================================================
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Nit: remove the unused comments.

@@ +84,5 @@
> +   *
> +   */
> +  DOMRequest disableScan();
> +
> +  /**

How about combine two APIs to scan(boolean enable) ?

@@ +101,5 @@
> +   * =========================================================================
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Ditto.

@@ +120,5 @@
> +   * =========================================================================
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Ditto.

@@ +123,5 @@
> +   * - Supported
> +   *
> +   */
> +  DOMRequest disconnect(DOMString address);
> +

What address is it ? bssid address ?

@@ +138,5 @@
> +   *   .wpsCapabilities  array of the supported |WPSMethod|
> +   *   .connectionStatus one of the |ConnectionStatus|
> +   *
> +   * Current Status
> +   * - Supported

Ditto.

@@ +157,5 @@
> +   * Notify if the pending pairing request is accepted or rejected
> +   * =========================================================================
> +   *
> +   * Current Status
> +   * - Supported

Ditto.

@@ +167,5 @@
> +   * Returns if Wifi Direct is enabled
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Ditto.

@@ +180,5 @@
> +   * =========================================================================
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Ditto.

@@ +196,5 @@
> +   * =========================================================================
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Ditto.

@@ +206,5 @@
> +   * The address of the changed peer will be stored in event.peerList.
> +   * See MozWifiP2pStatusChangeEvent.webidl
> +   *
> +   * Current Status
> +   * - Supported

Ditto.

@@ +215,5 @@
> +  /**
> +   * An event listener that is called whenever Wifi Direct is enabled
> +   *
> +   * Current Status
> +   * - Supported

Ditto.

@@ +225,5 @@
> +   * An event listener that is called whenever Wifi Direct is disabled
> +   *
> +   * Current Status
> +   * - Supported
> +   *

Ditto.

@@ +228,5 @@
> +   * - Supported
> +   *
> +   */
> +  attribute EventHandler ondisabled;
> +};

Instead of using settings API, we have a plan to have APIs such as wifiEnable(boolean enable) and wifiHotSpotEnable(boolean enable) for wifi and wifiHotspot.   
Not quite sure if we still need onenabled/ondisabled APIs if we do the same thing for p2p.

::: dom/wifi/nsIDOMMozWifiP2pStatusChangeEvent.idl
@@ +15,5 @@
> +
> +    [noscript] void initMozWifiP2pStatusChangeEvent(in DOMString aType,
> +                                                    in boolean aCanBubble,
> +                                                    in boolean aCancelable,
> +                                                    in DOMString aPeerAddress);

Nit: indent with 2 spaces please.

Comment 69

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part4: p2p core implementation

Comment 70

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part5: Starting wpa_supplicant with proper p2pSupported flag

Comment 71

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #68)
> Comment on attachment 818217 [details] [diff] [review]
> Bug 811635 - Part1: idl/webidl for WifiP2pManager
> 
> Review of attachment 818217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good. Please address the comments and ask mrbkap to do
> superreview.
> 

Thanks for the review :)

> @@ +228,5 @@
> > +   * - Supported
> > +   *
> > +   */
> > +  attribute EventHandler ondisabled;
> > +};
> 
> Instead of using settings API, we have a plan to have APIs such as
> wifiEnable(boolean enable) and wifiHotSpotEnable(boolean enable) for wifi
> and wifiHotspot.   
> Not quite sure if we still need onenabled/ondisabled APIs if we do the same
> thing for p2p.
> 

The necessity of onenable/ondisable of Wifi depends on:
1) How we define DOMRequest.onsuccess of wifiEnable. If it means the operation is
   actually done and success, then we don't need this API.
2) Wifi wouldn't be turned on/off automatically in gecko.

Besides, I would suggest let the life cycle the same as wifi, which means turning on
wifi also turns on wifi direct and vice versa.

Comment 72

[Henry Chang [:hchang]]

(In reply to _auzias from comment #63)
> IMO, Wi-Fi direct is not better than bluetooth. However having a "*create*
> an ad hoc network" ability would be great !
> 
> Here is an explanation why :
> https://bugzilla.mozilla.org/show_bug.cgi?id=928362

Hi:

I agree wifi direct is not the best in all aspects but there's a
killing application on top of wifi direct already in the market: "wifi display".
We are going to implement wifi display (bug 925615)

Thanks :)

Comment 73

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 823867 [details] [diff] [review]
Bug 811635 - Part5: Starting wpa_supplicant with proper p2pSupported flag

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

Looks good.

Comment 74

[Henry Chang [:hchang]]

Hi Vincent:

One thing I need to sync up with you. I've added the p2p network interface and its register/unregister/update handling:

https://bitbucket.org/changhenry/mozilla-central/commits/87de2135579519d69708c710059e407639083c97

Without this modification, |Services.io.offline| won't be set to |true| and nsSocketTransport::InitiateSocket() will fail due to the status: "offline".

I will include this change in the patch after the review. Thanks :)

Comment 75

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 818217 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager

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

I would like to see an updated version of this patch with Vincent's comments addressed as well as my couple of comments below.

::: dom/webidl/WifiP2pManager.webidl
@@ +54,5 @@
> +   *
> +   * onsuccess: Succeeded to start wifi direct scan
> +   * onerror:   Failed to start wifi direct scan
> +   *
> +   * =================== Implementation Notes ================================

In addition to what Vincent said, it is weird to put implementation notes in the declaration of the interface. I'd rather that these go in the actual implementation. Also, as he mentioned it'd be odd to expose a function in the API that wasn't supported, so please remove those comments.

@@ +84,5 @@
> +   *
> +   */
> +  DOMRequest disableScan();
> +
> +  /**

I like the idea of combining the two functions, but I'd suggest "enableScan" as a name ("scan" alone implies that the function will return scan results instead of having a lasting effect).

@@ +103,5 @@
> +   * Current Status
> +   * - Supported
> +   *
> +   **/
> +  DOMRequest connect(optional ConnectionConfig config);

Why is the config optional?

@@ +160,5 @@
> +   * Current Status
> +   * - Supported
> +   *
> +   */
> +  DOMRequest setPairingConfirmation(optional ConfirmationResult result);

Why optional?

::: dom/wifi/nsIDOMMozWifiP2pStatusChangeEvent.idl
@@ +8,5 @@
> +interface nsIDOMMozWifiP2pStatusChangeEvent : nsIDOMEvent
> +{
> +    /**
> +     * Network object with a SSID field describing the network affected by
> +     * this change. This might be null.

This comment doesn't appear to apply to this interface.

Comment 76

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #75)
> Comment on attachment 818217 [details] [diff] [review]
> Bug 811635 - Part1: idl/webidl for WifiP2pManager
> 
> Review of attachment 818217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would like to see an updated version of this patch with Vincent's comments
> addressed as well as my couple of comments below.
> 
> @@ +103,5 @@
> > +   * Current Status
> > +   * - Supported
> > +   *
> > +   **/
> > +  DOMRequest connect(optional ConnectionConfig config);
> 
> Why is the config optional?
> 
> @@ +160,5 @@
> > +   * Current Status
> > +   * - Supported
> > +   *
> > +   */
> > +  DOMRequest setPairingConfirmation(optional ConfirmationResult result);
> 
> Why optional?

Thanks for the review!
I didn't make them optional until this error message from webidl parser:

From ../../dist/idl: Kept 1160 existing; Added/updated 0; Removed 0 files and 0 directories.
Traceback (most recent call last):
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/dom/bindings/GlobalGen.py", line 81, in <module>
    main()
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/dom/bindings/GlobalGen.py", line 56, in main
    parserResults = parser.finish()
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/dom/bindings/parser/WebIDL.py", line 4860, in finish
    production.finish(self.globalScope())
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/dom/bindings/parser/WebIDL.py", line 642, in finish
    member.finish(scope)
  File "/home/hchang/dev/b2g-hamachi/mozilla-central/dom/bindings/parser/WebIDL.py", line 3179, in finish
    [argument.location])
WebIDL.WebIDLError: error: Dictionary argument or union argument containing a dictionary not followed by a required argument must be optional, WifiP2pManager.webidl line 164:55
  DOMRequest setPairingConfirmation(ConfirmationResult result);
                                                       ^

Comment 77

[Henry Chang [:hchang]]

Attachment: Patch for hamachi p2p support (NOT for check-in)

Patch for testing p2p on hamachi or legacy devices

Comment 78

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 823866 [details] [diff] [review]
Bug 811635 - Part4: p2p core implementation

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

The patches look good generally, thank you. Please help to address the comments, especially for the async. calls in state machine.

::: dom/wifi/WifiCommand.jsm
@@ +355,5 @@
> +    } else {
> +      command += "go_intent=" + goIntent;
> +    }
> +
> +    debug('P2P connect command: ' + command);

Can we have better debug message on this ? Something like debug("P2P connect to" + address + .......) ?

@@ +360,5 @@
> +    doBooleanCommand(command, "OK", callback);
> +  };
> +
> +  command.p2pGroupRemove = function(iface, callback) {
> +    debug("groupRemove()");

ditto

@@ +365,5 @@
> +    doBooleanCommand("P2P_GROUP_REMOVE " + iface, "OK", callback);
> +  };
> +
> +  command.p2pEnable = function(deviceName, callback) {
> +    var deviceType = "10-0050F204-5";

Do we need to hard code this ?

@@ +387,5 @@
> +  command.p2pDisableScan = function(callback) {
> +    doBooleanCommand("P2P_STOP_FIND", "OK", callback);
> +  };
> +
> +  command.p2pGetGroupCapab = function(address, callback) {

s/p2pGetGroupCapab/p2pGetGroupCapability

@@ +389,5 @@
> +  };
> +
> +  command.p2pGetGroupCapab = function(address, callback) {
> +    command.p2pPeer(address, function(reply) {
> +      debug('p2p_peer reply: ' + reply);

ditto

@@ +394,5 @@
> +      if (!reply) {
> +        callback(0);
> +        return;
> +      }
> +      var capab = /group_capab=0x([0-9a-fA-F]+)/.exec(reply)[1];

s/capab/capability

::: dom/wifi/WifiP2pManager.jsm
@@ +15,5 @@
> +                                   "nsISystemMessagesInternal");
> +
> +this.EXPORTED_SYMBOLS = ["WifiP2pManager"];
> +
> +// Events form supplicant for p2p.

nit: s/form/from

@@ +39,5 @@
> +const EVENT_CTRL_EVENT_BSS_REMOVED       = 101;
> +const EVENT_CTRL_EVENT_SCAN_RESULTS      = 102;
> +const EVENT_WPS_AP_AVAILABLE             = 103;
> +const EVENT_AP_STA_CONNECTED             = 104;
> +const EVENT_AP_STA_DISCONNECTED          = 105;

I think we need also the supplicant lost event.(In case of wpa_supplicant is terminated or crashed)

@@ +65,5 @@
> +const WPS_METHOD_DISPLAY = "display";
> +const WPS_METHOD_KEYPAD  = "keypad";
> +
> +// Role string.
> +const P2P_ROLE_GO     = "GO";

s/P2P_ROLE_GO/P2P_ROLE_GROUP_OWNER

@@ +93,5 @@
> +  startIp: "10.10.10.2",
> +  endIp:   "10.10.10.254"
> +};
> +
> +const DEBUG = true;

It should be false after you get the r+.

@@ +107,5 @@
> +
> +  var _stateMachine = P2pStateMachine(aP2pCommand, aNetUtil);
> +
> +  // @param aObserver used to notify WifiWorker what's happening
> +  //        in the internal p2p state machine.

Since you have put the comments of function arguments here, it would be better if you could add the function description, too.

@@ +283,5 @@
> +    } else if (0 === aEventString.indexOf("P2P-GO-NEG-REQUEST")) {
> +      id = EVENT_P2P_GO_NEG_REQUEST;
> +      info.address = tokens[1];
> +      switch (parseInt(tokens[2].split("=")[1], 10)) {
> +        case 5: // DEV_PW_REGISTRAR_SPECIFIED = 0x0005. Peer is display.

Instead of using 5, please use const DEV_PW_REGISTRAR_SPECIFIED = 2 in the beginning of this file. 
So you can use case DEV_PW_REGISTRAR_SPECIFIED:

@@ +286,5 @@
> +      switch (parseInt(tokens[2].split("=")[1], 10)) {
> +        case 5: // DEV_PW_REGISTRAR_SPECIFIED = 0x0005. Peer is display.
> +          info.wpsMethod = WPS_METHOD_KEYPAD;
> +          break;
> +        case 1: // DEV_PW_USER_SPECIFIED = 0x0001. Peer is keypad.

Ditto.

@@ +289,5 @@
> +          break;
> +        case 1: // DEV_PW_USER_SPECIFIED = 0x0001. Peer is keypad.
> +          info.wpsMethod = WPS_METHOD_DISPLAY;
> +          break;
> +        case 4: // DEV_PW_PUSHBUTTON = 0x0004. Peer is pbc.

Ditto.

@@ +533,5 @@
> +      var retryCnt = 3;
> +      var retryTimer;
> +
> +      function connectToSupplicantWithRetry(aCallback) {
> +        aP2pCommand.connectToSupplicant(function(status) {

We should cancel the timer when exit() this state.

@@ +541,5 @@
> +            return;
> +          }
> +          retryCnt--;
> +          if (!retryTimer) {
> +            retryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Ditto. Please check the life cycle of retryTimer.

@@ +549,5 @@
> +          }, 5000, Ci.nsITimer.TYPE_ONE_SHOT);
> +        });
> +      }
> +
> +      connectToSupplicantWithRetry(function(success) {

We should make sure wpa_supplicant is started before connect to it, and this seem a duplicate because we have done the same thing in wifi right ?
Can we just receive a connect to supplicant event with result here ?

@@ +575,5 @@
> +        }
> +
> +        // Enable P2P.
> +        aP2pCommand.p2pEnable(_localDevice.deviceName, function(success) {
> +          if (!success) {

Is it possible that EVENT_P2P_PROV_DISC_PBC_REQ, EVENT_P2P_PROV_DISC_SHOW_PIN and EVENT_P2P_PROV_DISC_ENTER_PIN events are sent from wpa_supplicant before we move to stateInactive ?

@@ +588,5 @@
> +            }
> +            sm.sendEvent({ id: EVENT_P2P_ENABLE_SUCCESS });
> +          });
> +        });
> +

If we receive EVENT_P2P_CMD_DISABLE event in this state and you are in the middle of async. calls. 
How do you handle this case ? Do we need some internal states and events to handle these async. calls ?

@@ +709,5 @@
> +            }
> +          } else {
> +            debug('User rejected this request');
> +            gotoState(stateInactive); // Reset to inactive state.
> +          }

Nit: you can prevent a if {} else { } using early return,

if (!aEvent.info.accepted) {
gotoState(stateInactive);
break;
}

@@ +790,5 @@
> +            });
> +          } else {
> +            debug('User rejected this request');
> +            gotoState(stateInactive); // Reset to inactive state.
> +          }

Ditto: early return.

@@ +1119,5 @@
> +            debug('Supplicant connection closed');
> +            aNetUtil.disableInterface(P2P_INTERFACE_NAME, function (success){
> +              debug('Disabled interface: ' + P2P_INTERFACE_NAME);
> +              sm.sendEvent({ id: EVENT_P2P_DISABLE_SUCCESS });
> +            });

There might be state switch while we are in the middle of enter() function. We need to find a way to prevent this in others state, too.

@@ +1204,5 @@
> +
> +        debug('Happy p2p client~');
> +        aCallback(true);
> +      });
> +    }

if (!_groupInfo.isGroupOwner) {
    ......
  return;
}

do group owner stuff ....

@@ +1294,5 @@
> +                            stateConnected,
> +                            stateDisconnecting];
> +
> +    for (let i = 0; i < p2pManagedStates.length; i++) {
> +      if (aState === p2pManagedStates[i]) {

Instead of comparing object, can we compare the name of the state ?

@@ +1303,5 @@
> +    return false;
> +  }
> +
> +  function initTimeoutTimer(aTimeoutMs, aTimeoutEvent) {
> +    var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Please check the life cycle of timer. Users of instances of nsITimer should keep a reference to the timer until it is no longer needed in order to assure the timer is fired.
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsITimer

::: dom/wifi/WifiP2pWorkerObserver.jsm
@@ +224,5 @@
> +          aDomMsgResponder.setScanEnabled(true, function(success) {
> +            if (success) {
> +              returnMessage(aMessage.name, true, true, msg);
> +            } else {
> +              returnMessage(aMessage.name, false, "ERROR", msg);

Can we pass the success to returnMessage() so that we only need to do if() {} else {} things there once ?

@@ +235,5 @@
> +            if (success) {
> +              returnMessage(aMessage.name, true, true, msg);
> +            } else {
> +              returnMessage(aMessage.name, false, "ERROR", msg);
> +            }

Ditto.

@@ +262,5 @@
> +              if (success) {
> +                returnMessage(aMessage.name, true, true, msg);
> +              } else {
> +                returnMessage(aMessage.name, false, "ERROR", msg);
> +              }

Ditto.

@@ +279,5 @@
> +              if (success) {
> +                returnMessage(aMessage.name, true, true, msg);
> +              } else {
> +                returnMessage(aMessage.name, false, "ERROR", msg);
> +              }

Ditto.

::: dom/wifi/WifiWorker.js
@@ +118,5 @@
>        if (manager.ifname === iface && handleEvent(event)) {
>          waitForEvent(iface);
>        }
> +      else if (p2pSupported) {
> +        if (WifiP2pManager.INTERFACE_NAME === iface) {

Nit: 
  } else if (p2pSupported && WifiP2pManager.INTERFACE_NAME === iface) {
    ......
  }

@@ +273,5 @@
>          });
>        });
>        return;
>      }
> +    manager.handlePreWifiScan();

Please add comments to describe why do we need to call this function here ?

@@ +767,5 @@
>        if (reEnableBackgroundScan) {
>          reEnableBackgroundScan = false;
>          setBackgroundScan("ON", function() {});
>        }
> +      manager.handlePostWifiScan();

Please add comments to describe why do we need to call this function here ?

@@ +950,5 @@
> +      if (p2pSupported) {
> +        p2pManager.setEnabled(false, { onDisabled: doDisableWifi });
> +      } else {
> +        doDisableWifi();
> +      }

We should also doDisalbeWifi even if p2pSupported is true, right ?

@@ +1164,5 @@
>    }
>    manager.enableNetwork = function(netId, disableOthers, callback) {
> +    if (p2pSupported) {
> +      p2pManager.setScanEnabled(false, function(success) {});
> +    }

Why do we need to disable scan ? Please add the comments here.

Comment 79

[Henry Chang [:hchang]]

Thanks for the review!

Following are my reply and questions about the review:

(In reply to Vincent Chang[:vchang] from comment #78)
> Comment on attachment 823866 [details] [diff] [review]
> Bug 811635 - Part4: p2p core implementation
> 
> Review of attachment 823866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patches look good generally, thank you. Please help to address the
> comments, especially for the async. calls in state machine.
> 
> ::: dom/wifi/WifiCommand.jsm
> @@ +355,5 @@
> > +    } else {
> > +      command += "go_intent=" + goIntent;
> > +    }
> > +
> > +    debug('P2P connect command: ' + command);
> 
> Can we have better debug message on this ? Something like debug("P2P connect
> to" + address + .......) ?

Isn't it already good enough for debugging now since |command| contains all the information?

> @@ +365,5 @@
> > +    doBooleanCommand("P2P_GROUP_REMOVE " + iface, "OK", callback);
> > +  };
> > +
> > +  command.p2pEnable = function(deviceName, callback) {
> > +    var deviceType = "10-0050F204-5";
> 
> Do we need to hard code this ?
> 

In the latest enhancement, it's changed to read from ro.moz.wifi.device_type

> @@ +387,5 @@
> > +  command.p2pDisableScan = function(callback) {
> > +    doBooleanCommand("P2P_STOP_FIND", "OK", callback);
> > +  };
> > +
> > +  command.p2pGetGroupCapab = function(address, callback) {
> 
> s/p2pGetGroupCapab/p2pGetGroupCapability
> 
> @@ +389,5 @@
> > +  };
> > +
> > +  command.p2pGetGroupCapab = function(address, callback) {
> > +    command.p2pPeer(address, function(reply) {
> > +      debug('p2p_peer reply: ' + reply);
> 
> ditto
> 
> @@ +394,5 @@
> > +      if (!reply) {
> > +        callback(0);
> > +        return;
> > +      }
> > +      var capab = /group_capab=0x([0-9a-fA-F]+)/.exec(reply)[1];
> 
> s/capab/capability
> 

What's wrong with the naming? 

> @@ +39,5 @@
> > +const EVENT_CTRL_EVENT_BSS_REMOVED       = 101;
> > +const EVENT_CTRL_EVENT_SCAN_RESULTS      = 102;
> > +const EVENT_WPS_AP_AVAILABLE             = 103;
> > +const EVENT_AP_STA_CONNECTED             = 104;
> > +const EVENT_AP_STA_DISCONNECTED          = 105;
> 
> I think we need also the supplicant lost event.(In case of wpa_supplicant is
> terminated or crashed)
> 

It will be handled in WifiWorker.js.

> @@ +65,5 @@
> > +const WPS_METHOD_DISPLAY = "display";
> > +const WPS_METHOD_KEYPAD  = "keypad";
> > +
> > +// Role string.
> > +const P2P_ROLE_GO     = "GO";
> 
> s/P2P_ROLE_GO/P2P_ROLE_GROUP_OWNER
> 

What's wrong with the naming?

> @@ +533,5 @@
> > +      var retryCnt = 3;
> > +      var retryTimer;
> > +
> > +      function connectToSupplicantWithRetry(aCallback) {
> > +        aP2pCommand.connectToSupplicant(function(status) {
> 
> We should cancel the timer when exit() this state.
> 

I am using the one shot timer. Should we still need to cancel the timer?

> @@ +549,5 @@
> > +          }, 5000, Ci.nsITimer.TYPE_ONE_SHOT);
> > +        });
> > +      }
> > +
> > +      connectToSupplicantWithRetry(function(success) {
> 
> We should make sure wpa_supplicant is started before connect to it, and this
> seem a duplicate because we have done the same thing in wifi right ?
> Can we just receive a connect to supplicant event with result here ?
> 

Yes you are right. I doing this just because originally I put enableP2p()
in different timing but the design has been changed. Now enableP2p() will
be called after we've connected to supplicant....

I'll remove this later.

> @@ +575,5 @@
> > +        }
> > +
> > +        // Enable P2P.
> > +        aP2pCommand.p2pEnable(_localDevice.deviceName, function(success) {
> > +          if (!success) {
> 
> Is it possible that EVENT_P2P_PROV_DISC_PBC_REQ,
> EVENT_P2P_PROV_DISC_SHOW_PIN and EVENT_P2P_PROV_DISC_ENTER_PIN events are
> sent from wpa_supplicant before we move to stateInactive ?
> 
> @@ +588,5 @@
> > +            }
> > +            sm.sendEvent({ id: EVENT_P2P_ENABLE_SUCCESS });
> > +          });
> > +        });
> > +
> 
> If we receive EVENT_P2P_CMD_DISABLE event in this state and you are in the
> middle of async. calls. 
> How do you handle this case ? Do we need some internal states and events to
> handle these async. calls ?
> 
> @@ +1119,5 @@
> > +            debug('Supplicant connection closed');
> > +            aNetUtil.disableInterface(P2P_INTERFACE_NAME, function (success){
> > +              debug('Disabled interface: ' + P2P_INTERFACE_NAME);
> > +              sm.sendEvent({ id: EVENT_P2P_DISABLE_SUCCESS });
> > +            });
> 
> There might be state switch while we are in the middle of enter() function.
> We need to find a way to prevent this in others state, too.
> 

To solve all these async call issues, maybe I will introduce something like 
StateMachine::deferMessage in AOSP.

> @@ +1204,5 @@
> > +
> > +        debug('Happy p2p client~');
> > +        aCallback(true);
> > +      });
> > +    }
> 
> if (!_groupInfo.isGroupOwner) {
>     ......
>   return;
> }
> 
> do group owner stuff ....
> 
> @@ +1294,5 @@
> > +                            stateConnected,
> > +                            stateDisconnecting];
> > +
> > +    for (let i = 0; i < p2pManagedStates.length; i++) {
> > +      if (aState === p2pManagedStates[i]) {
> 
> Instead of comparing object, can we compare the name of the state ?
> 

Why should we compare the state's name rather than the object?
Comparing objects is just pointer comparison, supposed to be much faster than comparing strings.

> @@ +1303,5 @@
> > +    return false;
> > +  }
> > +
> > +  function initTimeoutTimer(aTimeoutMs, aTimeoutEvent) {
> > +    var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> Please check the life cycle of timer. Users of instances of nsITimer should
> keep a reference to the timer until it is no longer needed in order to
> assure the timer is fired.
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsITimer
> 

The timer returned by this function will be kept in the state object 
as a member variable. So don't worry about it :)


> ::: dom/wifi/WifiP2pWorkerObserver.jsm
> @@ +224,5 @@
> > +          aDomMsgResponder.setScanEnabled(true, function(success) {
> > +            if (success) {
> > +              returnMessage(aMessage.name, true, true, msg);
> > +            } else {
> > +              returnMessage(aMessage.name, false, "ERROR", msg);
> 
> Can we pass the success to returnMessage() so that we only need to do if()
> {} else {} things there once ?
> 

YES!!!

Comment 80

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part1: idl/webidl for WifiP2pManager (v2)

Comment 81

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part2: Implementation of webidl/idls (v2, r+'d)

Comment 82

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part4: p2p core implementation (v2)

Comment 83

[Henry Chang [:hchang]]

Comment on attachment 830079 [details] [diff] [review]
Bug 811635 - Part4: p2p core implementation (v2)

How I solved the async operation issue is to add 3 functions to the state machine: pause/resume/deferEvent. Besides, I extracted the state machine to StateMachine.jsm:

|StateMachine::sendEvent()| now becomes a async function, it simply pushes the event to an event queue and then does the event handling in the next time slot. I currently implement the async call by timer. (not sure if it's the only and the best way.)

|StateMachine::pause()| sets the flag |_paused|, which avoids |handleFirstEvent()| processing events until |StateMachine::resume()| gets called. When resume() is called, it unsets the flag |_paused| as well as asynchronously handles the next event.

|StateMachine::deferEvent()| is another important enhancement of the new state machine. It puts the event in another queue, which will be processed when 1) we enter a new state or 2) we receive a new regular event. The reason we want defer a event is: we couldn't handle the event in the current state. But we could handle it in another state or after receiving other events.
For example, if the "connecting" state, we cannot handle "disconnect" (but we don't want to ignore it) until we enter "connected" state. In this case, we can defer "disconnect" event when we are in the "connecting" state.

Comment 84

[Henry Chang [:hchang]]

Attachment: Part4: p2p core implementation (v3)

Comment 85

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 830077 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager (v2)

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

The patch looks good for me, Blake, we need your help to do superreview.

::: dom/webidl/WifiP2pManager.webidl
@@ +130,5 @@
> +  /**
> +   * An event listener that is called whenever the Wifi Direct peer list is
> +   * updated. Use getPeerList() to get the up-to-date peer list.
> +   */
> +  attribute EventHandler onpeerinfoupdate;

We have the getPeerList() API and onpeerinforupdate event handler here. Basically, both of them doing the same thing to obtain peer list. Not quite sure if we can skip one.

Comment 86

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 830693 [details] [diff] [review]
Part4: p2p core implementation (v3)

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

The patch looks good, r=me with comments addressed.
This is really a big patches. :-) To prevent any regression, I would like to see 
the tryserver test and gaia-ui test result before landed.

::: dom/wifi/StateMachine.jsm
@@ +18,5 @@
> +  function debug(aMsg) {
> +    if (DEBUG) {
> +      dump('-------------- StateMachine: ' + aMsg);
> +    }
> +  }

We can turn on/off debug message for wifi using settings. Can we use that to turn on/off the debug message for wifi direct, too ? You can do that in the follow-up.

@@ +110,5 @@
> +    // We are in the new state now. We got a chance to handle the
> +    // deferred events.
> +    handleDeferredEvents();
> +
> +    sm.resume();

Do we need to call resume() here ? Pause() and resume() should be a pair, right ? Calling resume() implicitly call asynCall() which schedules a task for itself. Maybe we can avoid resume in gotoState() to prevent any performance impact ?

@@ +144,5 @@
> +      return;
> +    }
> +
> +    if (_paused) {
> +      debug('PAUSED...............');

Nit: can we have a better debug message here ? Or you can remove it.

@@ +154,5 @@
> +    handleOneEvent(_eventQueue.shift()); // The handler may defer this event.
> +
> +    // We've handled one event. If we had deferred events before, now is
> +    // a good chance to handle them.
> +    if (hadDeferredEvents) {

Nit: if (_deferredEventQueue.length > 0) is good enough here. So we don't need to declare a variable.

::: dom/wifi/WifiP2pManager.jsm
@@ +575,5 @@
> +        _sm.gotoState(stateInactive);
> +      }
> +
> +      _sm.pause();
> +

I don't see the _sm.resume() be called below, shouldn't we call it to resume the state machine here ? I am worried about that we might forget to call pause/resume pair, can we refactor that to make sure these two could be called ? Maybe we can do it in the follow-up ?

::: dom/wifi/WifiWorker.js
@@ +117,5 @@
>      onWaitEvent: function(event, iface) {
>        if (manager.ifname === iface && handleEvent(event)) {
>          waitForEvent(iface);
>        }
> +      else if (p2pSupported) {

Nit: } else if (p2pSupported) {

@@ +125,5 @@
> +          // rather than blocking. So when we see this special event string,
> +          // just return immediately.
> +          const TERMINATED_EVENT = 'CTRL-EVENT-TERMINATING  - connection closed';
> +          if (-1 !== event.indexOf(TERMINATED_EVENT)) {
> +            return;

This event indicates that wpa_supplicant is terminated somehow. We may need to pass this event to p2p state machine.

@@ +168,5 @@
> +  var p2pManager;
> +  if (p2pSupported) {
> +    let p2pCommand = WifiCommand(controlMessage, WifiP2pManager.INTERFACE_NAME);
> +    p2pManager = WifiP2pManager(p2pCommand, netUtil);
> +  }

I think we should move Wifi P2P stuff code snippet before we start wifi service.

Comment 87

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 830077 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager (v2)

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

::: dom/webidl/WifiP2pManager.webidl
@@ +61,5 @@
> +   *            invalid peer address, unsupported wps method or any
> +   *            preliminary error.
> +   *
> +   **/
> +  DOMRequest connect(optional ConnectionConfig config);

This still bothers me. It isn't legal to pass zero arguments to this function. I'd rather pull the required arguments out of the dictionary and into the IDL to be more clear about what's required.

@@ +102,5 @@
> +   * onsuccess: Command succeeded.
> +   * onerror:   Command failed.
> +   *
> +   */
> +  DOMRequest setPairingConfirmation(optional ConfirmationResult result);

Here too.

Comment 88

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part1: idl/webidl for WifiP2pManager (v3)

Comment 89

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part2: Implementation of webidl/idls (v3, r+'d)

Comment 90

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part3: Adding system message "wifip2p-pairing-request" (v2, rebased, r+'d)

Comment 91

[Henry Chang [:hchang]]

Attachment: Bug811635 - Part4: p2p core implementation (v4) r+'d

Comment 92

[Henry Chang [:hchang]]

Comment on attachment 8339809 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager (v3)

This patch takes out the arguments from dictionaries used in connect and setPairingConfirmation in WifiP2pManager.webidl.

Comment 93

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8339809 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager (v3)

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

Thanks!

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0338e84d99a7
https://hg.mozilla.org/integration/b2g-inbound/rev/6c9ca70f582d
https://hg.mozilla.org/integration/b2g-inbound/rev/b695cdc91edc
https://hg.mozilla.org/integration/b2g-inbound/rev/4acc0f028076
https://hg.mozilla.org/integration/b2g-inbound/rev/ad72f0b9e16a

Comment 95

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-8 failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/4e8b08358660

https://tbpl.mozilla.org/php/getParsedLog.php?id=31380731&tree=B2g-Inbound

Comment 96

[Henry Chang [:hchang]]

Attachment: Bug811635 - Part6: DOM API Test

Add newly exposed DOM API to dom/tests/mochitest/general/test_interfaces.html

Comment 97

[Henry Chang [:hchang]]

Attachment: Bug 811635 - Part1: idl/webidl for WifiP2pManager (v4, r+'d)

Normalize DOM interface names to starting with 'Moz'

Comment 98

[Henry Chang [:hchang]]

Attachment: Part2: Implementation of webidl/idls (v4, r+'d)

Normalize DOM interface names to starting with 'Moz'

Comment 99

[Henry Chang [:hchang]]

Attachment: Part2: Implementation of webidl/idls (v4, r+'d)

Comment 100

[Henry Chang [:hchang]]

Attachment: Part1: idl/webidl for WifiP2pManager (v5, r+'ed)

Comment 101

[Henry Chang [:hchang]]

Attachment: Part2: Implementation of webidl/idls (v5, r+'ed, rebased)

Comment 102

[Henry Chang [:hchang]]

Attachment: Part3: Adding system message "wifip2p-pairing-request" (v3, rebased, r+'ed)

Comment 103

[Henry Chang [:hchang]]

Attachment: Part4: p2p core implementation (v5) r+'ed, rebased

Comment 104

[Henry Chang [:hchang]]

Attachment: Part5: Starting wpa_supplicant with proper p2pSupported flag (r+'ed)

Comment 105

[Henry Chang [:hchang]]

Attachment: Part6: DOM API Test

Comment 106

[Henry Chang [:hchang]]

Try result of the latest patches:

https://tbpl.mozilla.org/?tree=Try&rev=cbbb6e52a83f

Comment 107

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8349901 [details] [diff] [review]
Part6: DOM API Test

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

Looks good. Thank you.

Comment 108

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Vincent Chang[:vchang] from comment #107)
> Comment on attachment 8349901 [details] [diff] [review]
> Part6: DOM API Test
> 
> Review of attachment 8349901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Thank you.

Henry, Vincent, this is unacceptable. The file explicitly states that only DOM peers can review changes to it.

Comment 109

[Vincent Chang[:vchang][changyihsin]]

Hi Ms2ger, sorry about that.

Comment 110

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8349901 [details] [diff] [review]
Part6: DOM API Test

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

Hi Blake, Happy New Year. 
Could you please help us to review this ?

Comment 111

[Henry Chang [:hchang]]

Ms2ger, sorry about that, too.
Vincent, thanks for asking for relevant people to review this :)

(In reply to :Ms2ger from comment #108)
> (In reply to Vincent Chang[:vchang] from comment #107)
> > Comment on attachment 8349901 [details] [diff] [review]
> > Part6: DOM API Test
> > 
> > Review of attachment 8349901 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good. Thank you.
> 
> Henry, Vincent, this is unacceptable. The file explicitly states that only
> DOM peers can review changes to it.

Comment 112

[Henry Chang [:hchang]]

Let (In reply to Vincent Chang[:vchang] from comment #110)
> Comment on attachment 8349901 [details] [diff] [review]
> Part6: DOM API Test
> 
> Review of attachment 8349901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Blake, Happy New Year. 
> Could you please help us to review this ?

This patch adds the exposed API to test_interfaces.html, which tests all interfaces supposed and not supposed to be seen on normal and xbl scope. Blake, do you need more information regarding this patch? Thanks :)

Comment 113

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8349901 [details] [diff] [review]
Part6: DOM API Test

Sorry for the delay. I only just got back from my New Years vacation.

Comment 114

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #113)
> Comment on attachment 8349901 [details] [diff] [review]
> Part6: DOM API Test
> 
> Sorry for the delay. I only just got back from my New Years vacation.

It's totally fine! Thanks for your review :)

Comment 115

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #114)
> (In reply to Blake Kaplan (:mrbkap) from comment #113)
> > Comment on attachment 8349901 [details] [diff] [review]
> > Part6: DOM API Test
> > 
> > Sorry for the delay. I only just got back from my New Years vacation.
> 
> It's totally fine! Thanks for your review :)

Hi Ms2ger, what do you think about this patch based on Blake's review?
If you also feel okay, I hope to be able to flag checkin-needed tonight. Thanks :)

Comment 116

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8349901 [details] [diff] [review]
Part6: DOM API Test

If Blake reviewed it, that's good then, I guess. Please make sure to update the commit message.

Comment 117

[Henry Chang [:hchang]]

Attachment: Part3: Adding system message "wifip2p-pairing-request" (v4, rebased, r+'ed)

Comment 118

[Henry Chang [:hchang]]

Attachment: Part4: p2p core implementation (v6) r+'ed, rebased

Rebased again

Comment 119

[Henry Chang [:hchang]]

Attachment: Part6: DOM API Test (r+'d updated commit message)

Updated commit message

Comment 120

[Henry Chang [:hchang]]

https://tbpl.mozilla.org/?tree=Try&rev=137154539474

Comment 121

[Henry Chang [:hchang]]

(In reply to :Ms2ger from comment #116)
> Comment on attachment 8349901 [details] [diff] [review]
> Part6: DOM API Test
> 
> If Blake reviewed it, that's good then, I guess. Please make sure to update
> the commit message.

Thanks Ms2ger! I've already updated the commit message.

Comment 122

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/89b90ccdf603
https://hg.mozilla.org/integration/b2g-inbound/rev/687a84bf0785
https://hg.mozilla.org/integration/b2g-inbound/rev/61694318819a
https://hg.mozilla.org/integration/b2g-inbound/rev/96772cc4e346
https://hg.mozilla.org/integration/b2g-inbound/rev/3f7ee595646d
https://hg.mozilla.org/integration/b2g-inbound/rev/2da820ed2587

Do we have any tests for this or a bug on file to add them?

Comment 123

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_interface.html failures that only occur on debug B2G mochitest runs :(
https://hg.mozilla.org/integration/b2g-inbound/rev/2ae84eaa1866

https://tbpl.mozilla.org/php/getParsedLog.php?id=33044177&tree=B2g-Inbound

Comment 124

[Henry Chang [:hchang]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #123)
> Backed out for test_interface.html failures that only occur on debug B2G
> mochitest runs :(
> https://hg.mozilla.org/integration/b2g-inbound/rev/2ae84eaa1866
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33044177&tree=B2g-Inbound

Oops sorry for this. I was aware of that but I thought it's okay since
mozilla-central didn't run that :(

Comment 125

[Henry Chang [:hchang]]

Attachment: Part6: DOM API Test (added permission control)

Comment 126

[Henry Chang [:hchang]]

Comment on attachment 8365761 [details] [diff] [review]
Part6: DOM API Test (added permission control)

There is a bug regarding "No wifi-manage permission on B2G ICS debug build" (Bug 961639). If the DOM API we expose requires 'wifi-manage' permission, test_interfaces.html could't pass both opt and debug build. I added the permission to the visibility map. If window.document has no permission specified in interfaceNamesInGlobalScope[], the interface shouldn't be visible.

https://tbpl.mozilla.org/?tree=Try&rev=f6e87b7e0ecc

Comment 127

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8365761 [details] [diff] [review]
Part6: DOM API Test (added permission control)

Redirecting this to bz, since I'm not really sure about the code change to the test.

Comment 128

[Henry Chang [:hchang]]

(In reply to Blake Kaplan (:mrbkap) from comment #127)
> Comment on attachment 8365761 [details] [diff] [review]
> Part6: DOM API Test (added permission control)
> 
> Redirecting this to bz, since I'm not really sure about the code change to
> the test.

Thanks Blake :) 

bz: could you please help review my patch regarding determining the visibility of DOM APIs based on permission as well?
Thanks!

Comment 129

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8365761 [details] [diff] [review]
Part6: DOM API Test (added permission control)

I'm not sure I follow this patch.  "MozWifiP2pGroupOwner" is unconditional in the earlier webidl patch, no?  And "MozWifiP2pManager" is conditional there...

Also, I'd think all three of these interfaces should be present only if the permission is set?

Comment 130

[Henry Chang [:hchang]]

(In reply to Boris Zbarsky [:bz] from comment #129)
> Comment on attachment 8365761 [details] [diff] [review]
> Part6: DOM API Test (added permission control)
> 
> I'm not sure I follow this patch.  "MozWifiP2pGroupOwner" is unconditional
> in the earlier webidl patch, no?  And "MozWifiP2pManager" is conditional
> there...
> 
> Also, I'd think all three of these interfaces should be present only if the
> permission is set?

Oops it should be "{name: "MozWifiP2pManager", b2g: true, permission: "wifi-manage"},"
Sorry it's my fault.

Meaningful MozWifiP2pGroupOwner and MozWifiP2pStatusChangeEvent could be obtained only when you have MozWifiP2pManager. Do you think they still require the permission?

Thanks :)

Comment 131

[Boris Zbarsky [:bzbarsky]]

> Do you think they still require the permission?

Yes!  The whole point of the test is to check that you're not sticking unwanted properties on the window object.  In particular, with the patch as-is window.MozWifiP2pGroupOwner and window.MozWifiP2pStatusChangeEvent exist on all webpages.  They shouldn't.

Comment 132

[Henry Chang [:hchang]]

Attachment: Part1: idl/webidl for WifiP2pManager (v6, r+'ed, added more permission control)

Comment 133

[Henry Chang [:hchang]]

Attachment: Part2: Implementation of webidl/idls (v6, r+'ed, rebased again)

Comment 134

[Henry Chang [:hchang]]

Attachment: Part5: Starting wpa_supplicant with proper p2pSupported flag (r+'ed, rebased)

Comment 135

[Henry Chang [:hchang]]

Attachment: Part6: DOM API Test (added permission control)

Comment 136

[Henry Chang [:hchang]]

Comment on attachment 8366503 [details] [diff] [review]
Part6: DOM API Test (added permission control)

Add permission constraint to MozWifiP2pManager/MozWifiP2pGroupOwner/MozWifiP2pStatusChangeEvent. Patch for webidls is also updated to have conditional constructor.

https://tbpl.mozilla.org/?tree=Try&rev=afab296c6177

Comment 137

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #136)
> Comment on attachment 8366503 [details] [diff] [review]
> Part6: DOM API Test (added permission control)
> 
> Add permission constraint to
> MozWifiP2pManager/MozWifiP2pGroupOwner/MozWifiP2pStatusChangeEvent. Patch
> for webidls is also updated to have conditional constructor.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=afab296c6177

Hi bz, 

I just attached a new patch for review and obsoleted the old one. Thanks!

Comment 138

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8366503 [details] [diff] [review]
Part6: DOM API Test (added permission control)

Much better, thank you!

Is this test ever run in an environment where the permission is set?  That is, would it still pass if these lines were simply not added?

Comment 139

[Henry Chang [:hchang]]

(In reply to Boris Zbarsky [:bz] from comment #138)
> Comment on attachment 8366503 [details] [diff] [review]
> Part6: DOM API Test (added permission control)
> 
> Much better, thank you!
> 
> Is this test ever run in an environment where the permission is set?  That
> is, would it still pass if these lines were simply not added?

Actually there's a story behind this patch :p

In the very beginning, the test only ran B2G ICS opt build where the permission is set, so I have to add these interfaces to the list.

After a while, the test started to run B2G ICS "debug" build where the permission is NOT set (for some reason), so the test would pass for opt build but fail for debug build. At this moment, I was aware that other than finding out why the permission is not set, we might also need to consider the permission just like how we deal with preferences.

The interesting thing is, the debug build test is running with "wifi-manage" permission now. That is, even not adding the permission check, we can still get pass for both opt and debug build. But I think the permission check is still worthy so that the test would not require the permission to be set or not.

Thanks!

Comment 140

[Boris Zbarsky [:bzbarsky]]

Makes sense, thanks!  Would be nice if we actually had it running in both environments...  Maybe a followup bug on that?

Comment 141

[Henry Chang [:hchang]]

(In reply to Boris Zbarsky [:bz] from comment #140)
> Makes sense, thanks!  Would be nice if we actually had it running in both
> environments...  Maybe a followup bug on that?

Well.. did you mean a test case for verifying the logic I added? I am glad to file one! It may be doing:

1. Using SpecialPowers.pushPermission to remove "wifi-manage" before running this test.
2. Re-run this test

And the test should still pass

(It somehow looks like a test case for test case :p)

Anyway, thanks!

Comment 142

[Boris Zbarsky [:bzbarsky]]

> (It somehow looks like a test case for test case :p)

Somewhat, yes.  ;)

Comment 143

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/020c2d3fca92
https://hg.mozilla.org/integration/b2g-inbound/rev/19de0a02e246
https://hg.mozilla.org/integration/b2g-inbound/rev/ea922926949b
https://hg.mozilla.org/integration/b2g-inbound/rev/92418aa51582
https://hg.mozilla.org/integration/b2g-inbound/rev/df06d3fc2540
https://hg.mozilla.org/integration/b2g-inbound/rev/103b26ebd28a

Comment 144

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/020c2d3fca92
https://hg.mozilla.org/mozilla-central/rev/19de0a02e246
https://hg.mozilla.org/mozilla-central/rev/ea922926949b
https://hg.mozilla.org/mozilla-central/rev/92418aa51582
https://hg.mozilla.org/mozilla-central/rev/df06d3fc2540
https://hg.mozilla.org/mozilla-central/rev/103b26ebd28a