Closed
Bug 935363
Opened 11 years ago
Closed 11 years ago
Extract stateless networking operations from NetworkManager to NetworkService
Categories
(Firefox OS Graveyard :: Wifi, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: hchang, Assigned: hchang)
References
Details
Attachments
(1 file, 3 obsolete files)
69.14 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
List of possible APIs of NetworkInterface: getNetworkInterfaceStats setWifiOperationMode resetRoutingTable setDNS setDefaultRouteAndDNS removeDefaultRoute addHostRoute removeHostRoute removeHostRoutes addHostRouteWithResolve removeHostRouteWithResolve setNetworkProxy setDhcpServer setWifiTethering setUSBTethering enableUsbRndis updateUpStream
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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
(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
Assignee | ||
Comment 6•11 years ago
|
||
(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!
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
(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?
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #828450 -
Attachment is obsolete: true
Attachment #830685 -
Attachment is obsolete: true
Attachment #828450 -
Flags: feedback?(vchang)
Attachment #828450 -
Flags: feedback?(gene.lian)
Assignee | ||
Updated•11 years ago
|
Attachment #830688 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #830688 -
Flags: review?(vchang)
Comment 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
(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.
Assignee | ||
Comment 15•11 years ago
|
||
> @@ +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....)
Comment 16•11 years ago
|
||
(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.
Assignee | ||
Comment 17•11 years ago
|
||
(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 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
This has been r+'d and addressed the review comments. Ready for commit.
Attachment #830688 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/f49134e2a4dc
Keywords: checkin-needed
Updated•11 years ago
|
Component: General → RIL
Hardware: x86_64 → ARM
Updated•11 years ago
|
Component: RIL → Wifi
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f49134e2a4dc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•