Closed
Bug 811635
Opened 12 years ago
Closed 11 years ago
B2G Wifi: Support Wifi Direct
Categories
(Core :: DOM: Device Interfaces, defect)
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)
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Provide DOM API and related implementation for Wifi Direct.
Reporter | ||
Comment 1•12 years ago
|
||
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.
Reporter | ||
Comment 3•12 years ago
|
||
1. Create wifiDirectManager for wifi direct API. 2. Refine function names.
Attachment #681422 -
Attachment is obsolete: true
Reporter | ||
Comment 4•12 years ago
|
||
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
Reporter | ||
Comment 6•12 years ago
|
||
Reporter | ||
Comment 7•12 years ago
|
||
Reporter | ||
Comment 8•12 years ago
|
||
It seems to be necessary to use different state handler for wifi/wifi direct. Needs refactory.
Reporter | ||
Comment 9•12 years ago
|
||
Add API to control dhcp server. It's better provide a service for controlling dhcp server/client, and provide dhcp status and data.
Reporter | ||
Comment 10•12 years ago
|
||
Reporter | ||
Comment 11•12 years ago
|
||
Reporter | ||
Comment 12•12 years ago
|
||
Reporter | ||
Comment 13•12 years ago
|
||
Reporter | ||
Comment 14•12 years ago
|
||
Reporter | ||
Comment 15•12 years ago
|
||
Not really affect system now because it is not included into image.
Comment 16•12 years ago
|
||
Chuck, what's the target milestone for this bug and why are we working on it now? Is this for a partner or something?
Reporter | ||
Comment 17•12 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [ETA: 8/16]
Assignee | ||
Updated•11 years ago
|
Attachment #784740 -
Attachment description: 0001.DOM_IDL.patch (rebased) → Gecko WIP: 0001.DOM_IDL.patch (rebased)
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #784741 -
Attachment description: Gecko WIP: 0002.DOM_Implementation → Gecko WIP: 0002.DOM_Implementation (rebased)
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #784744 -
Attachment description: Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker → Gecko WIP: 0004.Support_Wifi_Direct_in_WifiWorker (rebased)
Assignee | ||
Comment 23•11 years ago
|
||
Assignee | ||
Comment 24•11 years ago
|
||
Assignee | ||
Comment 25•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #784747 -
Attachment description: Gaia Test App: Bug811635-0001.Addconfig (rebased) → Gaia Test App WIP: 0001.Addconfig (rebased)
Assignee | ||
Updated•11 years ago
|
Attachment #784748 -
Attachment description: Gaia Test App: Bug811635-0002.AddwifidirectUIentryinsettings.patch (rebased) → Gaia Test App: 0002.AddwifidirectUIentryinsettings.patch (rebased)
Assignee | ||
Updated•11 years ago
|
Attachment #784748 -
Attachment description: Gaia Test App: 0002.AddwifidirectUIentryinsettings.patch (rebased) → Gaia Test App WIP: 0002.AddwifidirectUIentryinsettings (rebased)
Assignee | ||
Comment 26•11 years ago
|
||
Assignee | ||
Comment 27•11 years ago
|
||
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
Assignee | ||
Comment 28•11 years ago
|
||
I've attached all the rebased and conflict-resolved patches for Gecko and Gaia. The hardcoded PIN is 11111111.
Reporter | ||
Comment 29•11 years ago
|
||
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-
Reporter | ||
Comment 30•11 years ago
|
||
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-
Reporter | ||
Comment 31•11 years ago
|
||
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-
Reporter | ||
Comment 32•11 years ago
|
||
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.
Reporter | ||
Comment 33•11 years ago
|
||
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.
Reporter | ||
Comment 34•11 years ago
|
||
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
Reporter | ||
Comment 35•11 years ago
|
||
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.
Reporter | ||
Comment 36•11 years ago
|
||
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.
Assignee | ||
Comment 37•11 years ago
|
||
Assignee | ||
Comment 38•11 years ago
|
||
Assignee | ||
Comment 39•11 years ago
|
||
Assignee | ||
Comment 40•11 years ago
|
||
Assignee | ||
Comment 41•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #787923 -
Attachment description: Gecko-0001 Adding DOMWifiDirectManager.idl → Gecko-0001: Adding DOMWifiDirectManager.idl
Comment 42•11 years ago
|
||
Henri, great doc in the idl! Can we just use webidl now and not have to convert later?
Assignee | ||
Updated•11 years ago
|
Attachment #688138 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #688139 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #784740 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #784741 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #784742 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #784745 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #784744 -
Attachment is obsolete: true
Assignee | ||
Comment 43•11 years ago
|
||
Removed dependencies on system/core/libnetutil and fixed some wifi regression
Assignee | ||
Comment 44•11 years ago
|
||
Hi Fabrice: Sure! But we are still revising the web APIs. We will in the end use webidl rather than idl. Thanks.
Assignee | ||
Comment 45•11 years ago
|
||
Attachment #787923 -
Attachment is obsolete: true
Attachment #787924 -
Attachment is obsolete: true
Attachment #787925 -
Attachment is obsolete: true
Attachment #787927 -
Attachment is obsolete: true
Assignee | ||
Comment 46•11 years ago
|
||
Assignee | ||
Comment 47•11 years ago
|
||
Assignee | ||
Comment 49•11 years ago
|
||
Assignee | ||
Comment 50•11 years ago
|
||
Assignee | ||
Comment 52•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Assignee: chulee → hchang
Assignee | ||
Comment 53•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #818217 -
Attachment filename: Bug811635_Part1_idl.patch → idls for WifiP2pManager
Assignee | ||
Updated•11 years ago
|
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
Assignee | ||
Comment 54•11 years ago
|
||
Assignee | ||
Comment 55•11 years ago
|
||
Assignee | ||
Comment 56•11 years ago
|
||
Assignee | ||
Comment 57•11 years ago
|
||
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.
Assignee | ||
Comment 58•11 years ago
|
||
Assignee | ||
Comment 59•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #818291 -
Attachment mime type: text/plain → text/html
Assignee | ||
Updated•11 years ago
|
Attachment #818217 -
Flags: review?(vchang)
Assignee | ||
Updated•11 years ago
|
Attachment #818220 -
Flags: review?(vchang)
Assignee | ||
Updated•11 years ago
|
Attachment #818221 -
Flags: review?(vchang)
Assignee | ||
Updated•11 years ago
|
Attachment #818222 -
Flags: review?(vchang)
Assignee | ||
Updated•11 years ago
|
Attachment #818224 -
Flags: review?(vchang)
Assignee | ||
Comment 60•11 years ago
|
||
Added the support of p2p invitation and persistent group
Attachment #818222 -
Attachment is obsolete: true
Attachment #818222 -
Flags: review?(vchang)
Assignee | ||
Comment 61•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #818940 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 62•11 years ago
|
||
See for clearer diff on bitbucket: https://bitbucket.org/changhenry/mozilla-central/compare/bug%2F811635-wifip2p..default#diff
Comment 63•11 years ago
|
||
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•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #818220 -
Flags: review?(vchang) → review+
Updated•11 years ago
|
Attachment #818221 -
Flags: review?(vchang) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #818939 -
Flags: review?(vchang)
Comment 65•11 years ago
|
||
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)
Assignee | ||
Comment 66•11 years ago
|
||
(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!
Assignee | ||
Comment 67•11 years ago
|
||
(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•11 years ago
|
||
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)
Assignee | ||
Comment 69•11 years ago
|
||
Attachment #818939 -
Attachment is obsolete: true
Attachment #818939 -
Flags: review?(vchang)
Assignee | ||
Comment 71•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #823867 -
Flags: review?(vchang)
Assignee | ||
Updated•11 years ago
|
Attachment #823866 -
Flags: review?(vchang)
Assignee | ||
Updated•11 years ago
|
Attachment #818217 -
Flags: superreview?(mrbkap)
Assignee | ||
Comment 72•11 years ago
|
||
(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•11 years ago
|
||
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+
Assignee | ||
Comment 74•11 years ago
|
||
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•11 years ago
|
||
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)
Assignee | ||
Comment 76•11 years ago
|
||
(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 78•11 years ago
|
||
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)
Assignee | ||
Comment 79•11 years ago
|
||
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!!!
Assignee | ||
Comment 81•11 years ago
|
||
Assignee | ||
Comment 82•11 years ago
|
||
Attachment #818220 -
Attachment is obsolete: true
Attachment #823866 -
Attachment is obsolete: true
Assignee | ||
Comment 83•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #830077 -
Flags: review?(vchang)
Assignee | ||
Comment 84•11 years ago
|
||
Attachment #830079 -
Attachment is obsolete: true
Attachment #830079 -
Flags: review?(vchang)
Attachment #830693 -
Flags: review?(vchang)
Comment 85•11 years ago
|
||
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 86•11 years ago
|
||
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 87•11 years ago
|
||
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-
Assignee | ||
Comment 89•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #830078 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #827195 -
Attachment description: Patch for hamachi p2p support (WIP) → Patch for hamachi p2p support (NOT for check-in)
Assignee | ||
Comment 92•11 years ago
|
||
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 93•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 94•11 years ago
|
||
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
Keywords: checkin-needed
Comment 95•11 years ago
|
||
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
Assignee | ||
Comment 96•11 years ago
|
||
Add newly exposed DOM API to dom/tests/mochitest/general/test_interfaces.html
Assignee | ||
Comment 97•11 years ago
|
||
Normalize DOM interface names to starting with 'Moz'
Attachment #8339809 -
Attachment is obsolete: true
Assignee | ||
Comment 98•11 years ago
|
||
Normalize DOM interface names to starting with 'Moz'
Attachment #8339810 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8349895 -
Attachment description: Bug 811635 - Part1: idl/webidl for WifiP2pManager (v5, r+'ed) → Part1: idl/webidl for WifiP2pManager (v5, r+'ed)
Assignee | ||
Updated•11 years ago
|
Attachment #8349900 -
Attachment description: Part5: Starting wpa_supplicant with proper p2pSupported flag → Part5: Starting wpa_supplicant with proper p2pSupported flag (r+'ed)
Assignee | ||
Comment 105•11 years ago
|
||
Attachment #8342241 -
Attachment is obsolete: true
Attachment #8349901 -
Flags: review?(vchang)
Assignee | ||
Comment 106•11 years ago
|
||
Try result of the latest patches: https://tbpl.mozilla.org/?tree=Try&rev=cbbb6e52a83f
Comment 107•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 108•11 years ago
|
||
(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
Updated•11 years ago
|
Attachment #8349901 -
Flags: review+
Comment 110•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8349901 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 111•11 years ago
|
||
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.
Assignee | ||
Comment 112•11 years ago
|
||
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 113•11 years ago
|
||
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)
Assignee | ||
Comment 114•11 years ago
|
||
(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 :)
Assignee | ||
Comment 115•11 years ago
|
||
(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 116•11 years ago
|
||
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)
Assignee | ||
Comment 118•11 years ago
|
||
Rebased again
Attachment #8349899 -
Attachment is obsolete: true
Assignee | ||
Comment 119•11 years ago
|
||
Updated commit message
Attachment #8349901 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8359710 -
Attachment description: Part6: DOM API Test → Part6: DOM API Test (r+'d updated commit message)
Assignee | ||
Comment 121•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Blocks: b2g-WLAN-2.0
Comment 122•11 years ago
|
||
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?
Flags: in-testsuite?
Keywords: checkin-needed
Comment 123•11 years ago
|
||
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
Assignee | ||
Comment 124•11 years ago
|
||
(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 :(
Assignee | ||
Comment 125•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8359710 -
Attachment is obsolete: true
Assignee | ||
Comment 126•11 years ago
|
||
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 127•11 years ago
|
||
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)
Assignee | ||
Comment 128•11 years ago
|
||
(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•11 years ago
|
||
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?
Assignee | ||
Comment 130•11 years ago
|
||
(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•11 years ago
|
||
> 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.
Assignee | ||
Comment 135•11 years ago
|
||
Assignee | ||
Comment 136•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8365761 -
Attachment is obsolete: true
Attachment #8365761 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 137•11 years ago
|
||
(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•11 years ago
|
||
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+
Assignee | ||
Comment 139•11 years ago
|
||
(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•11 years ago
|
||
Makes sense, thanks! Would be nice if we actually had it running in both environments... Maybe a followup bug on that?
Assignee | ||
Comment 141•11 years ago
|
||
(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!
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 143•11 years ago
|
||
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
Keywords: checkin-needed
Comment 144•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Keywords: dev-doc-needed
Updated•10 years ago
|
Depends on: CVE-2015-4494
You need to log in
before you can comment on or make changes to this bug.
Description
•