Closed Bug 935363 Opened 6 years ago Closed 6 years ago

Extract stateless networking operations from NetworkManager to NetworkService

Categories

(Firefox OS Graveyard :: Wifi, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(1 file, 3 obsolete files)

This is a subtask of refactoring NetworkManager. For now NetworkManager includes some stateless networking operation like set host route, start dhcp, etc, which can be extracted independently to a XPCOM say, NetworkService, and be used by NetworkManager or other modules.
List of possible APIs of NetworkInterface:

getNetworkInterfaceStats
setWifiOperationMode
resetRoutingTable
setDNS
setDefaultRouteAndDNS
removeDefaultRoute
addHostRoute
removeHostRoute
removeHostRoutes
addHostRouteWithResolve
removeHostRouteWithResolve
setNetworkProxy
setDhcpServer
setWifiTethering
setUSBTethering
enableUsbRndis
updateUpStream
Assignee: nobody → hchang
Attached patch Proposed patch. (obsolete) — Splinter Review
This patch does the following works:

1) Extract stateless, simple network operations in NetworkManager to NetworkService:

setWifiTethering
setDhcpServer
getNetworkInterfaceStats
setWifiOperationMode
setNetworkProxy
setUSBTethering
resetRoutingTable
setDNS
setDefaultRouteAndDNS
removeDefaultRoute
addHostRoute
removeHostRoute
removeHostRoutes
addHostRouteWithResolve
removeHostRouteWithResolve
enableUsbRndis
updateUpStream

For any function in the above, say |foo|, if NetworkManager::foo is (a)synchronous, NetworkService::foo will be (a)synchronous.

2) Move interfaces defined in the original nsINetworkManager.idl to nsNetworkService.idl except nsINetworkManager.

3) SystemWorkerManager uses worker in NetworkService rather than NetworkManager.
Comment on attachment 828450 [details] [diff] [review]
Proposed patch.

Asking for the feedback of this WIP. I myself have a question: do we need MOZ_B2G_RIL in NetworkService? 

Besides, there might be some follow-up task after this patch:

1) Change all the functions to async. (?)
2) Remove worker holder from NetworkService (use registerNetd or something else)
3) Rewrite to c++
4) Remove all wrapper functions in NetworkManager. User can directly call those functions via NetworkService. (or we can just do it in this patch)
Attachment #828450 - Flags: feedback?(vyang)
Attachment #828450 - Flags: feedback?(vchang)
Attachment #828450 - Flags: feedback?(gene.lian)
Comment on attachment 828450 [details] [diff] [review]
Proposed patch.

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

::: dom/system/gonk/NetworkManager.js
@@ +817,4 @@
>  
> +    gNetworkService.setWifiTethering(enable, config, function (error) {
> +      var resetSettings = error;
> +      this.notifyError(resetSettings, callback, error);

so basically |resetSettings| and |error| are the same thing :p
Blocks: 928289
(In reply to Henry Chang [:henry] from comment #3)
> Comment on attachment 828450 [details] [diff] [review]`
> 4) Remove all wrapper functions in NetworkManager. User can directly call
> those functions via NetworkService. (or we can just do it in this patch)

hmm... it's little weird to have the same interface on two different components
(In reply to John Shih from comment #4)
> Comment on attachment 828450 [details] [diff] [review]
> Proposed patch.
> 
> Review of attachment 828450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +817,4 @@
> >  
> > +    gNetworkService.setWifiTethering(enable, config, function (error) {
> > +      var resetSettings = error;
> > +      this.notifyError(resetSettings, callback, error);
> 
> so basically |resetSettings| and |error| are the same thing :p

Yes but it's more readable!
(In reply to John Shih from comment #5)
> (In reply to Henry Chang [:henry] from comment #3)
> > Comment on attachment 828450 [details] [diff] [review]`
> > 4) Remove all wrapper functions in NetworkManager. User can directly call
> > those functions via NetworkService. (or we can just do it in this patch)
> 
> hmm... it's little weird to have the same interface on two different
> components

Just because IMO I don't want this patch to jump too much...
Removing the wrapper interfaces can be done in another patch.
Comment on attachment 828450 [details] [diff] [review]
Proposed patch.

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

self review :p

::: dom/system/gonk/NetworkManager.js
@@ +707,5 @@
>  
>    handleUSBTetheringToggle: function handleUSBTetheringToggle(enable) {
>      if (!enable) {
>        this.tetheringSettings[SETTINGS_USB_ENABLED] = false;
> +      gNetworkService.enableUsbRndis(false, this.enableUsbRndisResult);

gNetworkService.enableUsbRndis(false, this.enableUsbRndisResult.bind(this));

@@ +720,5 @@
>          this._tetheringInterface[TETHERING_TYPE_USB].externalInterface = mobile.name;
>        }
>      }
>      this.tetheringSettings[SETTINGS_USB_ENABLED] = true;
> +    gNetworkService.enableUsbRndis(true, this.enableUsbRndisResult);

gNetworkService.enableUsbRndis(true, this.enableUsbRndisResult.bind(this));

@@ +817,5 @@
>  
> +    gNetworkService.setWifiTethering(enable, config, function (error) {
> +      var resetSettings = error;
> +      this.notifyError(resetSettings, callback, error);
> +    });

the callback function should bind |this|

@@ +871,3 @@
>      this._usbTetheringAction = TETHERING_STATE_IDLE;
>      // Disable tethering settings when fail to enable it.
> +    if (!error) {

should be: if (error)

@@ +919,5 @@
>  
>      let callback = (function () {
>        // Update external network interface.
>        debug("Update upstream interface to " + network.name);
> +      gNetworkService.updateUpStream(previous, current, this.onConnectionChangedReport);

gNetworkService.updateUpStream(previous, current, this.onConnectionChangedReport.bind(this));
Comment on attachment 828450 [details] [diff] [review]
Proposed patch.

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

::: dom/system/gonk/NetworkManager.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +/*global dump, Components, XPCOMUtils, Services, debug, gNetworkService*/
> +/*global gDNSService, gSettingsService, ppmm, FileUtils, CaptivePortalDetectionHelper*/

nit: personal notes?

@@ +92,5 @@
>  
>  const DEFAULT_WIFI_DHCPSERVER_STARTIP  = "192.168.1.10";
>  const DEFAULT_WIFI_DHCPSERVER_ENDIP    = "192.168.1.30";
>  
> +const DEBUG = true;

nit: remember to turn it off in final patch, or better, just don't include this change in any patch uploaded for feedback or review. :)

@@ +412,5 @@
>      this._overriddenActive = network;
>      this.setAndConfigureActive();
>    },
>  
>    getNetworkInterfaceStats: function getNetworkInterfaceStats(networkName, callback) {

I'd like to move this interface method into nsINetworkService, if Vincent, Gene don't mind.  Have a indirect call only for one debug line doesn't make sense to me.

@@ +422,4 @@
>      });
>    },
>  
>    setWifiOperationMode: function setWifiOperationMode(interfaceName, mode, callback) {

ditto

@@ +433,2 @@
>  #ifdef MOZ_B2G_RIL
>    setExtraHostRoute: function setExtraHostRoute(network) {

ditto.  Move the whole function instead.

@@ +453,4 @@
>      }
>    },
>  
>    removeExtraHostRoute: function removeExtraHostRoute(network) {

ditto

@@ +561,1 @@
>    resolveHostname: function resolveHostname(hosts) {

ditto

@@ +595,2 @@
>  
>    setNetworkProxy: function setNetworkProxy(network) {

ditto

@@ +784,5 @@
>      }
>    },
>  
>    // Enable/Disable DHCP server.
>    setDhcpServer: function setDhcpServer(enabled, config, callback) {

ditto

::: dom/system/gonk/nsINetworkManager.idl
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
> +#include "nsINetworkService.idl"

I think we don't really need it here.

@@ -7,5 @@
> -/**
> - * Information about networks that is exposed to network manager API consumers.
> - */
> -[scriptable, uuid(f4cf9d88-f962-4d29-9baa-fb295dad387b)]
> -interface nsINetworkInterface : nsISupports

Please keep it in nsINetworkManager.idl.

@@ -82,5 @@
> -
> -};
> -
> -[scriptable, function, uuid(91824160-fb25-11e1-a21f-0800200c9a66)]
> -interface nsIWifiTetheringCallback : nsISupports

ditto.
Attachment #828450 - Flags: feedback?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)
> Comment on attachment 828450 [details] [diff] [review]
> Proposed patch.
> 
> Review of attachment 828450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js

> @@ +412,5 @@
> >      this._overriddenActive = network;
> >      this.setAndConfigureActive();
> >    },
> >  
> >    getNetworkInterfaceStats: function getNetworkInterfaceStats(networkName, callback) {
> 
> I'd like to move this interface method into nsINetworkService, if Vincent,
> Gene don't mind.  Have a indirect call only for one debug line doesn't make
> sense to me.
> 
> @@ +422,4 @@
> >      });
> >    },
> >  
> >    setWifiOperationMode: function setWifiOperationMode(interfaceName, mode, callback) {
> 
> ditto
> 

setWifiTethering
setDhcpServer
getNetworkInterfaceStats
setWifiOperationMode
setNetworkProxy

For these APIs in nsINetworkManager, I think we have to make a decision here: 

Keep them in nsINetworkManager until the next patch or 
Remove them from nsINetworkManager in this patch and modify every user of nsINetworkManager.

I personally prefer the first one but the latter one is also okay for me...
But eventually they should be removed from nsINetworkManager.

> @@ +433,2 @@
> >  #ifdef MOZ_B2G_RIL
> >    setExtraHostRoute: function setExtraHostRoute(network) {
> 
> ditto.  Move the whole function instead.
> 
> @@ +453,4 @@
> >      }
> >    },
> >  
> >    removeExtraHostRoute: function removeExtraHostRoute(network) {
> 
> ditto
> 
> @@ +561,1 @@
> >    resolveHostname: function resolveHostname(hosts) {
> 
> ditto

I think setExtraHostRoute/removeExtraHostRoute/resolveHostname is a kind of "customized" 
function. We already have gDNSService.resolve(). resolveHostname() is more like
a helper function. Besides, what 'extra' to setExtraHostRoute/removeExtraHostRoute may only
be extra to NetworkManager. 

> @@ -7,5 @@
> > -/**
> > - * Information about networks that is exposed to network manager API consumers.
> > - */
> > -[scriptable, uuid(f4cf9d88-f962-4d29-9baa-fb295dad387b)]
> > -interface nsINetworkInterface : nsISupports
> 
> Please keep it in nsINetworkManager.idl.
> 
> @@ -82,5 @@
> > -
> > -};
> > -
> > -[scriptable, function, uuid(91824160-fb25-11e1-a21f-0800200c9a66)]
> > -interface nsIWifiTetheringCallback : nsISupports
> 
> ditto.

Any concern for moving out nsINetworkInterface and nsIWifiTetheringCallback?
Attached patch Patch for review (obsolete) — Splinter Review
Attached patch Patch for review (obsolete) — Splinter Review
Attachment #828450 - Attachment is obsolete: true
Attachment #830685 - Attachment is obsolete: true
Attachment #828450 - Flags: feedback?(vchang)
Attachment #828450 - Flags: feedback?(gene.lian)
Attachment #830688 - Flags: review?(vyang)
Attachment #830688 - Flags: review?(vchang)
Comment on attachment 830688 [details] [diff] [review]
Patch for review

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

::: dom/system/gonk/NetworkManager.js
@@ -285,3 @@
>  #endif
>              }
> -

nit: keep this empty line for a break between two code chunks of different purposes.

@@ +781,3 @@
>  
> +    gNetworkService.setWifiTethering(enable, config, (function (error) {
> +      var resetSettings = error;

nit: let

@@ +791,5 @@
>                                              callback) {
>      let params = this.getUSBTetheringParameters(enable, tetheringInterface);
>  
>      if (params === null) {
> +      gNetworkService.enableUsbRndis(false, function() {});

|gNetworkService.enableUsbRndis(false);| and make that callback parameter optional.

::: dom/system/gonk/NetworkService.js
@@ +23,5 @@
> +const NETD_COMMAND_ERROR        = 500;
> +// 6xx - Unsolicited broadcasts
> +const NETD_COMMAND_UNSOLICITED  = 600;
> +
> +const WIFI_CTRL_INTERFACE         = "wl0.1";

nit: alignment, just strip extra ws.

@@ +30,5 @@
> +
> +const DEBUG = false;
> +
> +function netdResponseType(code) {
> +  return Math.floor(code/100)*100;

SP between operator and operants.

@@ +40,5 @@
> +}
> +
> +function debug(msg) {
> +  if (DEBUG) {
> +    dump("-*- NetworkService: " + msg + "\n");

For new code, we should wrap all debug lines with DEBUG instead.  That is:

  function debug(msg) {
    dump("-*- NetworkService: " + msg + "\n");
  }

  if (DEBUG) debug("foo");

So that we don't bother preparing messages that won't be printed out.

@@ +49,5 @@
> + * This component watches for network interfaces changing state and then
> + * adjusts routes etc. accordingly.
> + */
> +function NetworkService() {
> +  debug("Starting worker.");

nit: "Starting net_worker" or something alike will be much more descriptive.
Attachment #830688 - Flags: review?(vyang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> Comment on attachment 830688 [details] [diff] [review]
> Patch for review
> 
> Review of attachment 830688 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review. Only one question though.

> @@ +781,3 @@
> >  
> > +    gNetworkService.setWifiTethering(enable, config, (function (error) {
> > +      var resetSettings = error;
> 
> nit: let
> 

Does it mean we should always use 'let' rather than 'var' in gecko?
I use 'var' here just because 'var' and 'let' makes no difference
at the top level scope in a function.
> @@ +791,5 @@
> >                                              callback) {
> >      let params = this.getUSBTetheringParameters(enable, tetheringInterface);
> >  
> >      if (params === null) {
> > +      gNetworkService.enableUsbRndis(false, function() {});
> 
> |gNetworkService.enableUsbRndis(false);| and make that callback parameter
> optional.

By the way, instead of making the callback optional, would it be better of changing to

if (params === null) {
  gNetworkService.enableUsbRndis(false, function(error) {
    this.usbTetheringResultReport("Invalid parameters");
  });
  return;
}

?

(since making only the callback of nsINetworkService::enableUsbRndis optional might not be natural....)
(In reply to Henry Chang [:henry] from comment #14)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> > Comment on attachment 830688 [details] [diff] [review]
> > Patch for review
> > 
> > Review of attachment 830688 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thanks for the review. Only one question though.
> 
> > @@ +781,3 @@
> > >  
> > > +    gNetworkService.setWifiTethering(enable, config, (function (error) {
> > > +      var resetSettings = error;
> > 
> > nit: let
> > 
> 
> Does it mean we should always use 'let' rather than 'var' in gecko?

No for components other that RIL.  Limit the scope of a variable and use the most strict expression delivers a more clear message to other developers that are going to refactor/extend your code.  There are still a few, not more then 5, 'var' usage in dom/system/gonk/<ril js>.  They should be corrected someday.  Yes, we should also use '===' and '!==' as possible.

> I use 'var' here just because 'var' and 'let' makes no difference
> at the top level scope in a function.

Not until you move the lines around.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> (In reply to Henry Chang [:henry] from comment #14)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> > > Comment on attachment 830688 [details] [diff] [review]
> > > Patch for review
> > > 
> > > Review of attachment 830688 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > 
> > Thanks for the review. Only one question though.
> > 
> > > @@ +781,3 @@
> > > >  
> > > > +    gNetworkService.setWifiTethering(enable, config, (function (error) {
> > > > +      var resetSettings = error;
> > > 
> > > nit: let
> > > 
> > 
> > Does it mean we should always use 'let' rather than 'var' in gecko?
> 
> No for components other that RIL.  Limit the scope of a variable and use the
> most strict expression delivers a more clear message to other developers
> that are going to refactor/extend your code.  There are still a few, not
> more then 5, 'var' usage in dom/system/gonk/<ril js>.  They should be
> corrected someday.  Yes, we should also use '===' and '!==' as possible.
> 

Totally agree. My personal policy is to use 'var' at the top level of a function
and use 'let' in a block scope.

> > I use 'var' here just because 'var' and 'let' makes no difference
> > at the top level scope in a function.
> 
> Not until you move the lines around.

I buy this argument :p 

I think I should check all code I wrote before to replace 'var' with 'let'...

Thanks~
Comment on attachment 830688 [details] [diff] [review]
Patch for review

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

r+ with vyang's comments addressed.
Attachment #830688 - Flags: review?(vchang) → review+
This has been r+'d and addressed the review comments. Ready for commit.
Attachment #830688 - Attachment is obsolete: true
Keywords: checkin-needed
Component: General → RIL
Hardware: x86_64 → ARM
Component: RIL → Wifi
https://hg.mozilla.org/mozilla-central/rev/f49134e2a4dc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.