919426 - Extract network utilities and wifi native command from WifiWorker.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Henry Chang [:hchang]]

During the development of Wifi Direct, some of the common wifi native command and network facilities will be extracted to separate jsm. In order to remove the duplicated part and reduce the number of lines in WifiWorker.js, we can re-use those jsm in WifiWorker.js

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 919426 - Part 1: Add new file WifiCommand.jsm

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug 919426 - Part 2: Added new file WifiNetUtil.jsm

Comment 3

[Henry Chang [:hchang]]

Attachment: Bug 919426 - Part 3: Make use of WifiCommand/WifiNetUtil

Comment 4

[Henry Chang [:hchang]]

What this patch does:

1. Move the fundamental control functions from WifiWorker to WifiCommand:
- voidControlMessage
- doCommand
- doIntCommand
- doBooleanCommand
- doStringCommand

2. Move all fundamental wifi functions from WifiWorker to WifiCommand:
//-----------------------------------------
// Direct replacement
//-----------------------------------------
- startSupplicant
- terminateSupplicant
- stopSupplicant
- killSupplicant
- connectToSupplicant
- closeSupplicantConnection
- listNetworksCommand
- addNetworkCommand
- setNetworkVariableCommand
- getNetworkVariableCommand
- removeNetworkCommand
- enableNetworkCommand
- disableNetworkCommand
- statusCommand
- pingCommand
- scanResultsCommand
- disconnectCommand
- reconnectCommand
- reassociateCommand
- doSetScanModeCommand
- setLogLevel
- getLogLevel
- wpsPbcCommand
- wpsPinCommand
- wpsCancelCommand
- startDriverCommand
- stopDriverCommand
- startPacketFiltering
- stopPacketFiltering
- doGetRssiCommand
- getRssiCommand
- getRssiApproxCommand
- getLinkSpeedCommand
- getConnectionInfoGB
- getConnectionInfoICS
- getMacAddressCommand
- setPowerModeCommandICS
- setPowerModeCommandJB
- getPowerModeCommand
- setNumAllowedChannelsCommand
- getNumAllowedChannelsCommand
- setBluetoothCoexistenceModeCommand
- setBluetoothCoexistenceScanModeCommand
- saveConfigCommand
- reloadConfigCommand
- setScanResultHandlingCommand
- addToBlacklistCommand
- clearBlacklistCommand
- setSuspendOptimizationsCommand

//------------------------------------------------------
// Some functions are fundamental but keep information 
// across function calls. So we don't direct replace 
// those functions. Instead we implement the actual 
// fundamental ones and make use of them in WifiWorker
// These functions are regarded as internal APIs.
//------------------------------------------------------
- loadDriver
- unloadDriver
- setBackgroundScan
- scanCommand 
- setScanModeCommand
- 

3. Move all fundamental network functions from WifiWorker to WifiNetUtil
- enableInterface
- disableInterface
- addHostRoute
- removeHostRoutes
- setDefaultRoute
- getDefaultRoute
- removeDefaultRoute
- resetConnections
- runDhcp
- stopDhcp
- releaseDhcpLease
- getDhcpError
- configureInterface
- runDhcpRenew
- runIpConfig

p.s. In the current attached patch, dhcpInfo is a member variable of WifiNetUtil.
     I already move it out of WifiNetUtil in the new patch. I'll attach the latest
     patch after getting the initial feedback. Thanks.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 809627 [details] [diff] [review]
Bug 919426 - Part 1: Add new file WifiCommand.jsm

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

The patch looks good. Thank you. 
Please help to address some nits below.

::: dom/wifi/WifiCommand.jsm
@@ +9,5 @@
> +// JSHint directive to suppressing warnings
> +/*global dump*/
> +/*global Components*/
> +/*global libcutils*/
> +

Why do we need these comments ?

@@ +210,5 @@
> +
> +  command.getLinkSpeed = function (callback) {
> +    doStringCommand("DRIVER LINKSPEED", function(reply) {
> +      if (reply) {
> +        reply = reply.split(" ")[1] | 0; // Format: LinkSpeed XX

period at end of sentence.

@@ +246,5 @@
> +
> +  command.getMacAddress = function (callback) {
> +    doStringCommand("DRIVER MACADDR", function(reply) {
> +      if (reply) {
> +        reply = reply.split(" ")[2]; // Format: Macaddr = XX.XX.XX.XX.XX.XX

period at end of sentence.

@@ +264,5 @@
> +  command.getPowerMode = function (callback) {
> +    doStringCommand("DRIVER GETPOWER", function(reply) {
> +      if (reply) {
> +        reply = (reply.split()[2]|0); // Format: powermode = XX
> +      }

period at end of sentence.

@@ +276,5 @@
> +
> +  command.getNumAllowedChannels = function (callback) {
> +    doStringCommand("DRIVER SCAN-CHANNELS", function(reply) {
> +      if (reply) {
> +        reply = (reply.split()[2]|0); // Format: Scan-Channels = X

period at end of sentence.

@@ +292,5 @@
> +                     "OK", callback);
> +  };
> +
> +  command.saveConfig = function (callback) {
> +    // Make sure we never write out a value for AP_SCAN other than 1

period at end of sentence.

@@ +330,5 @@
> +
> +  command.getMacAddress = function(callback) {
> +    doStringCommand("DRIVER MACADDR", function(reply) {
> +      if (reply) {
> +        reply = reply.split(" ")[2]; // Format: Macaddr = XX.XX.XX.XX.XX.XX

period at end of sentence.

@@ +355,5 @@
> +  /*
> +  command.p2pDisconnect = function(callback) {
> +    doBooleanCommand("P2P_GROUP_REMOVE p2p0", "OK", callback);
> +  };
> +  */

Do we still need this ? If not, please remove it.

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

Is it constant value ? Not sure if it's a platform dependent variable ?

@@ +364,5 @@
> +
> +  command.p2pEnable = function(deviceName, callback) {
> +    var deviceType = "10-0050F204-5";
> +
> +    var COMMAND_CHAIN = [ "SET device_name " + deviceName,

s/COMMAND_CHAIN/commandChain/

@@ +427,5 @@
> +  function doBooleanCommandChain(commandChain, callback, i) {
> +    if (undefined === i) {
> +      i = 0;
> +    }
> +

Maybe we can pass i=-1 when we call this function first time ? So that we can remove the condition check here.

@@ +434,5 @@
> +        return callback(false);
> +      }
> +      i++;
> +      if (i === commandChain.length || !commandChain[i]) {
> +        // reach the end or empty command

period at end of sentence.

Comment 6

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #5)
> Comment on attachment 809627 [details] [diff] [review]
> Bug 919426 - Part 1: Add new file WifiCommand.jsm
> 
> Review of attachment 809627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good. Thank you. 
> Please help to address some nits below.
> 
> ::: dom/wifi/WifiCommand.jsm
> @@ +9,5 @@
> > +// JSHint directive to suppressing warnings
> > +/*global dump*/
> > +/*global Components*/
> > +/*global libcutils*/
> > +
> 
> Why do we need these comments ?

They are jshint directives to declare the external symbols.

> @@ +355,5 @@
> > +  /*
> > +  command.p2pDisconnect = function(callback) {
> > +    doBooleanCommand("P2P_GROUP_REMOVE p2p0", "OK", callback);
> > +  };
> > +  */
> 
> Do we still need this ? If not, please remove it.

Well, I think I can just remove all p2p functions
since they won't be used at this time.
 
> @@ +427,5 @@
> > +  function doBooleanCommandChain(commandChain, callback, i) {
> > +    if (undefined === i) {
> > +      i = 0;
> > +    }
> > +
> 
> Maybe we can pass i=-1 when we call this function first time ? So that we
> can remove the condition check here.

It's the common default argument alternative in javascript. If the internal
coding guide considers this a bad practice or harmful, I can change all the
calling point to doBooleanCommandChain(xxxChain, callback, 0)

I'll attach a new patch to add all the missing trailing period, remove
all p2p functions and add my dhcpInfo changes I mentioned in the previous post.

Thanks!

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 809628 [details] [diff] [review]
Bug 919426 - Part 2: Added new file WifiNetUtil.jsm

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

Looks good. Thank you.

::: dom/wifi/WifiNetUtil.jsm
@@ +11,5 @@
> +/*global XPCOMUtils*/
> +/*global gNetworkManager*/
> +/*global libcutils*/
> +/*global dump*/
> +

Why do we need these comments ?

@@ +125,5 @@
> +
> +  util.releaseDhcpLease = function (ifname, callback) {
> +    controlMessage({ cmd: "dhcp_release_lease", ifname: ifname }, function(data) {
> +      util.dhcpInfo = null;
> +      //notify("dhcplost");

Please remove it.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 809629 [details] [diff] [review]
Bug 919426 - Part 3: Make use of WifiCommand/WifiNetUtil

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

Looks good.

Comment 9

[Henry Chang [:hchang]]

Attachment: Bug 919426 - Part1: Add WifiCommand.jsm v2

Comment 10

[Henry Chang [:hchang]]

Attachment: Bug 919426 - Part 2: Added new file WifiNetUtil.jsm v2

Comment 11

[Henry Chang [:hchang]]

Attachment: Bug 919426 - Part 3: Make use of WifiCommand/WifiNetUtil v2

Comment 12

[Henry Chang [:hchang]]

Added new 3 patches (marked v2). The changes are:

1) Added trailing periods.
2) Removed jshint directives.
3) Removed currently unused p2p commands.
4) Renamed COMMAND_CHAIN to commandChain.
5) Moved out WifiNetUtil::dhcpInfo to WifiWorker.js (See line 367/381/393/560/1128 @ new WifiWorker.js).

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 811795 [details] [diff] [review]
Bug 919426 - Part1: Add WifiCommand.jsm v2

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

Looks good.

Comment 14

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 811796 [details] [diff] [review]
Bug 919426 - Part 2:  Added new file WifiNetUtil.jsm v2

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

Looks good. Thank you.

Comment 15

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 811798 [details] [diff] [review]
Bug 919426 - Part 3: Make use of WifiCommand/WifiNetUtil v2

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

Looks good. Thank you.

Comment 16

[Henry Chang [:hchang]]

Thanks Vincent! I also obsoleted old patches. The current 3 patches are the ones for checkin-needed flag. Thanks.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ac83fa32e867
https://hg.mozilla.org/integration/b2g-inbound/rev/21754eba4931
https://hg.mozilla.org/integration/b2g-inbound/rev/f75c25e53e53

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac83fa32e867
https://hg.mozilla.org/mozilla-central/rev/21754eba4931
https://hg.mozilla.org/mozilla-central/rev/f75c25e53e53