Closed Bug 919426 Opened 6 years ago Closed 6 years ago

Extract network utilities and wifi native command from WifiWorker.js

Categories

(Firefox OS Graveyard :: Wifi, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(3 files, 3 obsolete files)

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
Blocks: 811635
Attachment #809627 - Attachment description: Add new file WifiCommand.jsm → Bug 919426 - Part 1: Add new file WifiCommand.jsm
Attachment #809627 - Attachment filename: Bug929426_1.patch → Bug919426_1.patch
Attachment #809627 - Flags: review?(vchang)
Attachment #809628 - Flags: review?(vchang)
Attachment #809629 - Flags: review?(vchang)
Assignee: nobody → 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 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.
Attachment #809627 - Flags: review?(vchang)
(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 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.
Attachment #809628 - Flags: review?(vchang) → review+
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.
Attachment #809629 - Flags: review?(vchang) → review+
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).
Attachment #811795 - Flags: review?(vchang)
Attachment #811796 - Flags: review?(vchang)
Attachment #811798 - Flags: review?(vchang)
Comment on attachment 811795 [details] [diff] [review]
Bug 919426 - Part1: Add WifiCommand.jsm v2

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

Looks good.
Attachment #811795 - Flags: review?(vchang) → review+
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.
Attachment #811796 - Flags: review?(vchang) → review+
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.
Attachment #811798 - Flags: review?(vchang) → review+
Attachment #809627 - Attachment is obsolete: true
Attachment #809628 - Attachment is obsolete: true
Attachment #809629 - Attachment is obsolete: true
Thanks Vincent! I also obsoleted old patches. The current 3 patches are the ones for checkin-needed flag. Thanks.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.