Closed Bug 1167132 Opened 5 years ago Closed 4 years ago

[NetworkManager] move network information into a separate interface

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.5+, tracking-b2g:backlog, firefox42 fixed)

RESOLVED FIXED
feature-b2g 2.5+
tracking-b2g backlog
Tracking Status
firefox42 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(15 files, 35 obsolete files)

17.02 KB, patch
jessica
: review+
Details | Diff | Splinter Review
4.67 KB, patch
edgar
: review+
Details | Diff | Splinter Review
11.84 KB, patch
jessica
: review+
Details | Diff | Splinter Review
42.97 KB, patch
jessica
: review+
Details | Diff | Splinter Review
17.34 KB, patch
jessica
: review+
Details | Diff | Splinter Review
14.51 KB, patch
jessica
: review+
Details | Diff | Splinter Review
3.77 KB, patch
jessica
: review+
Details | Diff | Splinter Review
6.98 KB, patch
jessica
: review+
Details | Diff | Splinter Review
1.25 KB, patch
jessica
: review+
Details | Diff | Splinter Review
3.57 KB, patch
jessica
: review+
Details | Diff | Splinter Review
7.37 KB, patch
jessica
: review+
Details | Diff | Splinter Review
1.65 KB, patch
jessica
: review+
Details | Diff | Splinter Review
20.46 KB, patch
jessica
: review+
Details | Diff | Splinter Review
23.32 KB, patch
jessica
: review+
Details | Diff | Splinter Review
13.25 KB, patch
jessica
: review+
Details | Diff | Splinter Review
We will move network information needed by other modules into a separate interface, say nsINetworkInfo, so we'll need to expose only this interface and not the entire nsINetworkInterface.
Assignee: nobody → jjong
Comment on attachment 8609217 [details] [diff] [review]
Part 1: interface changes, v1.

Edgar, may I have your feedback on the interface changes? Thanks.
Attachment #8609217 - Flags: feedback?(echen)
(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> Comment on attachment 8609217 [details] [diff] [review]
> Part 1: interface changes, v1.
> 
> Edgar, may I have your feedback on the interface changes? Thanks.

While modifying the changes needed in other modules, I realized we may need to change addHostRoute()/removeHostRoute() APIs not to take nsINetworkInterface as parameter, cause 'network-connection-state-changed' event is fired with nsINetworkInfo and not nsINetworkInterface now. How about just using Ci.nsINetworkInfo.NETWORK_TYPE_* as parameter?
(In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> > Comment on attachment 8609217 [details] [diff] [review]
> > Part 1: interface changes, v1.
> > 
> > Edgar, may I have your feedback on the interface changes? Thanks.
> 
> While modifying the changes needed in other modules, I realized we may need
> to change addHostRoute()/removeHostRoute() APIs not to take
> nsINetworkInterface as parameter, cause 'network-connection-state-changed'
> event is fired with nsINetworkInfo and not nsINetworkInterface now. How
> about just using Ci.nsINetworkInfo.NETWORK_TYPE_* as parameter?

Taking only Ci.nsINetworkInfo.NETWORK_TYPE_* as parameter probably isn't enough. For mobile connection, we use serviceId to distinguish different sim slot.
(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> > > Comment on attachment 8609217 [details] [diff] [review]
> > > Part 1: interface changes, v1.
> > > 
> > > Edgar, may I have your feedback on the interface changes? Thanks.
> > 
> > While modifying the changes needed in other modules, I realized we may need
> > to change addHostRoute()/removeHostRoute() APIs not to take
> > nsINetworkInterface as parameter, cause 'network-connection-state-changed'
> > event is fired with nsINetworkInfo and not nsINetworkInterface now. How
> > about just using Ci.nsINetworkInfo.NETWORK_TYPE_* as parameter?
> 
> Taking only Ci.nsINetworkInfo.NETWORK_TYPE_* as parameter probably isn't
> enough. For mobile connection, we use serviceId to distinguish different sim
> slot.

Oh, you're right. So, I guess we should use nsINetworkInfo as parameter for addHostRoute()/removeHostRoute()?
(In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> Oh, you're right. So, I guess we should use nsINetworkInfo as parameter for
> addHostRoute()/removeHostRoute()?

Yes. There seems no other better idea.
Attached patch Part 1: interface changes, v2. (obsolete) — Splinter Review
Added changes for addHostRoute()/removeHostRoute().
Attachment #8609217 - Attachment is obsolete: true
Attachment #8609217 - Flags: feedback?(echen)
Attachment #8610418 - Flags: feedback?(echen)
Comment on attachment 8610418 [details] [diff] [review]
Part 1: interface changes, v2.

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

Looks good, thank you.

::: dom/system/gonk/nsIDataCallManager.idl
@@ +7,2 @@
>  
> +interface nsINetworkInfo;

Don't need this forward declaration.
Attachment #8610418 - Flags: feedback?(echen) → feedback+
[Tracking Requested - why for this release]:
Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r=echen
Attachment #8620764 - Flags: review?(echen)
Bug 1167132 - Part 2: [NetworkManager] move network information into a separate interface (NetworkManager)
Attachment #8620766 - Flags: review?(echen)
Bug 1167132 - Part 3: [NetworkManager] move network information into a separate interface (Wifi)
Attachment #8620767 - Flags: review?(hchang)
Bug 1167132 - Part 4: [NetworkManager] move network information into a separate interface (DataCall)
Attachment #8620768 - Flags: review?(echen)
Bug 1167132 - Part 5: [NetworkManager] move network information into a separate interface (Tethering)
Bug 1167132 - Part 6: [NetworkManager] move network information into a separate interface (RadioInterface)
Bug 1167132 - Part 7: [NetworkManager] move network information into a separate interface (MobileConnectionService)
Bug 1167132 - Part 8: [NetworkManager] move network information into a separate interface (MmsService)
Bug 1167132 - Part 9: [NetworkManager] move network information into a separate interface (shell)
Bug 1167132 - Part 10: [NetworkManager] move network information into a separate interface (PushService)
Bug 1167132 - Part 11: [NetworkManager] move network information into a separate interface (Gps)
Bug 1167132 - Part 12: [NetworkManager] move network information into a separate interface (NetStatistics)
Bug 1167132 - Part 13: [NetworkManager] move network information into a separate interface (discovery)
Bug 1167132 - Part 14: [NetworkManager] move network information into a separate interface (NetworkStats)
Bug 1167132 - Part 15: [NetworkManager] move network information into a separate interface (Mtransport)
Comment on attachment 8620779 [details]
MozReview Request: Bug 1167132 - Part 15: [NetworkManager] move network information into a separate interface (Mtransport)

Bug 1167132 - Part 15: [NetworkManager] move network information into a separate interface (Mtransport)
Attachment #8620764 - Attachment is obsolete: true
Attachment #8620764 - Flags: review?(echen)
Attachment #8620766 - Attachment is obsolete: true
Attachment #8620766 - Flags: review?(echen)
Attachment #8620767 - Attachment is obsolete: true
Attachment #8620767 - Flags: review?(hchang)
Attachment #8620768 - Attachment is obsolete: true
Attachment #8620768 - Flags: review?(echen)
Attachment #8620769 - Attachment is obsolete: true
Attachment #8620770 - Attachment is obsolete: true
Attachment #8620771 - Attachment is obsolete: true
Attachment #8620772 - Attachment is obsolete: true
Attachment #8620773 - Attachment is obsolete: true
Attachment #8620774 - Attachment is obsolete: true
Attachment #8620775 - Attachment is obsolete: true
Attachment #8620776 - Attachment is obsolete: true
Attachment #8620777 - Attachment is obsolete: true
Attachment #8620778 - Attachment is obsolete: true
Ufff, first time using mozreview. I am not sure why my patches were obsoleted automatically, I'll ask for review again. Sorry if you are getting lots of notifications.
Comment on attachment 8620764 [details]
MozReview Request: Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r?echen

Edgar, may I have your review on this? Thanks.
Attachment #8620764 - Attachment is obsolete: false
Attachment #8620764 - Flags: review?(echen)
Attachment #8620766 - Attachment is obsolete: false
Attachment #8620766 - Flags: review?(echen)
Comment on attachment 8620764 [details]
MozReview Request: Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r?echen

Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r?echen
Attachment #8620764 - Attachment description: MozReview Request: Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r=echen → MozReview Request: Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r?echen
Comment on attachment 8620766 [details]
MozReview Request: Bug 1167132 - Part 2: [NetworkManager] move network information into a separate interface (NetworkManager)

Bug 1167132 - Part 2: [NetworkManager] move network information into a separate interface (NetworkManager)
Attachment #8620767 - Attachment is obsolete: false
Attachment #8620767 - Flags: review?(hchang)
Comment on attachment 8620767 [details]
MozReview Request: Bug 1167132 - Part 3: [NetworkManager] move network information into a separate interface (Wifi)

Bug 1167132 - Part 3: [NetworkManager] move network information into a separate interface (Wifi)
Attachment #8620768 - Attachment is obsolete: false
Attachment #8620768 - Flags: review?(echen)
Comment on attachment 8620768 [details]
MozReview Request: Bug 1167132 - Part 4: [NetworkManager] move network information into a separate interface (DataCall)

Bug 1167132 - Part 4: [NetworkManager] move network information into a separate interface (DataCall)
Comment on attachment 8620769 [details]
MozReview Request: Bug 1167132 - Part 5: [NetworkManager] move network information into a separate interface (Tethering)

Bug 1167132 - Part 5: [NetworkManager] move network information into a separate interface (Tethering)
Attachment #8620769 - Attachment is obsolete: false
Comment on attachment 8620770 [details]
MozReview Request: Bug 1167132 - Part 6: [NetworkManager] move network information into a separate interface (RadioInterface)

Bug 1167132 - Part 6: [NetworkManager] move network information into a separate interface (RadioInterface)
Attachment #8620770 - Attachment is obsolete: false
Comment on attachment 8620771 [details]
MozReview Request: Bug 1167132 - Part 7: [NetworkManager] move network information into a separate interface (MobileConnectionService)

Bug 1167132 - Part 7: [NetworkManager] move network information into a separate interface (MobileConnectionService)
Attachment #8620771 - Attachment is obsolete: false
Comment on attachment 8620772 [details]
MozReview Request: Bug 1167132 - Part 8: [NetworkManager] move network information into a separate interface (MmsService)

Bug 1167132 - Part 8: [NetworkManager] move network information into a separate interface (MmsService)
Attachment #8620772 - Attachment is obsolete: false
Comment on attachment 8620773 [details]
MozReview Request: Bug 1167132 - Part 9: [NetworkManager] move network information into a separate interface (shell)

Bug 1167132 - Part 9: [NetworkManager] move network information into a separate interface (shell)
Attachment #8620773 - Attachment is obsolete: false
Comment on attachment 8620774 [details]
MozReview Request: Bug 1167132 - Part 10: [NetworkManager] move network information into a separate interface (PushService)

Bug 1167132 - Part 10: [NetworkManager] move network information into a separate interface (PushService)
Attachment #8620774 - Attachment is obsolete: false
Comment on attachment 8620775 [details]
MozReview Request: Bug 1167132 - Part 11: [NetworkManager] move network information into a separate interface (Gps)

Bug 1167132 - Part 11: [NetworkManager] move network information into a separate interface (Gps)
Attachment #8620775 - Attachment is obsolete: false
Attachment #8620776 - Attachment is obsolete: false
Comment on attachment 8620776 [details]
MozReview Request: Bug 1167132 - Part 12: [NetworkManager] move network information into a separate interface (NetStatistics)

Bug 1167132 - Part 12: [NetworkManager] move network information into a separate interface (NetStatistics)
Comment on attachment 8620777 [details]
MozReview Request: Bug 1167132 - Part 13: [NetworkManager] move network information into a separate interface (discovery)

Bug 1167132 - Part 13: [NetworkManager] move network information into a separate interface (discovery)
Attachment #8620777 - Attachment is obsolete: false
Comment on attachment 8620778 [details]
MozReview Request: Bug 1167132 - Part 14: [NetworkManager] move network information into a separate interface (NetworkStats)

Bug 1167132 - Part 14: [NetworkManager] move network information into a separate interface (NetworkStats)
Attachment #8620778 - Attachment is obsolete: false
Comment on attachment 8620779 [details]
MozReview Request: Bug 1167132 - Part 15: [NetworkManager] move network information into a separate interface (Mtransport)

Bug 1167132 - Part 15: [NetworkManager] move network information into a separate interface (Mtransport)
Attachment #8620764 - Flags: review?(echen) → review+
Comment on attachment 8620764 [details]
MozReview Request: Bug 1167132 - Part 1: [NetworkManager] move network information into a separate interface (idl). r?echen

https://reviewboard.mozilla.org/r/10827/#review9825

Looks good.
Comment on attachment 8620768 [details]
MozReview Request: Bug 1167132 - Part 4: [NetworkManager] move network information into a separate interface (DataCall)

https://reviewboard.mozilla.org/r/10833/#review10819

Please see my comments, thank you.

::: dom/system/gonk/DataCallManager.js:1450
(Diff revision 2)
> -    let reason = Ci.nsINetworkInterface.DATACALL_DEACTIVATE_NO_REASON;
> +    let reason = Ci.nsIDataCallInterface.DATACALL_DEACTIVATE_NO_REASON;

Nice catch!!

::: dom/system/gonk/DataCallManager.js:1659
(Diff revision 2)
>      return this.state == NETWORK_STATE_CONNECTED;

```this.state == NETWORK_STATE_CONNECTED;```

::: dom/system/gonk/DataCallManager.js:841
(Diff revision 2)
> -      let dataCall = networkInterface.dataCall;
> +      let dataCall = networkInterface.info.dataCall;

Should this be?

```
let dataCall = networkInterface.dataCall;
```
Attachment #8620768 - Flags: review?(echen)
Comment on attachment 8620767 [details]
MozReview Request: Bug 1167132 - Part 3: [NetworkManager] move network information into a separate interface (Wifi)

Looks good to me! Sorry for late review :p
Attachment #8620767 - Flags: review?(hchang) → review+
Comment on attachment 8620766 [details]
MozReview Request: Bug 1167132 - Part 2: [NetworkManager] move network information into a separate interface (NetworkManager)

https://reviewboard.mozilla.org/r/10829/#review10053

Mostly looks good, just few comments. And I would like to review it again since there are a lot of changes in this part. :)

::: dom/system/gonk/NetworkManager.js:262
(Diff revision 2)
> -  getNetworkId: function(network) {
> +  getNetworkId: function(networkInfo) {

s/networkInfo/aNetworkInfo/

::: dom/system/gonk/NetworkManager.js:553
(Diff revision 2)
> -  isValidatedNetwork: function(network) {
> +  isValidatedNetwork: function(networkInfo) {

s/networkInfo/aNetworkInfo/

::: dom/system/gonk/NetworkManager.js:891
(Diff revision 2)
> -  convertConnectionType: function(network) {
> +  convertConnectionType: function(aNetwork) {

s/aNetwork/aNetworkInfo/

::: dom/system/gonk/NetworkManager.js:793
(Diff revision 2)
>      this.active = null;

`active` now caches the networkInfo of active network, how about having a clearer naming, maybe `activeNetworkInfo`?
Attachment #8620766 - Flags: review?(echen)
https://reviewboard.mozilla.org/r/10829/#review10053

> `active` now caches the networkInfo of active network, how about having a clearer naming, maybe `activeNetworkInfo`?

`activeNetworkInfo` will definitely be clearer, I was just worried we would be changing too much other module's code since this is exposed in the interface. But I agree with you. :)
Attached patch Part 1: interface changes, v4. (obsolete) — Splinter Review
Hi Edgar, I have changed 'active' to 'activeNetworkInfo', may I have you review again? Thanks.
Attachment #8610418 - Attachment is obsolete: true
Attachment #8620764 - Attachment is obsolete: true
Attachment #8630852 - Flags: review?(echen)
Addressed review comment 46.
Attachment #8620766 - Attachment is obsolete: true
Attachment #8630911 - Flags: review?(echen)
Attachment #8620767 - Attachment is obsolete: true
Attachment #8630913 - Flags: review+
Attached patch Part 4: data call changes, v2. (obsolete) — Splinter Review
Addressed review comment 44.

Edgar, may I have your review again? Thanks.
Attachment #8620768 - Attachment is obsolete: true
Attachment #8630915 - Flags: review?(echen)
Attached patch Part 5: tethering changes, v1. (obsolete) — Splinter Review
Attachment #8620769 - Attachment is obsolete: true
Attachment #8630916 - Flags: review?(echen)
Attachment #8620770 - Attachment is obsolete: true
Attachment #8630917 - Flags: review?(echen)
Attachment #8620771 - Attachment is obsolete: true
Attachment #8630918 - Flags: review?(echen)
Attachment #8630852 - Flags: review?(echen) → review+
Comment on attachment 8630911 [details] [diff] [review]
Part 2: NetworkManager changes, v2.

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

Nice, thank you.

::: dom/system/gonk/NetworkManager.js
@@ -492,5 @@
>      }
>      this._preferredNetworkType = val;
>    },
>  
> -  active: null,

Nit: Put `_activeNetwork: null,` here (follow same style as _preferredNetworkType).
Attachment #8630911 - Flags: review?(echen) → review+
Comment on attachment 8630915 [details] [diff] [review]
Part 4: data call changes, v2.

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

::: dom/system/gonk/DataCallManager.js
@@ +415,5 @@
>      this.dataNetworkInterfaces.clear();
>      this._dataCalls = [];
>      this.clientId = null;
>  
> +    this.dataCallInterface.unregisterListener(this);

Nice catch!!! :)
Attachment #8630915 - Flags: review?(echen) → review+
Comment on attachment 8630916 [details] [diff] [review]
Part 5: tethering changes, v1.

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

::: dom/system/gonk/TetheringService.js
@@ +631,5 @@
>        return;
>      }
>  
> +    this._tetheringInterface[TETHERING_TYPE_WIFI].internalInterface =
> +      aNetwork.info.name;

It seems that `setWifiTethering` needs interface name only. Maybe we don't have to take nsINetworkInterface as argument [1], unless we might need more information from nsINetworkInterface in the future. Just share my thought here, we could do further discussion for this.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsITetheringService.idl#13-29
Attachment #8630916 - Flags: review?(echen) → review+
Comment on attachment 8630917 [details] [diff] [review]
Part 6: RadioInterface changes, v1.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -80,5 @@
> -const NETWORK_TYPE_MOBILE      = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
> -const NETWORK_TYPE_MOBILE_MMS  = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> -const NETWORK_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> -const NETWORK_TYPE_MOBILE_IMS  = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_IMS;
> -const NETWORK_TYPE_MOBILE_DUN  = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN;

NETWORK_TYPE_MOBILE_* are unused consts, it seem we could remove them.
Attachment #8630917 - Flags: review?(echen) → review+
Comment on attachment 8630918 [details] [diff] [review]
Part 7: MobileConnection changes, v1.

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +1606,1 @@
>              this.notifyDataError(rilNetwork.serviceId, rilNetwork);

Ah, I found a flaw existing in the code base for a while: the second argument of notifyDataError should be error message [1], not NetworkInterface or NetworkInfo.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/gonk/MobileConnectionService.js?from=MobileConnectionService.js#1378-1385
Attachment #8630918 - Flags: review?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #59)
> Comment on attachment 8630918 [details] [diff] [review]
> Part 7: MobileConnection changes, v1.
> 
> Review of attachment 8630918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/gonk/MobileConnectionService.js
> @@ +1606,1 @@
> >              this.notifyDataError(rilNetwork.serviceId, rilNetwork);
> 
> Ah, I found a flaw existing in the code base for a while: the second
> argument of notifyDataError should be error message [1], not
> NetworkInterface or NetworkInfo.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/gonk/
> MobileConnectionService.js?from=MobileConnectionService.js#1378-1385

Right! thanks for catching this, will fix it in the next version.
(In reply to Edgar Chen [:edgar][:echen] from comment #57)
> Comment on attachment 8630916 [details] [diff] [review]
> Part 5: tethering changes, v1.
> 
> Review of attachment 8630916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/TetheringService.js
> @@ +631,5 @@
> >        return;
> >      }
> >  
> > +    this._tetheringInterface[TETHERING_TYPE_WIFI].internalInterface =
> > +      aNetwork.info.name;
> 
> It seems that `setWifiTethering` needs interface name only. Maybe we don't
> have to take nsINetworkInterface as argument [1], unless we might need more
> information from nsINetworkInterface in the future. Just share my thought
> here, we could do further discussion for this.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> nsITetheringService.idl#13-29

I agree, passing network interface name is enough. It's a good opportunity to fix this, will upload a new patch revising this. Thanks.
Addressed nit in review comment 55.
Attachment #8630911 - Attachment is obsolete: true
Attachment #8637615 - Flags: review+
Attached patch Part 4: data call changes, v3. (obsolete) — Splinter Review
Some more fixes in DataCallManager.js
Attachment #8630915 - Attachment is obsolete: true
Attachment #8637616 - Flags: review+
Attached patch Part 5: tethering changes, v2. (obsolete) — Splinter Review
Use interfaceName as parameter (instead of network interface) for setWifiTethering().
Attachment #8630916 - Attachment is obsolete: true
Attachment #8637617 - Flags: review?(echen)
Removed unused consts, as mentioned in comment 58.
Attachment #8630917 - Attachment is obsolete: true
Attachment #8637618 - Flags: review+
Pass error message when calling notifyDataError().
Attachment #8630918 - Attachment is obsolete: true
Attachment #8637620 - Flags: review?(echen)
Comment on attachment 8637620 [details] [diff] [review]
Part 7: MobileConnection changes. r=echen

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

::: dom/system/gonk/DataCallManager.js
@@ +840,5 @@
>      if (networkInterface && networkInterface.enabled) {
>        let dataCall = networkInterface.dataCall;
> +      if (this._compareDataCallOptions(dataCall, aDataCall)) {
> +        Services.obs.notifyObservers(networkInterface.info,
> +                                     TOPIC_DATA_CALL_ERROR, aErrorMsg);

I removed the part where we compare cid, cause this is called only on data call setup error.
Attached patch Part 8: mms changes, v1. (obsolete) — Splinter Review
Bevis, we are exposing nsINetworkInfo instead of nsINetworkInterface now. I have made the changes in mms, would you mind reviewing this? Thanks.
Attachment #8620772 - Attachment is obsolete: true
Attachment #8637627 - Flags: review?(btseng)
Attached patch Part 9: shell changes, v1. (obsolete) — Splinter Review
Fabrice, we are exposing nsINetworkInfo instead of nsINetworkInterface in 'network-connection-state-changed' events. I've made the corresponding changes in shell.js, would you mind review this? Thanks.
Attachment #8620773 - Attachment is obsolete: true
Attachment #8637632 - Flags: review?(fabrice)
Attached patch Part 10: push changes, v1. (obsolete) — Splinter Review
Hi :nsm, we are exposing nsINetworkInfo (.activeNetworkInfo) instead of nsINetworkInterface (.active) in NetworkManager. I've made the corresponding changes in Push modules, would you mind reviewing this? Thanks.
Attachment #8620774 - Attachment is obsolete: true
Attachment #8637637 - Flags: review?(nsm.nikhil)
Attached patch Part 11: gps changes, v1. (obsolete) — Splinter Review
Hi Kanru, we are exposing nsINetworkInfo instead of nsINetworkInterface in NetworkManager. I've made the corresponding changes in gps, would you mind review this? Thanks.
Attachment #8620775 - Attachment is obsolete: true
Attachment #8637646 - Flags: review?(kchen)
Comment on attachment 8637615 [details] [diff] [review]
Part 2: NetworkManager changes, v3.

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

::: dom/system/gonk/NetworkManager.js
@@ +573,3 @@
>      }
>  
> +    return this.resolveHostname(networkInfo, host)

nit: The signature of resolveHostname is still unchanged and take "network" as 1st parameter. :p
Comment on attachment 8637627 [details] [diff] [review]
Part 8: mms changes, v1.

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

r=me after the inlined comment addressed.

Thanks!

::: dom/mobilemessage/gonk/MmsService.js
@@ +522,5 @@
>            return;
>          }
>  
>          // Check if the network state change belongs to this service.
> +        let network = subject.QueryInterface(Ci.nsIRilNetworkInfo);

nit: please replace "network" as "networkInfo" as well for better understanding.
Attachment #8637627 - Flags: review?(btseng) → review+
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #72)
> Comment on attachment 8637615 [details] [diff] [review]
> Part 2: NetworkManager changes, v3.
> 
> Review of attachment 8637615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +573,3 @@
> >      }
> >  
> > +    return this.resolveHostname(networkInfo, host)
> 
> nit: The signature of resolveHostname is still unchanged and take "network"
> as 1st parameter. :p

Will do!

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #73)
> Comment on attachment 8637627 [details] [diff] [review]
> Part 8: mms changes, v1.
> 
> Review of attachment 8637627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me after the inlined comment addressed.
> 
> Thanks!
> 
> ::: dom/mobilemessage/gonk/MmsService.js
> @@ +522,5 @@
> >            return;
> >          }
> >  
> >          // Check if the network state change belongs to this service.
> > +        let network = subject.QueryInterface(Ci.nsIRilNetworkInfo);
> 
> nit: please replace "network" as "networkInfo" as well for better
> understanding.

Will do. Thanks Bevis!
Attached patch Part 12: discovery changes, v1. (obsolete) — Splinter Review
Hi jryans, we are exposing nsINetworkInfo instead of nsINetworkInterface in NetworkManager. I've made the corresponding changes in discovery, would you mind review this? Thanks.
Attachment #8620776 - Attachment is obsolete: true
Attachment #8637731 - Flags: review?(jryans)
Hi Ethan, we are exposing nsINetworkInfo instead of nsINetworkInterface in NetworkManager. I've made the corresponding changes in necko/netstats, would you mind review this? Thanks.
Attachment #8620777 - Attachment is obsolete: true
Attachment #8637733 - Flags: review?(ettseng)
Attached patch Part 14: netstats changes, v1. (obsolete) — Splinter Review
Ethan, would mind reviewing this too? Thanks.
Attachment #8620778 - Attachment is obsolete: true
Attachment #8637734 - Flags: review?(ettseng)
Edgar, here are the changes for NetworkInterfaceListService, would you mind reviewing this? Thanks.
Attachment #8620779 - Attachment is obsolete: true
Attachment #8637735 - Flags: review?(echen)
Comment on attachment 8637646 [details] [diff] [review]
Part 11: gps changes, v1.

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

LGTM
Attachment #8637646 - Flags: review?(kchen) → review+
Comment on attachment 8637617 [details] [diff] [review]
Part 5: tethering changes, v2.

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

r=me with comment addressed, thank you.

::: dom/system/gonk/nsITetheringService.idl
@@ +16,5 @@
>     * @param enabled
>     *        Boolean that indicates whether tethering should be enabled (true) or
>     *        disabled (false).
> +   * @param interfaceName
> +   *        The Wifi network interface name for external interface.

s/external/internal/
Attachment #8637617 - Flags: review?(echen) → review+
Attachment #8637620 - Flags: review?(echen) → review+
Comment on attachment 8637733 [details] [diff] [review]
Part 13: necko/netstats changes, v1.

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

This patch simply replaces nsINetworkInterface by nsINetworkInfo. It looks pretty good.
Thanks Jessica! r=me. :)
Attachment #8637733 - Flags: review?(ettseng) → review+
Comment on attachment 8637734 [details] [diff] [review]
Part 14: netstats changes, v1.

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

This patch also looks perfect to me! r=me.
Attachment #8637734 - Flags: review?(ettseng) → review+
it's within the 2.5 scope - As a user, I want to have a robust control in data connection.
feature-b2g: --- → 2.5+
Attachment #8637735 - Flags: review?(echen) → review+
Attachment #8637632 - Flags: review?(fabrice) → review+
rebased.
Attachment #8630852 - Attachment is obsolete: true
Attachment #8640367 - Flags: review+
Rebased and renamed the first parameter of resolveHostname to aNetworkInfo.
Attachment #8637615 - Attachment is obsolete: true
Attachment #8640369 - Flags: review+
rebased.
Attachment #8637616 - Attachment is obsolete: true
Attachment #8640372 - Flags: review+
Correct comment in nsITetheringService.idl as comment 80.
Attachment #8637617 - Attachment is obsolete: true
Attachment #8640373 - Flags: review+
rebased.
Attachment #8637618 - Attachment is obsolete: true
Attachment #8640375 - Flags: review+
Attachment #8637620 - Attachment description: Part 7: MobileConnection changes, v2. → Part 7: MobileConnection changes. r=echen
Use variable name "networkInfo" instead of "network" for better understanding.
Attachment #8637627 - Attachment is obsolete: true
Attachment #8640376 - Flags: review+
Add r=fabrice.
Attachment #8637632 - Attachment is obsolete: true
Attachment #8640378 - Flags: review+
Add r=nsm.
Attachment #8637637 - Attachment is obsolete: true
Attachment #8640379 - Flags: review+
Add r=kanru.
Attachment #8637646 - Attachment is obsolete: true
Attachment #8640380 - Flags: review+
Add r=jryans.
Attachment #8637731 - Attachment is obsolete: true
Attachment #8640381 - Flags: review+
Add r=ethan.
Attachment #8637733 - Attachment is obsolete: true
Attachment #8640383 - Flags: review+
Add r=ethan.
Attachment #8637734 - Attachment is obsolete: true
Attachment #8640384 - Flags: review+
Rebased and add r=echen.
Attachment #8637735 - Attachment is obsolete: true
Attachment #8640385 - Flags: review+
Wow, you got r+ for all parts of patches! Well done!
Thanks to all the reviewers! :)

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=202656ad4802
Keywords: checkin-needed
Status: NEW → ASSIGNED
Depends on: 1197667
You need to log in before you can comment on or make changes to this bug.