Closed Bug 1109479 Opened 9 years ago Closed 9 years ago

B2G tethering: move tethering code out of NetworkManager

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(tracking-b2g:backlog, firefox39 fixed)

RESOLVED FIXED
2.2 S7 (6mar)
tracking-b2g backlog
Tracking Status
firefox39 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

(Whiteboard: [grooming])

Attachments

(1 file, 3 obsolete files)

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
Blocks: 904514
Assignee: nobody → jjong
blocking-b2g: --- → backlog
Whiteboard: [grooming]
Attached patch patch, v1. (obsolete) — Splinter Review
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.
Attachment #8561259 - Flags: review?(echen)
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/
Attachment #8561259 - Flags: review?(echen)
Attached patch patch, v2. (obsolete) — Splinter Review
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
Attachment #8561259 - Attachment is obsolete: true
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?
Attachment #8564875 - Flags: review?(echen)
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?
Attachment #8564875 - Flags: review?(echen)
(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!
Attached patch patch, v3. (obsolete) — Splinter Review
Edgar, may I have your review again? Thanks.
Attachment #8564875 - Attachment is obsolete: true
Attachment #8569694 - Flags: review?(echen)
Comment on attachment 8569694 [details] [diff] [review]
patch, v3.

Found some bugs, will update another version to fix them.
Attachment #8569694 - Flags: review?(echen)
Attached patch patch, v4.Splinter Review
Acquire network interfaces from NetworkManager. Didn't catch this because the default interface was used, and it was workable. :(
Attachment #8569694 - Attachment is obsolete: true
Attachment #8571720 - Flags: review?(echen)
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.
Attachment #8571720 - Flags: review?(echen) → review+
https://hg.mozilla.org/mozilla-central/rev/0b754b3afe46
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S7 (6mar)
blocking-b2g: backlog → ---
Blocks: 1217298
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: