717122 - B2G network manager component

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

We need a component that sets up routes, DNS, etc. and manages switch over from 3G to WiFi and back and all that stuff.

I imagine this will sit between 3G (bug 710493) and WiFi (bug 712629) and the Network API (bug 713199) / Network Manager API (bug 697355)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: thinker's proposed Network Manager API

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: thinker's proposed interface for NIC backends

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 588284 [details]
thinker's proposed Network Manager API

Great work, thinker! Some comments/questions below.

>  /**
>   * Current network state. Must be one of the NETWORK_STATE_* constants.
>   */
>  attribute long currentState;

Could just be `state`.

>  /**
>   * Is a default data connection?. If so, notice it should not coexist
>   * (simultaneity not allowed) with other default data connections.
>   */
>  attribute bool defaultDataConnection;
>
>  /**
>   * List of system property names containing DNS server addresses for this
>   * network connection.
>   */
>  attribute nsIArray dnsPropName;

What are system property names? Can't this just be a list of strings containing the IP addresses?

>  attribute long defaultGatewayAddr;

Why do we have to expose DNS and gateway? I would imagine the the network manager just does this transparently. Which other component would be interested in this information? The UI maybe? Seems overkill to me. What do others think?

>  /**
>   * Network type (e.g., WIFI). Must be one of NETWORK_TYPE_* contanst.
>   */
>  attribute long networkType;
>
>  attribute string interfaceName;

s/string/DOMString/

>
>  attribute nsINetworkState networkState;

Why not inline the information from nsINetworkState here?

>[scriptable, uuid(f13e5f84-3092-11e1-a06d-0010183a41af)]
>interface nsINetworkManagerCallback : nsISupports
>{
>  void onactivenetworkchange(in jsval event);
>};

Why don't we make this about all status changes? Let's call it `onnetworkstatuschange` or something and we'll notify it every time some property of a network changes.

Also, we can make this a [function] interface so that JS can just use a function here.

I also feel like we should expand on what information `event` contains. I think a description of the event type (what changed) and the network info object (which contains all necessary information) would be appropriate. Could just pass them as parameters.

To sum up:

  [scriptable, function, uuid(f13e5f84-3092-11e1-a06d-0010183a41af)]
  interface nsINetworkManagerCallback : nsISupports
  {
    void onnetworkstatuschange(in short event_type, in nsINetworkInfo info);
  };

>[scriptable, uuid(3bc29392-2fba-11e1-80fd-0010183a41af)]
>interface nsINetworkManager : nsISupports
>{
>  //XXX Is this correct?
>  const long DEFAULT_NETWORK_PREFERENCE = 0;

What does this even mean?

>  /**
>   * Prefered network type.
>   */
>  attribute long preferedNetworkType;

Typo: preferred

>  /**
>   * Mobile data enabled?
>   */
>  attribute boolean mobileDataEnabled;

Is this a short-cut for enabling/disabling all non-WiFi connections?

>  /**
>   * Returns Array of nsINetworkInfo for the active (i.e., connected)
>   * network interfaces.
>   */
>  nsIArray getActiveNetworkInfo();
>
>  nsIArray getAllNetworkInfo();

Not sure it's worth it distinguishing between active and non-active. Easy enough to filter on the consumer side.

>  /**
>   * Activate a network interface.
>   *
>   * @param networkType
>   *        Type of the network for activating.
>   *
>   * XXX: Wouldn´t it be better to activate it by using the network interface
>   *      name, as it is the only unique parameter. It is possible to have more
>   *      than one network with the same type (i.e. usb 3G dongle)

I agree with this!

>  /**
>   * Sets a network route to deliver traffic to the specified host via the
>   * specified network interface.
>   *
>   * @param networkType
>   *        The type of the network over which traffic to the specified host
>   *        is to be routed.
>   *
>   * @param hostAddress
>   *        The IP address of the host to which the route is desired.
>   *
>   * @return true on success, false on failure.
>   */
>  boolean requestRouteToHost(in long networkType,
>			     in unsigned long hostAddress);

Same question as above for DNS and gateway: which consumers would be interested in this functionality? This seems like something internal to the network manager component.

Comment 4

[Thinker Li [:sinker]]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> Comment on attachment 588284 [details]
> thinker's proposed Network Manager API
> 
> Great work, thinker! Some comments/questions below.

This proposal was initialized by jaoo.  I make some changes.
So, something should be explained by jaoo.

> >  /**
> >   * Is a default data connection?. If so, notice it should not coexist
> >   * (simultaneity not allowed) with other default data connections.
> >   */
> >  attribute bool defaultDataConnection;
> >
> >  /**
> >   * List of system property names containing DNS server addresses for this
> >   * network connection.
> >   */
> >  attribute nsIArray dnsPropName;
> 
> What are system property names? Can't this just be a list of strings
> containing the IP addresses?

Yes.. I agree with you.  I had mentioned the same opinion at Bug 713199.  I think jaoo put property names here because RIL of Android before v3.0 puts IP addresses in system properties and return names of the properties.  But, I find it from ril.h that RIL return IP addresses directly after Android 3.0.  I think it is responsibility of backends to translate property names to IP addresses.

> 
> >  attribute long defaultGatewayAddr;
> 
> Why do we have to expose DNS and gateway? I would imagine the the network
> manager just does this transparently. Which other component would be
> interested in this information? The UI maybe? Seems overkill to me. What do
> others think?

For VPN?  Some applications may like to know if their traffic are routed properly, for example, through an VPN interface.


> 
> >[scriptable, uuid(3bc29392-2fba-11e1-80fd-0010183a41af)]
> >interface nsINetworkManager : nsISupports
> >{
> >  //XXX Is this correct?
> >  const long DEFAULT_NETWORK_PREFERENCE = 0;
> 
> What does this even mean?

This constant is an alias of nsINetworkInfo.NETWORK_TYPE_WIFI.  Carrier may like to set it to NETWORK_TYPE_MOBILE :)  (see also https://bugzilla.mozilla.org/show_bug.cgi?id=713199#c4 )

> 
> >  /**
> >   * Sets a network route to deliver traffic to the specified host via the
> >   * specified network interface.
> >   *
> >   * @param networkType
> >   *        The type of the network over which traffic to the specified host
> >   *        is to be routed.
> >   *
> >   * @param hostAddress
> >   *        The IP address of the host to which the route is desired.
> >   *
> >   * @return true on success, false on failure.
> >   */
> >  boolean requestRouteToHost(in long networkType,
> >			     in unsigned long hostAddress);
> 
> Same question as above for DNS and gateway: which consumers would be
> interested in this functionality? This seems like something internal to the
> network manager component.

When multiple links are activated, some application need to specify route for a host.  For example, we may set the default route to an VPN interface, but we need to setup a route for VPN gateway to send their traffic through WIFI or 3G.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Thinker Li from comment #4)
> (In reply to Philipp von Weitershausen [:philikon] from comment #3)
> > Comment on attachment 588284 [details]
> > thinker's proposed Network Manager API
> > 
> > Great work, thinker! Some comments/questions below.
> 
> This proposal was initialized by jaoo.  I make some changes.
> So, something should be explained by jaoo.
> 

Feel free to make any changes. I am a beginner.  

> > >  /**
> > >   * Is a default data connection?. If so, notice it should not coexist
> > >   * (simultaneity not allowed) with other default data connections.
> > >   */
> > >  attribute bool defaultDataConnection;
> > >
> > >  /**
> > >   * List of system property names containing DNS server addresses for this
> > >   * network connection.
> > >   */
> > >  attribute nsIArray dnsPropName;
> > 
> > What are system property names? Can't this just be a list of strings
> > containing the IP addresses?
> 
> Yes.. I agree with you.  I had mentioned the same opinion at Bug 713199.  I
> think jaoo put property names here because RIL of Android before v3.0 puts
> IP addresses in system properties and return names of the properties.  But,
> I find it from ril.h that RIL return IP addresses directly after Android
> 3.0.  I think it is responsibility of backends to translate property names
> to IP addresses.
> 

Fully agree. Thinker noticed that. I defined an array of property names due how RIL manages DNS server addresses in Android 2.3.5. 

> > 
> > >  attribute long defaultGatewayAddr;
> > 
> > Why do we have to expose DNS and gateway? I would imagine the the network
> > manager just does this transparently. Which other component would be
> > interested in this information? The UI maybe? Seems overkill to me. What do
> > others think?
> 
> For VPN?  Some applications may like to know if their traffic are routed
> properly, for example, through an VPN interface.
> 
> 
> > 
> > >[scriptable, uuid(3bc29392-2fba-11e1-80fd-0010183a41af)]
> > >interface nsINetworkManager : nsISupports
> > >{
> > >  //XXX Is this correct?
> > >  const long DEFAULT_NETWORK_PREFERENCE = 0;
> > 
> > What does this even mean?
> 
> This constant is an alias of nsINetworkInfo.NETWORK_TYPE_WIFI.  Carrier may
> like to set it to NETWORK_TYPE_MOBILE :)  (see also
> https://bugzilla.mozilla.org/show_bug.cgi?id=713199#c4 )
> 

Yes, I work for a carrier ;) .

> > 
> > >  /**
> > >   * Sets a network route to deliver traffic to the specified host via the
> > >   * specified network interface.
> > >   *
> > >   * @param networkType
> > >   *        The type of the network over which traffic to the specified host
> > >   *        is to be routed.
> > >   *
> > >   * @param hostAddress
> > >   *        The IP address of the host to which the route is desired.
> > >   *
> > >   * @return true on success, false on failure.
> > >   */
> > >  boolean requestRouteToHost(in long networkType,
> > >			     in unsigned long hostAddress);
> > 
> > Same question as above for DNS and gateway: which consumers would be
> > interested in this functionality? This seems like something internal to the
> > network manager component.
> 
> When multiple links are activated, some application need to specify route
> for a host.  For example, we may set the default route to an VPN interface,
> but we need to setup a route for VPN gateway to send their traffic through
> WIFI or 3G.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Thinker Li from comment #4)
> > >  /**
> > >   * List of system property names containing DNS server addresses for this
> > >   * network connection.
> > >   */
> > >  attribute nsIArray dnsPropName;
> > 
> > What are system property names? Can't this just be a list of strings
> > containing the IP addresses?
> 
> Yes.. I agree with you.  I had mentioned the same opinion at Bug 713199.  I
> think jaoo put property names here because RIL of Android before v3.0 puts
> IP addresses in system properties and return names of the properties.  But,
> I find it from ril.h that RIL return IP addresses directly after Android
> 3.0.  I think it is responsibility of backends to translate property names
> to IP addresses.

Agreed.

> > >  attribute long defaultGatewayAddr;
> > 
> > Why do we have to expose DNS and gateway? I would imagine the the network
> > manager just does this transparently. Which other component would be
> > interested in this information? The UI maybe? Seems overkill to me. What do
> > others think?
> 
> For VPN?  Some applications may like to know if their traffic are routed
> properly, for example, through an VPN interface.

Hmm. Is VPN an actual use case? Do we expect web apps to be able to set up VPNs and speciali routes and all that? If there's a use case there, I feel like it's not one we care about today. So I'm inclined to defer this until later.

> > >  //XXX Is this correct?
> > >  const long DEFAULT_NETWORK_PREFERENCE = 0;
> > 
> > What does this even mean?
> 
> This constant is an alias of nsINetworkInfo.NETWORK_TYPE_WIFI.  Carrier may
> like to set it to NETWORK_TYPE_MOBILE :)  (see also
> https://bugzilla.mozilla.org/show_bug.cgi?id=713199#c4 )

Ok. It's still kind of unclear to me what settings/behaviour this value controls. Also, since it's a constant, how would carriers be able to change this value? By patching Gecko?

Comment 7

[Philipp von Weitershausen [:philikon]]

Thanks for your feedback, Thinker and jaoo. I will take a stab at the implementation. If you have any work-in-progress for this already, please upload it. Any kind of input is gladly appreciated. Thanks!

Comment 8

[Thinker Li [:sinker]]

I have made a prototype for network manager component.  We seems have to make all functions asynchronized, or callers would be blocked.

Comment 9

[Thinker Li [:sinker]]

(In reply to Thinker Li from comment #8)
> I have made a prototype for network manager component.  We seems have to
> make all functions asynchronized, or callers would be blocked.
I means some functions, for example: activateNetwork()/deactivateNetwork()...

Comment 10

[Thinker Li [:sinker]]

Attachment: Implement the network manager

Comment 11

[Thinker Li [:sinker]]

Attachment: IDL files for Network Manager

Comment 12

[Thinker Li [:sinker]]

Attachment: Testcases for network manager

Comment 13

[Thinker Li [:sinker]]

Attachment: script to run xpcshell-test

Comment 14

[Thinker Li [:sinker]]

(In reply to Thinker Li from comment #12)
> Created attachment 588722 [details] [diff] [review]
> Testcases for network manager

This is a testcase with xpcshell-test for network manager.  Currently, I run this testcase with a build of xulrunner and with following script.

(In reply to Thinker Li from comment #13)
> Created attachment 588723 [details]
> script to run xpcshell-test

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to Thinker Li from comment #8)
> I have made a prototype for network manager component.  We seems have to
> make all functions asynchronized, or callers would be blocked.

Correct. Everything needs to be non-blocking. My idea was that all methods return immediately and when their work was done, the appropriate event is fired via the `onnetworkstatuschange` callbacks.

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): interfaces

Here's a cut down version of the interfaces that Thinker and Jose proposed. A lot of information doesn't have to be exposed externally since it's either available through setprop/setprop or it's not of interest to external components.

To summarize quickly the way I envision the components to talk to each other: The RIL and WiFi subsystems each register an nsINetworkInterface. They know how to bring those up and down. When they do, they notify the network manager via observer notifications. The network manager than prepares the routes and DNS via getprop/setprop. It can also perform DHCP. (A lot of this is in the WiFi code right now, but it's not WiFi specific, so I'm working on separating that out.)

Comment 17

[Thinker Li [:sinker]]

Comment on attachment 592839 [details] [diff] [review]
Part 1 (v1): interfaces

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

I am a little confused.  Is this IDL for Gecko's internal use or an Web API?  For internal use, a lot of important functions seem to be removed.

::: dom/system/b2g/nsINetworkManager.idl
@@ +77,5 @@
> +  /**
> +   * Indicates whether DHCP should be run when the interface connects.
> +   */
> +  readonly attribute boolean dhcp;
> +

Since this interface is for Gecko's internal use, we need functions to control the power of network interfaces, enable/disable them, ...,etc.  Is there more interfaces for these purposes?

@@ +132,5 @@
> +   *
> +   * Consumers can observe changes to the active network by subscribing to
> +   * the 'network-active-changed' observer notification.
> +   */
> +  long overrideActive(in nsINetworkInterface network);

Since there is overrideActive, we need a function to stop overriding.

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to Thinker Li [:sinker] from comment #17)
> I am a little confused.  Is this IDL for Gecko's internal use or an Web API?

Gecko-internal use. Public Web API would be bug 697355.

> For internal use, a lot of important functions seem to be removed.

How so?

Maybe it will be apparent from the implementation patch that I will post later, once I have it in a somewhat presentable state.

> ::: dom/system/b2g/nsINetworkManager.idl
> @@ +77,5 @@
> > +  /**
> > +   * Indicates whether DHCP should be run when the interface connects.
> > +   */
> > +  readonly attribute boolean dhcp;
> > +
> 
> Since this interface is for Gecko's internal use, we need functions to
> control the power of network interfaces, enable/disable them, ...,etc.  Is
> there more interfaces for these purposes?

I thought long and hard about this, but it seems that the activation is pretty network device specific. I want to leave this up to each component (RIL, WiFi, etc.) for now and revisit later if we find common patterns.

> > +   *
> > +   * Consumers can observe changes to the active network by subscribing to
> > +   * the 'network-active-changed' observer notification.
> > +   */
> > +  long overrideActive(in nsINetworkInterface network);
> 
> Since there is overrideActive, we need a function to stop overriding.

I intended `overrideActive(null)` for this, but I forgot to document it... ooops! Thanks for catching that.

Comment 19

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (WIP 1): Implement network manager

This implements a minimal network manager that lets different network interfaces register themselves and know about state changes. It watches those state changes and then decides whether to divert all traffic to it by switching routes and DNS accordingly. It does this using libcutils/libnetutils via ctypes in a worker (these bits are cannibalized from the existing Wifi code.)

Comment 20

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (WIP 2): Implement network manager

Woah, somehow managed to grab an ancient version of the patch.

Comment 21

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (WIP 1): RIL network interface

This hooks up the RIL to the Network Manager. I found a bunch of problems with the existing 3G code in ril_worker.js, so that got fixed too.

Comment 22

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (WIP 3): Implement network manager

Progress & fixes.

Comment 23

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (WIP 2): RIL network interface

Bring up datacalls based on bug 725137 and bug 725901.

Comment 24

[Andreas Gal :gal]

Finally got around to check this out. Very nice work.

Comment 25

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (WIP 4): Implement network manager

Rebased after bug 725750 changed some WiFi code

Comment 26

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Implement network manager

Initial implementation of the network manager. The scope is intentionally kept small. It merely factors some of the network setup code out to a central component, and has a bit of logic to switch between networks. Wifi continues to work the same way with this patch applied.

(This likely to get some small conflicts with the patches in bug 727680, but they should be easily resolved; it can't hurt to get the review process going now.)

Comment 27

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595923 [details] [diff] [review]
Part 3 (WIP 2): RIL network interface

This is pretty much ready to go, except for the fact that on the SGS2 I can't get data calls to ever register themselves as ACTIVE_UP. They're always ACTIVE_DOWN for me, much like Thinker said in bug 725901 comment 2. I have yet to verify on another device, but the ones I have here have non-working RILs for now. Assuming it's a Samsung rild bug, I tried to treat ACTIVE_DOWN as ACTIVE_UP and configured the interface. I couldn't get a data connection going yet, but I bet that's just a small detail. I think we can start the review process already.

Also, the settings bits which is a bit ugly in preferences right now. I could replace them with sample settings and comment them out so that people could play with 3G, but maybe there's a better way? How far along is navigator.mozSettings? PLEASE ADVISE.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 592839 [details] [diff] [review]
Part 1 (v1): interfaces

>diff --git a/dom/system/b2g/nsINetworkManager.idl b/dom/system/b2g/nsINetworkManager.idl

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * ***** END LICENSE BLOCK ***** */
>+

New license header plz.

>+interface nsINetworkInterface : nsISupports
>+  const long NETWORK_STATE_SUSPENDED = 2;

What does this mean?  It doesn't seem to be used in these patches.
The other states are self-explanatory so happy as are, but please
either comment this one or remove it.

>+  const long NETWORK_TYPE_WIFI        = 0;
>+  const long NETWORK_TYPE_MOBILE      = 1;
>+  const long NETWORK_TYPE_MOBILE_MMS  = 2;
>+

I'm not sure this matters all that much yet, but what's a MOBILE
network?  And what's MOBILE_MMS?  Is the idea that they abstract over
particular technologies, so that { edge, 3g, 4g } \subset MOBILE?
Does MOBILE_MMS mean an mms-capable network?  If so, please document
:).

We might want more fine-grained control later but this is fine for
now.

>+/**
>+ * Manage network interfaces.
>+ */
>+[scriptable, uuid(3bc29392-2fba-11e1-80fd-0010183a41af)]
>+interface nsINetworkManager : nsISupports
>+{
>+  /**
>+   * Unregister the given network interface with the network manager.

"Register"

>+   *
>+   * Consumers will be notified with the 'network-interface-registered'
>+   * observer notification.
>+   */
>+  void registerNetworkInterface(in nsINetworkInterface network);
>+
>+  /**
>+   * Unregister the given network interface with the network manager.

"Unregister ... from the"

What happens if the interface isn't being managed?

Mostly doc and typos here, so r=me pending questions above.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 597905 [details] [diff] [review]
Part 2 (v1): Implement network manager

I'm not the guy to review JS/XPCOM stuff.  mrbkap is, deferring to
him.

There seems to be a higher-level issue with this patch: let's say I'm
connected to a wifi network, but set the preferred network type to 3g.
We'll connect to the 3g network, but how do we disconnect the wifi
network?  Continuing to chat with the AP is good for open sockets when
the switch is made, but wastes battery in the long run.  Maybe I'm
missing something.  But either way, we can do this in a followup.

>diff --git a/dom/system/b2g/NetworkManager.js b/dom/system/b2g/NetworkManager.js

This should be "gonk" since there's some gonk-specific stuff in here.
But that's not your fault, so can go to a followup.

>+NetworkManager.prototype = {

>+  registerNetworkInterface: function registerNetworkInterface(network) {

>+    if (network.name in this._networkInterfaces) {
>+      throw Components.Exception("Network with that name already registered!",
>+                                 Cr.NS_ERROR_INVALID_ARG);
>+    }

This seems like a reasonable contract but it needs to be documented in
the interface.

>+  unregisterNetworkInterface: function unregisterNetworkInterface(network) {
>+    if (!(network instanceof Ci.nsINetworkInterface)) {
>+      throw Components.Exception("Argument must be nsINetworkInterface.",
>+                                 Cr.NS_ERROR_INVALID_ARG);
>+    }

Please factor out this check into a helper.

>+    if (!(network.name in this._networkInterfaces)) {
>+      throw Components.Exception("No network with that name registered.",
>+                                 Cr.NS_ERROR_INVALID_ARG);
>+    }

Plz doc in interface.  You might also want to check that
|this._networkInterfaces[network.name] === network|.  Alternatively,
you could alter the interface to allow passing a string name to
remove, and check for either here.

>+  overrideActive: function overrideActive(network) {

Type check |network| here.  Use helper.

>diff --git a/dom/wifi/nsWifiWorker.js b/dom/wifi/nsWifiWorker.js

100% deferring to mrbkap on remaining stuff.

r=me with above addressed.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 595923 [details] [diff] [review]
Part 3 (WIP 2): RIL network interface

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js

>+// Data connection settings. These will eventually live in navigator.settings
>+// API, but for this will have to do.

This is fine.  Not even close to the ugliest proto-API-impl yet.

>+pref("ril.data.roaming.enabled", false);
>+pref("ril.data.apn", "wap.cingular");
>+pref("ril.data.user", "[e-mail]");
>+pref("ril.data.passwd", "CINGULAR1");

I wonder if it might be a bit friendlier to devs to make this
"ril.data.provider" with { att, tmobile ...} options, and then put the
details in gecko.  For now.  Friends may have an opinion.  I think
this would be easy enough to fix here.

>diff --git a/dom/system/b2g/RadioInterfaceLayer.js b/dom/system/b2g/RadioInterfaceLayer.js

>+  dataCallStateChanged: function dataCallStateChanged(cid, interfaceName, callState) {
>+    if (this.connecting &&
>+        callState == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTING) {
>+      this.connecting = false;

Is CONNECTING what you want here?  This code reads a bit oddly because
you unset this.connecting when state == CONNECTING.  AFAICT this is a
separate connecting state, but plz doc as such.  Maybe a different
name is clearer.  Deferring to Kyle here.

Rest looks OK, deferring to Kyle.

r=me with changes above

Comment 31

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595923 [details] [diff] [review]
Part 3 (WIP 2): RIL network interface

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

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +227,5 @@
> +  updateDataConnection: function updateDataConnection() {
> +    // GSM has a separate registration state for the 3G radio. If that's not
> +    // present, fall back to the main radio's one.
> +    let state = this.currentState.gprsRegistrationState ||
> +                this.currentState.registrationState;

Some further investigation has shown that it's not as simple as that. gprsRegistrationState may exist and have NETWORK_CREG_TECH_UNKNOWN while registrationState is NETWORK_CREG_STATE_REGISTERED_HOME and has NETWORK_CREG_TECH_EDGE.

Comment 32

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> >+interface nsINetworkInterface : nsISupports
> >+  const long NETWORK_STATE_SUSPENDED = 2;
> 
> What does this mean?  It doesn't seem to be used in these patches.

It's used in the RIL patch to match DATACALL_ACTIVE_DOWN. Not that I know what exactly that means...

> >+  const long NETWORK_TYPE_WIFI        = 0;
> >+  const long NETWORK_TYPE_MOBILE      = 1;
> >+  const long NETWORK_TYPE_MOBILE_MMS  = 2;
> >+
> 
> I'm not sure this matters all that much yet, but what's a MOBILE
> network?  And what's MOBILE_MMS?

I ... don't know. Thinker?

> Is the idea that they abstract over
> particular technologies, so that { edge, 3g, 4g } \subset MOBILE?

Yes.

Comment 33

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): interfaces

Addressed review nits.

Comment 34

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): Implement network manager

Rebased on top of bug 727680.

Comment 35

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (WIP 3): RIL network interface

Rebased.

Comment 36

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: patch to network manager

the patch is generated from b2g-network branch of philikon's github
1.switch default network to MOBILE
2.there might be some bugs in the network state

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

sorry but forgot my last quick patch
I think this issue depends on bug 725901
there is some bug in the rild on galaxy s2
it always return active=1 when REQUEST_DATA_CALL_LIST

I've tried two other HTC Android phones, they return active=2 

the way I test is to in Android Dialer App (not in B2G)
1.dial *#*#4636#*#*  (ps 4636 means 'info')
2.choose Phone information
3.press Menu
4. Enable Data Connection
5. Get PDP List
6. using adb logcat -b radio to see the output

Comment 38

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIPv1 (to applay on of existing patches)

This patch should apply cleanly on philikon's mozilla-central b2g-network branch. It makes some modifications to setup the data call and make the network manager being responsible for establishing the pdp0 interface as the default route and setting DNS configuration. I'll try to rebase these patches during this weekend and test much deeper. This is only valid for Samsung Galaxy S2. I also have another one for akami but I am not able to get the device connected because 'netmgrd' crashes apparently when establishing the connection. We will need help from mvines.

Comment 39

[José Antonio Olivera Ortega [:jaoo]]

I've rebased attached patches and I'm wondering if I could attach the new ones and mark as obsolete the existing ones.

Comment 40

[José Antonio Olivera Ortega [:jaoo]]

I forgot to mention that my code lives in https://github.com/jaoo/mozilla-central/tree/b2g-network.

Comment 41

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

verified jaoo's code, it can work well 
also check Android's implementation, it's not necessary to call REQUEST_DATA_CALL_LIST after SETUP_DATA_CALL to ensure the link is active or not

but in that case, the 'active' member of datacall in ril_worker.js and RadioInterfaceLayer.js will always be -1, even the link is actually connected

Comment 42

[Philipp von Weitershausen [:philikon]]

In order to make progress on 3G, I have moved the 3G-only parts of part 2 and 3 to bug 734299. I will rebase the rest of the Network Manager stuff next week, once Wifi has stabilized a bit.

Comment 43

[Kan-Ru Chen [:kanru] (UTC+9)]

Does this API allow us to connect to different APN? IIRC carriers can use different APN for different applications, internet, mms, supl, etc.

Comment 44

[Dan Williams]

(In reply to Kan-Ru Chen [:kanru] from comment #43)
> Does this API allow us to connect to different APN? IIRC carriers can use
> different APN for different applications, internet, mms, supl, etc.

You'd at least expect the ability to create two active PDP contexts, each using a different APN.  The first would be for an active datacall, the second for MMS.  Some modems only support one or two active contexts which depends entirely on the number of TTY or pseudo-ethernet interfaces they expose.  If they expose say two AT-capable TTYs, then they support at max 2 active PDP contexts (since each TTY would be used for PPP for a context).  In a phone that's pretty silly because you also need a TTY or some other channel for the RIL's control channel.  If the modem supports a pseudo-ethernet interface like rmnet or cdc-ether/cdc-ncm then that's your primary context channel and any secondary contexts must use PPP over available TTYs.  If the modem supports a more than one pseudo-ethernet interface and more than one TTY then you're probably golden for multiple contexts.

I'd expect any Qualcomm-based Android phone to support rmnet plus one or more TTYs for RIL control and possibly for secondary contexts.  Any ST-Ericsson phone would have good multi-context support using a combination of cdc-ncm/cdc-acm/cdc-wdm for AT, ether, and CAIF channels.

Most carriers do have a separate APN for MMS.  You can also create a separate PDP context for application-specific things that would require different levels of QoS.  That second context could use a different APN or it could use the same APN just with different QoS and a different IP address.

Comment 45

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #43)
> Does this API allow us to connect to different APN? IIRC carriers can use
> different APN for different applications, internet, mms, supl, etc.

The network manager is pretty oblivious to what the actual network connection is. It just unifies things like setting routes and DNS. It would be the job of RadioInterfaceLayer et.al. to bring up the MMS PDP connection (just like it brings up the 3G PDP connection already.)

I would love to start investigating (and implementing) MMS, but please file a separate bug (or bugs) for it. Thanks!

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hey guys, is there anything blocking this work?  It's high priority for M2.5.

Comment 47

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #46)
> Hey guys, is there anything blocking this work?  It's high priority for M2.5.

Nope. Patches have bitrotted, so they need to be rebased, but that's all.

Comment 48

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v3): Implement network manager

Network manager implementation, rebased on top bug 734299  whichalready landed the network worker code and much Wifi progress. The Wifi parts are also moved out to a separate patch (Part 4) now.

Comment 49

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v4): RIL network interface

Also rebased on top of bug 734299, making this patch pretty small now.

Comment 50

[Philipp von Weitershausen [:philikon]]

Attachment: Part 4 (v1): Wifi network interface

The most minimal solution for integrating Wifi with the Network Manager. We continue to do DHCP in the Wifi code for now and just use the Network Manager to set the routes. We can consolidate the duplication in a follow-up. Point is, we can now switch between 3G and Wifi seamlessly. Yay!

Comment 51

[Andreas Gal :gal]

Comment on attachment 613508 [details] [diff] [review]
Part 2 (v3): Implement network manager

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

::: dom/system/gonk/NetworkManager.js
@@ +22,5 @@
> +const TOPIC_INTERFACE_REGISTERED     = "network-interface-registered";
> +const TOPIC_INTERFACE_UNREGISTERED   = "network-interface-unregistered";
> +const TOPIC_DEFAULT_ROUTE_CHANGED    = "network-default-route-changed";
> +
> +const DEBUG = true;

do we want this enabled by default?

@@ +29,5 @@
> + * This component watches for network interfaces changing state and then
> + * adjusts routes etc. accordingly.
> + */
> +function NetworkManager() {
> +  this._networkInterfaces = {};

Doesn't seem very nice to maintain this twice. Probably causes extra cycle collector garbage.

Comment 52

[Andreas Gal :gal]

Comment on attachment 613510 [details] [diff] [review]
Part 4 (v1): Wifi network interface

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

::: dom/wifi/WifiWorker.js
@@ +1102,5 @@
>    return !/[^a-fA-F0-9]/.test(s);
>  }
>  
> +
> +let WifiNetworkInterface = {

This seesm to be duplicating RILNetworkInterface. Is it ok if the constants get out of whack?

Comment 53

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 613510 [details] [diff] [review]
Part 4 (v1): Wifi network interface

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

::: dom/wifi/WifiWorker.js
@@ +1331,5 @@
>          break;
>      }
>    };
>  
>    WifiManager.ondhcpconnected = function() {

We need to call this callback with this.info === null if DHCP failed for whatever reason. That also means that the notifyObservers call should be in the "this.info" branch of the if statement below.

Comment 54

[Philipp von Weitershausen [:philikon]]

(In reply to Andreas Gal :gal from comment #51)
> > +const DEBUG = true;
> 
> do we want this enabled by default?

No, I'll disable it for the landing. Thanks for catching that.

> > +function NetworkManager() {
> > +  this._networkInterfaces = {};
> 
> Doesn't seem very nice to maintain this twice. Probably causes extra cycle
> collector garbage.

Fair. I can just expose the this._networkInterfaces mapping as a jsval, rather than maintaining both the nsIArray and the private mapping.

(In reply to Andreas Gal :gal from comment #52)
> > +
> > +let WifiNetworkInterface = {
> 
> This seesm to be duplicating RILNetworkInterface. Is it ok if the constants
> get out of whack?

Well, it implements the interface, so it must also provide the constants. But instead of hardcoding the constants I can refer to nsINetworkInterface.<constantname> to make it less prone to get out of whack.

(In reply to Blake Kaplan (:mrbkap) from comment #53)
> >    WifiManager.ondhcpconnected = function() {
> 
> We need to call this callback with this.info === null if DHCP failed for
> whatever reason. That also means that the notifyObservers call should be in
> the "this.info" branch of the if statement below.

Ah yes, I see how I missed to translate the error case. Thanks for catching that, will fix.

Comment 55

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v3): interfaces

Incorporate gal's review comment (ditch nsIArray and just expose the simple mapping as a jsval)

Comment 56

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v4): Implement network manager

Incorporate gal's review comments.

Comment 57

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v5): RIL network interface

Incorporate gal's review comments

Comment 58

[Philipp von Weitershausen [:philikon]]

Attachment: Part 4 (v2): Wifi network interface

Incorporate mrbkap's comments.

Comment 59

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/637a250a8a70
https://hg.mozilla.org/integration/mozilla-inbound/rev/68962a3b0d30
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cfa8abfea14
https://hg.mozilla.org/integration/mozilla-inbound/rev/162bfe6dac96

Comment 60

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/637a250a8a70
https://hg.mozilla.org/mozilla-central/rev/68962a3b0d30
https://hg.mozilla.org/mozilla-central/rev/9cfa8abfea14
https://hg.mozilla.org/mozilla-central/rev/162bfe6dac96