Closed Bug 811635 Opened 7 years ago Closed 6 years ago

B2G Wifi: Support Wifi Direct

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: chucklee, Assigned: hchang)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [ETA: 8/16])

Attachments

(10 files, 64 obsolete files)

198.67 KB, image/png
Details
285 bytes, text/html
Details
221 bytes, text/html
Details
4.98 KB, patch
Details | Diff | Splinter Review
1005 bytes, patch
Details | Diff | Splinter Review
95.64 KB, patch
Details | Diff | Splinter Review
7.45 KB, patch
Details | Diff | Splinter Review
14.96 KB, patch
Details | Diff | Splinter Review
2.75 KB, patch
Details | Diff | Splinter Review
3.23 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Provide DOM API and related implementation for Wifi Direct.
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.
Attached file Wifi Direct API v0.1 (obsolete) —
Draft of Wifi Direct API
Attached file Wifi Direct API v0.2 (obsolete) —
1. Create wifiDirectManager for wifi direct API.
2. Refine function names.
Attachment #681422 - Attachment is obsolete: true
Attached file Wifi Direct API v0.3 (obsolete) —
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.
Attachment #683871 - Attachment is obsolete: true
Attached patch Gecko WIP : 0001.IDL (obsolete) — Splinter Review
Attachment #686983 - Attachment is obsolete: true
It seems to be necessary to use different state handler for wifi/wifi direct.
Needs refactory.
Add API to control dhcp server.
It's better provide a service for controlling dhcp server/client, and provide dhcp status and data.
Not really affect system now because it is not included into image.
Chuck, what's the target milestone for this bug and why are we working on it now? Is this for a partner or something?
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.
Blocks: 844712
Whiteboard: [ETA: 8/16]
Attachment #688125 - Attachment is obsolete: true
Attachment #784740 - Attachment description: 0001.DOM_IDL.patch (rebased) → Gecko WIP: 0001.DOM_IDL.patch (rebased)
Attachment #784741 - Attachment description: Gecko WIP: 0002.DOM_Implementation → Gecko WIP: 0002.DOM_Implementation (rebased)
Attachment #784744 - Attachment description: Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker → Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker (rebased)
Attachment #784747 - Attachment description: Gaia Test App: Bug811635-0001.Addconfig (rebased) → Gaia Test App WIP: 0001.Addconfig (rebased)
Attachment #784748 - Attachment description: Gaia Test App: Bug811635-0002.AddwifidirectUIentryinsettings.patch (rebased) → Gaia Test App: 0002.AddwifidirectUIentryinsettings.patch (rebased)
Attachment #784748 - Attachment description: Gaia Test App: 0002.AddwifidirectUIentryinsettings.patch (rebased) → Gaia Test App WIP: 0002.AddwifidirectUIentryinsettings (rebased)
Attachment #688126 - Attachment is obsolete: true
Attachment #688127 - Attachment is obsolete: true
Attachment #688128 - Attachment is obsolete: true
Attachment #688129 - Attachment is obsolete: true
Attachment #688133 - Attachment is obsolete: true
Attachment #688134 - Attachment is obsolete: true
Attachment #688135 - Attachment is obsolete: true
Attachment #688137 - Attachment is obsolete: true
I've attached all the rebased and conflict-resolved patches for Gecko and Gaia. The hardcoded PIN is 11111111.
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.
Attachment #688138 - Flags: review-
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.
Attachment #688139 - Flags: review-
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.
Attachment #784745 - Flags: review-
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 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.
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
Attachment #784747 - Attachment is obsolete: true
Attachment #784748 - Attachment is obsolete: true
Attachment #784751 - Attachment is obsolete: true
Attachment #784752 - Attachment is obsolete: true
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.
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.
Attachment #787923 - Attachment description: Gecko-0001 Adding DOMWifiDirectManager.idl → Gecko-0001: Adding DOMWifiDirectManager.idl
Henri, great doc in the idl! Can we just use webidl now and not have to convert later?
Attachment #688138 - Attachment is obsolete: true
Attachment #688139 - Attachment is obsolete: true
Attachment #784740 - Attachment is obsolete: true
Attachment #784741 - Attachment is obsolete: true
Attachment #784742 - Attachment is obsolete: true
Attachment #784745 - Attachment is obsolete: true
Attachment #784744 - Attachment is obsolete: true
Removed dependencies on system/core/libnetutil and fixed some wifi regression
Hi Fabrice: Sure! But we are still revising the web APIs. We will in the end use webidl rather than idl. Thanks.
Attachment #787923 - Attachment is obsolete: true
Attachment #787924 - Attachment is obsolete: true
Attachment #787925 - Attachment is obsolete: true
Attachment #787927 - Attachment is obsolete: true
Attached patch 0003_Wifi_backend_support.patch (obsolete) — Splinter Review
Attachment #787926 - Attachment is obsolete: true
Attached patch 0005_Build.patch (obsolete) — Splinter Review
Attachment #786875 - Attachment is obsolete: true
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
Depends on: 912015
Depends on: 919426
Assignee: chulee → hchang
Attachment #790647 - Attachment is obsolete: true
Attachment #790648 - Attachment is obsolete: true
Attachment #790650 - Attachment is obsolete: true
Attachment #790651 - Attachment is obsolete: true
Attachment #790652 - Attachment is obsolete: true
Attachment #790653 - Attachment is obsolete: true
Attachment #790659 - Attachment is obsolete: true
Attachment #818217 - Attachment filename: Bug811635_Part1_idl.patch → idls for WifiP2pManager
Attachment #818217 - Attachment description: Bug811635_Part1_idl.patch → Bug 811635 - Part1: idl/webidl for WifiP2pManager
Attachment #818217 - Attachment filename: idls for WifiP2pManager → Bug811635_Part1_idl.patch
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.
Attachment #818291 - Attachment mime type: text/plain → text/html
Attachment #818217 - Flags: review?(vchang)
Attachment #818220 - Flags: review?(vchang)
Attachment #818221 - Flags: review?(vchang)
Attachment #818222 - Flags: review?(vchang)
Attachment #818224 - Flags: review?(vchang)
Added the support of p2p invitation and persistent group
Attachment #818222 - Attachment is obsolete: true
Attachment #818222 - Flags: review?(vchang)
Attachment #818940 - Attachment mime type: text/plain → text/html
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 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.
Attachment #818220 - Flags: review?(vchang) → review+
Attachment #818221 - Flags: review?(vchang) → review+
Attachment #818939 - Flags: review?(vchang)
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 ?
Attachment #818224 - Flags: review?(vchang)
(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!
(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 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.
Attachment #818217 - Flags: review?(vchang)
Attachment #818939 - Attachment is obsolete: true
Attachment #818939 - Flags: review?(vchang)
(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.
Attachment #823867 - Flags: review?(vchang)
Attachment #823866 - Flags: review?(vchang)
Attachment #818217 - Flags: superreview?(mrbkap)
(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 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.
Attachment #823867 - Flags: review?(vchang) → review+
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 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.
Attachment #818217 - Flags: superreview?(mrbkap)
(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);
                                                       ^
Patch for testing p2p on hamachi or legacy devices
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.
Attachment #823866 - Flags: review?(vchang)
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!!!
Attachment #818217 - Attachment is obsolete: true
Attachment #818220 - Attachment is obsolete: true
Attachment #823866 - Attachment is obsolete: true
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.
Attachment #830079 - Flags: review?(vchang)
Attachment #830077 - Flags: review?(vchang)
Attachment #830079 - Attachment is obsolete: true
Attachment #830079 - Flags: review?(vchang)
Attachment #830693 - Flags: review?(vchang)
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.
Attachment #830077 - Flags: superreview?(mrbkap)
Attachment #830077 - Flags: review?(vchang)
Attachment #830077 - Flags: review+
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.
Attachment #830693 - Flags: review?(vchang) → review+
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.
Attachment #830077 - Flags: superreview?(mrbkap) → superreview-
Attachment #830077 - Attachment is obsolete: true
Attachment #830693 - Attachment is obsolete: true
Attachment #830078 - Attachment is obsolete: true
Attachment #827195 - Attachment description: Patch for hamachi p2p support (WIP) → Patch for hamachi p2p support (NOT for check-in)
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.
Attachment #8339809 - Flags: superreview?(mrbkap)
Comment on attachment 8339809 [details] [diff] [review]
Bug 811635 - Part1: idl/webidl for WifiP2pManager (v3)

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

Thanks!
Attachment #8339809 - Flags: superreview?(mrbkap) → superreview+
Keywords: checkin-needed
Attached patch Bug811635 - Part6: DOM API Test (obsolete) — Splinter Review
Add newly exposed DOM API to dom/tests/mochitest/general/test_interfaces.html
Normalize DOM interface names to starting with 'Moz'
Attachment #8339809 - Attachment is obsolete: true
Normalize DOM interface names to starting with 'Moz'
Attachment #8339810 - Attachment is obsolete: true
Attachment #8342247 - Attachment is obsolete: true
Depends on: 946815
Blocks: 925615
Attachment #8342244 - Attachment is obsolete: true
Attachment #8349895 - Attachment description: Bug 811635 - Part1: idl/webidl for WifiP2pManager (v5, r+'ed) → Part1: idl/webidl for WifiP2pManager (v5, r+'ed)
Attachment #8342258 - Attachment is obsolete: true
Attachment #8339813 - Attachment is obsolete: true
Attachment #8349900 - Attachment description: Part5: Starting wpa_supplicant with proper p2pSupported flag → Part5: Starting wpa_supplicant with proper p2pSupported flag (r+'ed)
Attached patch Part6: DOM API Test (obsolete) — Splinter Review
Attachment #8342241 - Attachment is obsolete: true
Attachment #8349901 - Flags: review?(vchang)
Try result of the latest patches:

https://tbpl.mozilla.org/?tree=Try&rev=cbbb6e52a83f
Comment on attachment 8349901 [details] [diff] [review]
Part6: DOM API Test

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

Looks good. Thank you.
Attachment #8349901 - Flags: review?(vchang) → review+
Keywords: checkin-needed
(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.
Keywords: checkin-needed
Hi Ms2ger, sorry about that.
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 ?
Attachment #8349901 - Flags: review?(mrbkap)
Attachment #8349901 - Flags: review?(Ms2ger)
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.
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 :)
Flags: needinfo?(mrbkap)
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.
Attachment #8349901 - Flags: review?(mrbkap) → review+
Flags: needinfo?(mrbkap)
(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 :)
(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 :)
Flags: needinfo?(Ms2ger)
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.
Attachment #8349901 - Flags: review?(Ms2ger)
Flags: needinfo?(Ms2ger)
Rebased again
Attachment #8349899 - Attachment is obsolete: true
Updated commit message
Attachment #8349901 - Attachment is obsolete: true
Attachment #8359710 - Attachment description: Part6: DOM API Test → Part6: DOM API Test (r+'d updated commit message)
(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.
Keywords: checkin-needed
(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 :(
Depends on: 961639
Attachment #8359710 - Attachment is obsolete: true
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
Attachment #8365761 - Flags: review?(mrbkap)
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.
Attachment #8365761 - Flags: review?(mrbkap) → review?(bzbarsky)
(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 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?
(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 :)
> 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 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
Attachment #8366503 - Flags: review?(bzbarsky)
Attachment #8365761 - Attachment is obsolete: true
Attachment #8365761 - Flags: review?(bzbarsky)
(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 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?
Attachment #8366503 - Flags: review?(bzbarsky) → review+
(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!
Makes sense, thanks!  Would be nice if we actually had it running in both environments...  Maybe a followup bug on that?
(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!
Keywords: checkin-needed
> (It somehow looks like a test case for test case :p)

Somewhat, yes.  ;)
You need to log in before you can comment on or make changes to this bug.