776294 - B2G 3G: Configure APN settings through SettingsService.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

We were configuring the APN setting for data calls through Gecko's preferences so this bug is for changing current implementation.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v1

This patch depends on bug 740640.

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 644699 [details] [diff] [review]
WIP v1

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

I'm much in favour of this! Great job! See below about some improvements, though.

::: 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;

ahem.

(I highly recommend having a separate patch in your queue for debugging purposes. That way you never contaminate your "real" patches with that stuff.)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +625,5 @@
>        this.setRadioEnabled(false);
>      }
>    },
>  
> +  _ensureAPNSettings: function _ensureAPNSettings() {

"_ensureAPNSettings" is not an ideal name. It sounds like a value check rather than something that actually changes the state of the data connection. How about "updateRILNetworkInterface"?

@@ +1064,5 @@
> +  _rilDataUser: null,
> +  _rilDataPasswd: null,
> +  _rilDataHttpProxyHost: null,
> +  _rilDataHttpProxyPort: null,
> +  _oldRilDataEnabledState: null,

I would prefer if we did this in a more structured approach, e.g. have

  dataSettings: {},

and then do `this.dataSettings["disabled"]` etc. (Please always use double quotes for strings.). This will also make your gigantic `switch` statements a lot simpler and we can just pass `this.dataSettings` around to ril_worker.js and RILNetworkInterface.

Comment 3

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

Comment on attachment 644699 [details] [diff] [review]
WIP v1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1884,5 @@
>    get connected() {
>      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
>    },
>  
> +  connect: function connect(data) {

Hi, philikon and jaoo
How do you think if we change data to a more high level argument, said 'options', or 'apn' here?

So this function can be used by MMS and AGPS.
(see Bug 772747)

For example:

options: {
  type: "data', // or 'mms', 'supl'
  apn: ...,
  user, ...,
  passwd, ...,
}

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #3)
> Comment on attachment 644699 [details] [diff] [review]
> WIP v1

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1884,5 @@
> >    get connected() {
> >      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
> >    },
> >  
> > +  connect: function connect(data) {
> 
> Hi, philikon and jaoo
> How do you think if we change data to a more high level argument, said
> 'options', or 'apn' here?
> 
> So this function can be used by MMS and AGPS.
> (see Bug 772747)
> 
> For example:
> 
> options: {
>   type: "data', // or 'mms', 'supl'
>   apn: ...,
>   user, ...,
>   passwd, ...,
> }

That is already changed in the new version of the patch that I gonna attach today. The connect function will receive an object as Philipp suggested in comment #2. 

+  connect: function connect(data) {
     ...
+    if (data) {
+      // Save the APN data locally for using them in connection retries. 
+      this.dataCallSettings = data;
     }

     this.mRIL.setupDataCall(RIL.DATACALL_RADIOTECHNOLOGY_GSM,
-                            apn, user, passwd,
+                            this.dataCallSettings["ril.data.apn"], 
+                            this.dataCallSettings["ril.data.user"], 
+                            this.dataCallSettings["ril.data.passwd"],
                             RIL.DATACALL_AUTH_PAP_OR_CHAP, "IP");
     ...
   }

Does it look good? 

I guess you could get here `this.dataCallSettings["ril.data.apn.type"]` if the caller provides it.

Comment 5

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> +  connect: function connect(data) {
>      ...
> +    if (data) {
> +      // Save the APN data locally for using them in connection retries. 
> +      this.dataCallSettings = data;
>      }
> 
>      this.mRIL.setupDataCall(RIL.DATACALL_RADIOTECHNOLOGY_GSM,
> -                            apn, user, passwd,
> +                            this.dataCallSettings["ril.data.apn"], 
> +                            this.dataCallSettings["ril.data.user"], 
> +                            this.dataCallSettings["ril.data.passwd"],
>                              RIL.DATACALL_AUTH_PAP_OR_CHAP, "IP");
>      ...

Oh, maybe need philikon's opinion,
because MMS and AGPS may also use this function,(by using MMS APN and AGPS APN),
so I am thinking maybe we replace the word 'data' as it may suggest that this is a DataConnection.

Or maybe we create another RILNetworkInterfaces for MMS and APN ?

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v2

This patch addresses what Philipp suggested in comment #2.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> I guess you could get here `this.dataCallSettings["ril.data.apn.type"]` if
> the caller provides it.

That's not going to work. I agree with Yoshi here, and if you read my comment 2 carefully, you'll see that that's what I meant.

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 645753 [details] [diff] [review]
WIP v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +632,5 @@
> +    if (typeof this.dataCallSettings["ril.data.apn"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.user"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.passwd"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.roaming.enabled"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.enabled"] === 'undefined') {

No `typeof` needed. Just compare to `null`:

  if (this.dataCallSettings["apn"] == null || ...)

although I'm inclined to treat missing/null values for apn, user, passwd as empty strings. It often works with mobile providers.

@@ +639,5 @@
> +      return;
> +    }
> +    if (this._oldRilDataEnabledState == this.dataCallSettings["ril.data.enabled"]) {
> +      debug("No changes for ril.data.enabled flag. Nothing to do.");
> +      return;

That seems wrong. What if I change the APN name or the password? Shouldn't we reconnect?

@@ +959,5 @@
> +      case "ril.data.apn":
> +      case "ril.data.user":
> +      case "ril.data.passwd":
> +        debug(setting.key + " is now " + setting.value);
> +        this.dataCallSettings[setting.key] = setting.value;

Strip "ril.data." from the key.

@@ +1030,1 @@
>    handle: function handle(aName, aResult) {

Sigh. I wish that `handle()` and `handleMozSettingsChanged()` had the same signature so we could just use the same method here.

@@ +1035,5 @@
> +        this._ensureRadioState();
> +        break;
> +      case "ril.data.enabled":
> +      case "ril.data.roaming.enabled":
> +        this.dataCallSettings[aName] = aResult == null ? false : aResult;

Ditto.

@@ +1053,3 @@
>    handleError: function handleError(aErrorMessage) {
>      debug("There was an error reading the 'ril.radio.disabled' setting., " +
>            "default to radio on.");

You're going to have to update this message. We now deal with more than just the `ril.radio.disabled` setting.

@@ +1059,5 @@
> +    this.dataCallSettings["ril.data.enabled"] = false;
> +    this.dataCallSettings["ril.data.roaming.enabled"] = false;
> +    this.dataCallSettings["ril.data.apn"] = "";
> +    this.dataCallSettings["ril.data.user"] = "";
> +    this.dataCallSettings["ril.data.passwd"] = "";

Why not

  this.dataCallSettings = {};

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v3

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Comment on attachment 645753 [details] [diff] [review]
> WIP v2
> 
> Review of attachment 645753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +632,5 @@
> > +    if (typeof this.dataCallSettings["ril.data.apn"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.user"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.passwd"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.roaming.enabled"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.enabled"] === 'undefined') {
> 
> No `typeof` needed. Just compare to `null`:
> 
>   if (this.dataCallSettings["apn"] == null || ...)
> 
> although I'm inclined to treat missing/null values for apn, user, passwd as
> empty strings. It often works with mobile providers.

What I want to do here is to ensure that all the APN data have been read from the setting DB before setting the data call up at boot. I have changed a bit the way of checking it. Every time I read an APN setting (name, user, etc.) I save its key and I only set the data call up if I have read every key.
 
Now we treat missing null values for apn, user and passwd. The RIL worker receives the following message and the data call is set up properly as well. That works with Telefonica as you mentioned.

I/Gecko   (  105): RIL Worker: Received DOM message {"type":"setupDataCall","radioTech":1,"apn":null,"user":null,"passwd":null,"chappap":3,"pdptype":"IP"}

> @@ +639,5 @@
> > +      return;
> > +    }
> > +    if (this._oldRilDataEnabledState == this.dataCallSettings["ril.data.enabled"]) {
> > +      debug("No changes for ril.data.enabled flag. Nothing to do.");
> > +      return;
> 
> That seems wrong. What if I change the APN name or the password? Shouldn't
> we reconnect?

The logic here is the same that we did in the past. 

+    // We only watch at "ril.data.enabled" flag changes for connecting or
+    // disconnecting the data call. If the value of "ril.data.enabled" is
+    // true and any of the remaining flags change the setting application
+    // should turn this flag to false and then to true in order to reload
+    // the new values and reconnect the data call.

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #10)
> +    // We only watch at "ril.data.enabled" flag changes for connecting or
> +    // disconnecting the data call. If the value of "ril.data.enabled" is
> +    // true and any of the remaining flags change the setting application
> +    // should turn this flag to false and then to true in order to reload
> +    // the new values and reconnect the data call.

Ah I remember now... You're right, there was a good reason for doing it this way because it leaves the UI in control of restarting the data call. So we want to make sure that Gaia flips the flag after changing any of the other values. Can you file (or even fix) that, please?

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 646384 [details] [diff] [review]
WIP v3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +635,5 @@
>    },
>  
> +  updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    for each (let key in DATA_CALL_SETTING_KEYS) {
> +      if(this._dataCallSettingsRead.indexOf(key) == -1) {

Nit: space after `if`.

@@ +640,5 @@
> +        debug("We haven't read completely the APN data from the " +
> +              "settings DB yet. Wait for that.");
> +        return;
> +      }
> +    }

I see now what you're trying to do here. It would be more efficient and elegant to start out with what is essentially DATA_CALL_SETTING_KEYS

  this._dataCallSettingsToRead = ["ril.data.enabled", ...];  // in constructor

and then remove items as we read them. That way, we would merely be left with testing for `this._dataCallSettingsToRead.length` here, which is much easier.

@@ +1018,5 @@
> +        this._radioEnabled = !aResult;
> +        this._ensureRadioState();
> +        break;
> +      case "ril.data.enabled":
> +        this._oldRilDataEnabledState = this.dataCallSettings["enabled"]; 

Please add

  // Fall through!

here

@@ +1025,5 @@
> +      case "ril.data.user":
> +      case "ril.data.passwd":
> +        let key = aName.search("roaming") != -1 ?
> +          "roaming" :
> +          aName.substring(aName.lastIndexOf(".") + 1, aName.length);

Meh, just strip the first 9 characters:

  key = key.slice(9);

Also no need to treat "roaming.enabled" as a special case. If we really care, we could rename it to "ril.data.roaming_enabled".

@@ +1028,5 @@
> +          "roaming" :
> +          aName.substring(aName.lastIndexOf(".") + 1, aName.length);
> +        this.dataCallSettings[key] = aResult;
> +        debug("'" + aName + "'" + " is now " + this.dataCallSettings[key]);
> +        if(this._dataCallSettingsRead.indexOf(aName) == -1) {

Nit: space after `if`.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v4

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Gaia pull request https://github.com/mozilla-b2g/gaia/pull/2895 for changing the setting app since we have change an APN setting key.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 646501 [details] [diff] [review]
WIP v4

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

r=me with final points below addressed.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +186,5 @@
> +  gSettingsService.getLock().get("ril.data.user", this);
> +  gSettingsService.getLock().get("ril.data.passwd", this);
> +  gSettingsService.getLock().get("ril.data.roaming_enabled", this);
> +  gSettingsService.getLock().get("ril.data.enabled", this);
> +  this._dataCallSettingsToRead = DATA_CALL_SETTING_KEYS; 

Well, no. There was a reason why I spelled my example out the way I did. What you're doing here is just saving a reference to the global array. So when we read settings, you're modifying that. Either you want to say

  this._dataCallSettingsToRead = DATA_CALL_SETTING_KEYS.slice();

Or, you know, just get rid of the DATA_CALL_SETTING_KEYS const because this is the only place we'd need it. (Please do the latter as I indicated previously.)

@@ +634,5 @@
>      }
>    },
>  
> +  updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    if (this._dataCallSettingsToRead.length > 0) {

Just

  if (this._dataCallSettingsToRead.length)

is enough.

@@ +983,5 @@
>    observe: function observe(subject, topic, data) {
>      switch (topic) {
>        case kMozSettingsChangedObserverTopic:
>          let setting = JSON.parse(data);
> +        this.handle(setting.key, setting.value);

<3

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v5

Latest comments addressed.

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 646881 [details] [diff] [review]
WIP v5

(Thanks! No need to ask for review again since I already gave you r+.)

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 646881 [details] [diff] [review]
WIP v5

Oops, I've tested it after latest airplane mode changes in gaia and I've noticed the data call does not reconnect after toggling off airplane mode. Looking at it.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v6

Comment 20

[Philipp von Weitershausen [:philikon]]

Is this ready to be pushed now?

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> Is this ready to be pushed now?

Yes it is. I did not ask for review at you since you already gave me r+. Just finished a test again after a clean build and it seems everything is working.

Comment 22

[Philipp von Weitershausen [:philikon]]

Great, thanks for the update. Next time, please set the checkin-needed keyword to indicate that the patch is ready to be pushed. Thanks!

Comment 23

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7c41b33485

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c7c41b33485

Comment 25

[Andrew Overholt [:overholt]]

Philikon says this is very useful but not critical so basecamp-'ing (even though it's fixed :).