I think we could move tethering code to an independent module, it would make the code clearer. I am thinking of calling it "TetheringManager", but is already used... orz

Comment on attachment 8561259 [details] [diff] [review] patch, v1. Edgar, I have moved tethering related code to a new component - TetheringService, code flow and logic remains unchanged, I'll file separate bugs for issues that I've found while moving the code. May I have your review on this? Thanks.

Comment on attachment 8561259 [details] [diff] [review] patch, v1. Review of attachment 8561259 [details] [diff] [review]: ----------------------------------------------------------------- Since you just move the code to a new file and haven't change the logic, I did not check logic very carefully, to be honest. :p And it is a good chance to correct the coding style. Please see my comments below, thank you. ::: dom/system/gonk/TetheringService.js @@ +90,5 @@ > +const MOBILE_DUN_CONNECT_TIMEOUT = 30000; > +const MOBILE_DUN_RETRY_INTERVAL = 5000; > +const MOBILE_DUN_MAX_RETRIES = 5; > + > +let DEBUG = false; Please also add supporting for dynamically enabling debug logging by modifying pref, maybe we could just reuse the existed pref, e.g. network.debugging.enabled. @@ +105,5 @@ > + // Possible usb tethering interfaces for different gonk platform. > + this.possibleInterface = POSSIBLE_USB_INTERFACE_NAME.split(","); > + > + // Default values for internal and external interfaces. > + this._tetheringInterface = Object.create(null); this._tetheringInterface = {}; @@ +158,5 @@ > + Ci.nsISettingsServiceCallback]), > + > + // nsIObserver > + > + observe: function(subject, topic, data) { Having `a` prefix for argument, here and elsewhere. @@ +195,5 @@ > + Services.obs.removeObserver(this, TOPIC_INTERFACE_REGISTERED); > + Services.obs.removeObserver(this, TOPIC_INTERFACE_UNREGISTERED); > + > + this.dunConnectTimer.cancel(); > + this.dunRetryTimer.cancel(); nit: break out even in the last case. @@ +203,5 @@ > + // nsISettingsServiceCallback > + > + _dataDefaultServiceId: null, > + > + _usbTetheringRequestCount: 0, We usually put a variable along with a function only when the variable is only used in that function. Since |_dataDefaultServiceId| and |_usbTetheringRequestCount| are used in many different places, I prefer putting them in the beginning of this object's prototype, around line #158. @@ +295,5 @@ > + } > + return null; > + }, > + > + _usbTetheringAction: TETHERING_STATE_IDLE, Ditto, putting it around line #158. @@ +297,5 @@ > + }, > + > + _usbTetheringAction: TETHERING_STATE_IDLE, > + > + _usbTetheringSettingsToRead: [], Ditto, and initialize it to null. @@ +299,5 @@ > + _usbTetheringAction: TETHERING_STATE_IDLE, > + > + _usbTetheringSettingsToRead: [], > + > + _oldUsbTetheringEnabledState: null, Ditto. @@ +302,5 @@ > + > + _oldUsbTetheringEnabledState: null, > + > + // External and internal interface name. > + _tetheringInterface: null, Ditto. @@ +332,5 @@ > + this._pendingWifiTetheringRequestArgs = null; > + } > + }, > + > + dunConnectTimer: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer), Ditto, and initialize it to null, and create timer object in `TetheringService() {...}`. @@ +346,5 @@ > + } > + } > + }, > + > + dunRetryTimes: 0, Ditto. @@ +348,5 @@ > + }, > + > + dunRetryTimes: 0, > + > + dunRetryTimer: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer), Ditto. @@ +381,5 @@ > + initWithCallback(this.setupDunConnection.bind(this), > + MOBILE_DUN_RETRY_INTERVAL, Ci.nsITimer.TYPE_ONE_SHOT); > + }, > + > + _pendingTetheringRequests: [], Ditto. @@ +486,5 @@ > + let dns2; > + let internalInterface = tetheringinterface.internalInterface; > + let externalInterface = tetheringinterface.externalInterface; > + > + interfaceIp = this.tetheringSettings[SETTINGS_USB_IP]; Merge with declaration, here and below. e.g. let interfaceIp = this.tetheringSettings[SETTINGS_USB_IP]; @@ +537,5 @@ > + }, Ci.nsIThread.DISPATCH_NORMAL); > + } > + }, > + > + _wifiTetheringRequestOngoing: false, Ditto, putting it around line #158. @@ +561,5 @@ > + if (this._usbTetheringRequestCount > 0) { > + debug('Perform pending USB tethering requests.'); > + this.handleLastUsbTetheringRequest(); > + } > + }).bind(this)); Use arrow function instead. @@ +564,5 @@ > + } > + }).bind(this)); > + }, > + > + _pendingWifiTetheringRequestArgs: null, Ditto, putting it around line #158. @@ +604,5 @@ > + return; > + } > + this._tetheringInterface[TETHERING_TYPE_WIFI].externalInterface = network.name; > + this.enableWifiTethering(true, config, callback); > + }.bind(this, config, callback)); Use arrow function instead. @@ +771,5 @@ > + callback.call(this); > + }, > +}; > + > +XPCOMUtils.defineLazyGetter(TetheringService.prototype, "mRil", function() { Hmm, how about renaming it to 'gRil', appending it global scope and also putting this LazyGetter along with other LazyGetter? I know we use 'mRil' in NetworkManager, too. But on a second thought, using `gRIL` probably makes more sense. What do you think? @@ +784,5 @@ > + > +let debug; > +if (DEBUG) { > + debug = function(s) { > + dump("-*- TetheringService: " + s + "\n"); Put all these lines along with `let DEBUG = false;` in line #94. ::: dom/system/gonk/nsITetheringService.idl @@ +15,5 @@ > + * > + * @param enabled > + * Boolean that indicates whether tethering should be enabled (true) or > + * disabled (false). > + * @param network s/network/networkInterface/

Address review comment 3: - enable dynamic debugging using 'network.debugging.enabled' - put some variables declaration in the beginning of the object's prototype - use arrow function instead - use 'gRil' instead of 'mRil' and append it global scope

Comment on attachment 8564875 [details] [diff] [review] patch, v2. Edgar, thanks for the feedback in comment 3, I have addressed them in this patch. May I have your review again?

Comment on attachment 8564875 [details] [diff] [review] patch, v2. Review of attachment 8564875 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks good (I haven't test this patch yet), just one question, please see my in-line comment. Thank you. ::: dom/system/gonk/TetheringService.js @@ +281,5 @@ > + this._dataDefaultServiceId = aResult || 0; > + debug("'_dataDefaultServiceId' is now " + this._dataDefaultServiceId); > + break; > + case SETTINGS_USB_ENABLED: > + let _oldUsbTetheringEnabledState = this.tetheringSettings[SETTINGS_USB_ENABLED]; Should this be `this._oldUsbTetheringEnabledState`? It looks like a type to me or you change to use `let` for some reason?

(In reply to Edgar Chen [:edgar][:echen] from comment #6) > Comment on attachment 8564875 [details] [diff] [review] > patch, v2. > > Review of attachment 8564875 [details] [diff] [review]: > ----------------------------------------------------------------- > > Mostly looks good (I haven't test this patch yet), just one question, please > see my in-line comment. Thank you. > > ::: dom/system/gonk/TetheringService.js > @@ +281,5 @@ > > + this._dataDefaultServiceId = aResult || 0; > > + debug("'_dataDefaultServiceId' is now " + this._dataDefaultServiceId); > > + break; > > + case SETTINGS_USB_ENABLED: > > + let _oldUsbTetheringEnabledState = this.tetheringSettings[SETTINGS_USB_ENABLED]; > > Should this be `this._oldUsbTetheringEnabledState`? It looks like a type to ^^^^ Sorry, it should be 'typo'. :p > me or you change to use `let` for some reason?

(In reply to Edgar Chen [:edgar][:echen] from comment #7) > (In reply to Edgar Chen [:edgar][:echen] from comment #6) > > Comment on attachment 8564875 [details] [diff] [review] > > patch, v2. > > > > Review of attachment 8564875 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Mostly looks good (I haven't test this patch yet), just one question, please > > see my in-line comment. Thank you. > > > > ::: dom/system/gonk/TetheringService.js > > @@ +281,5 @@ > > > + this._dataDefaultServiceId = aResult || 0; > > > + debug("'_dataDefaultServiceId' is now " + this._dataDefaultServiceId); > > > + break; > > > + case SETTINGS_USB_ENABLED: > > > + let _oldUsbTetheringEnabledState = this.tetheringSettings[SETTINGS_USB_ENABLED]; > > > > Should this be `this._oldUsbTetheringEnabledState`? It looks like a type to > ^^^^ > Sorry, it should be 'typo'. :p > > me or you change to use `let` for some reason? Oh, you are right. I was trying to do something else, then I changed it back. Will upload an updated version to fix this. Thanks for catching this!

Edgar, may I have your review again? Thanks.

Comment on attachment 8569694 [details] [diff] [review] patch, v3. Found some bugs, will update another version to fix them.

Acquire network interfaces from NetworkManager. Didn't catch this because the default interface was used, and it was workable. :(

Comment on attachment 8571720 [details] [diff] [review] patch, v4. Review of attachment 8571720 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I also did some basic test regarding to tethering and it works good on flame-kk. Thank you, Jessica.

Thank you, Edgar! try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=002fdc2f69fc