741862 - B2G 3G: Settings API hookup

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

We should let the user to set the APN settings, user name and password through the setting application. The navigator.settings API is already landed so we have to fill in the gap between the navigator.settings API and the RIL plumbing. The flow would be Settings application (APN settings) > Settings API > RIL plumbing (Data Calls).

Comment 1

[Philipp von Weitershausen [:philikon]]

José, any progress here? This and the network manager are the two main things that are blocking 3G on the phone right now. I'm working on getting network manager landed, we need this other piece asap. Thanks!

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Yes, I am uploading the WIP patch tomorrow morning (CET). I will request feedback at you once uploaded. We have been a couple of days off due Easter.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v1

This is an incomplete WIP patch in which I try to set the data call settings in a way a little bit tricky. This is because It seems the settings API is mainly  designed to be used from the DOM (I think so, sorry if I am wrong). I was not able to retrieve the SettingsManager service for using it and access its functionality from RadioLayerInterface. I cannot call set/get functions because some permission problems([1], [2]). I know about the domain permissions for every application that wants to use the Setting API but this is not that case. What about to use this API from other place than an application? Maybe Gregor could shed some light on that.  Taken this into account I have decided to set the data call settings from shell.js by using the 'pref' service. Maybe you don't like this approach. In RadioLayerInterface I have observed the Settings API notifications in order to handle the setting changes in data calls. The latter is still incomplete.

Please some feedback on this patch would be very appreciated.

[1]

I/Gecko   ( 2623): -*- SettingsManager: get not allowed
E/GeckoConsole( 2623): [JavaScript Error: "uncaught exception: [Exception... "'Method not implemented' when calling method: [nsIDOMSettingsLock::get]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js :: _isDataEnabled :: line 303"  data: no]"]
 
[2] http://mxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js#227

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #3)
> Created attachment 613273 [details] [diff] [review]
> WIP v1
> 
> This is an incomplete WIP patch in which I try to set the data call settings
> in a way a little bit tricky. This is because It seems the settings API is
> mainly  designed to be used from the DOM (I think so, sorry if I am wrong).
> I was not able to retrieve the SettingsManager service for using it and
> access its functionality from RadioLayerInterface.

Right now we don't have a SettingsManager service.
This will change after implementing Bug 743336.
Currently you can use the window to get the settingsManager and
perform get/set.

 I cannot call set/get
> functions because some permission problems([1], [2]). I know about the
> domain permissions for every application that wants to use the Setting API
> but this is not that case. What about to use this API from other place than
> an application? Maybe Gregor could shed some light on that. 


You mean chrome code in JS or C++? Our implementation is focused on modifying settings from the settings-app and setting-changes should be observable in chrome and content. Using the API from chrome will be easier once the service is added.

Comment 5

[Philipp von Weitershausen [:philikon]]

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

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

Ooof, I didn't realize we didn't have an XPCOM-accessible way to read settings. I would be ok with this approach as a temporary hack, so long as we fix it properly once bug 743336 is fixed.

f- for this because it doesn't watch for settings updates! We need to add a listener here, update the prefs on the fly, and notify RILNetworkInterface so that -- depending on the changes / new values for the settings -- it cancels the current datacall or starts a new one (potentially with updated credentials).

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v2

New WIP pacth. This watches for data call setting changes and connect
or disconnect the data call depending of those new settings. The last
patch did not do that just it read the APN setting at booting time.

Now RadioLayerInterface implements the nsIObserver interface and watch
for changes in the data call settings. I have decided to implement the
following rule regarding setting changes. 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. Please, let me know if this makes sense or
not. I think we should let the application to request a reconnection
and keep the RIL as simple as possible when dealing with data call
setting changes.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

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

Just noticed Philikon has landed the network manager component so I'll rebase this patch and request review at Philikon again. I think this makes sense because the relationship between both issues.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v3

Rebased and ready for a review.

Comment 9

[Gregor Wagner [:gwagner]]

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


>+(function DataCallSettings() {
>+  let sm = navigator.mozSettings;
>+  DATA_CALL_SETTING_BOLKEYS.forEach(function(key) {
>+    let request = sm.getLock().get(key);
>+    request.onsuccess = function onSuccess() {
>+      let value = request.result[key] || false;
>+      Services.prefs.setBoolPref(key, value);
>+      dump("DataCallSettings - " + key + ":" + value);
>+    };
>+    request.onerror = function onError() {
>+      Services.prefs.setBoolPref(key, false);
>+    };
>+  });
>+
>+  DATA_CALL_SETTING_CHARKEYS.forEach(function(key) {
>+    let request = sm.getLock().get(key);
>+    request.onsuccess = function onSuccess() {
>+      let value = request.result[key] || "";
>+      Services.prefs.setCharPref(key, value);
>+      dump("DataCallSettings - " + key + ":" + value);
>+    };
>+    request.onerror = function onError() {
>+      Services.prefs.setCharPref(key, "");
>+    };
>+  });


You don't need to get a lock for every single request.
Just use a single lock.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v4

This patch addresses what Gregor commented in comment #c9.

Comment 11

[Philipp von Weitershausen [:philikon]]

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

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

Looks great! Just a bunch of nits here and there.

::: b2g/chrome/content/shell.js
@@ +368,5 @@
> +const DATA_CALL_SETTING_BOLKEYS = ["ril.data.enabled",
> +                                   "ril.data.roaming.enabled"];
> +const DATA_CALL_SETTING_CHARKEYS = ["ril.data.apn",
> +                                "ril.data.user",
> +                                "ril.data.passwd"];

align plz

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +66,5 @@
>  
> +const DATA_CALL_SETTING_KEYS = ["ril.data.enabled", "ril.data.roaming.enabled",
> +                                "ril.data.apn", "ril.data.user",
> +                                "ril.data.passwd"];
> +

suggesting one entry per line for readability

@@ +190,5 @@
>  
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIWorkerHolder,
> +                                         Ci.nsIRadioInterfaceLayer,
> +                                         Ci.nsISupportsWeakReference,
> +                                         Ci.nsIObserver]),

No need for the weak reference stuff. Let's just add a proper clean up during "xpcom-shutdown" below.

@@ +585,5 @@
>      if (topic == "xpcom-shutdown") {
>        ppmm.removeMessageListener("RIL:GetRadioState", this);
>        Services.obs.removeObserver(this, "xpcom-shutdown");
>        ppmm = null;
> +    } else {

Let's turn this into a switch statement and put the kMozSettingsChangedObserverTopic branch in a helper method.

@@ +597,5 @@
> +            " setting value: " + setting.value);
> +      // 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

That make sense.

@@ +599,5 @@
> +      // 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.
> +      if (setting.key === "ril.data.enabled") {

Bail out early, please.

@@ +1281,5 @@
>  
> +  isConnected: function isConnected() {
> +    return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
> +  },
> +

Would prefer this to be a getter: get connected() { ... }

@@ +1316,5 @@
> +    if (this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTING ||
> +        this.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED) {
> +      return;
> +    }
> +    let reason = 0; // No specific reason specified.

Instead of using a magic number and having to comment it, you could also use RIL.DATACALL_DEACTIVATE_NO_REASON. :)

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v5

This patch addresses the review comments (#c11) from Philikon.

Comment 13

[Philipp von Weitershausen [:philikon]]

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

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

r=me with nits

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +582,5 @@
> +    // 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.
> +    if (setting.key === "ril.data.enabled") {

This would still read nicer in bail out early style:

  if (setting.key != "ril.data.enabled") {
    return;
  }

@@ +606,5 @@
> +      case "xpcom-shutdown":
> +        ppmm.removeMessageListener("RIL:GetRadioState", this);
> +        Services.obs.removeObserver(this, "xpcom-shutdown");
> +        ppmm = null;
> +        Services.obs.removeObserver(this, kMozSettingsChangedObserverTopic);

Super anal nit: group ppmm and Services.obs related lines together.

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v6

This patch addresses last Philikon's comments.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 617916 [details] [diff] [review]
WIP v6

My bien. (You didn't have to ask for review again, though, since I already r+'ed... :))

Comment 16

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/440371943508

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/440371943508