713426 - RIL: implement 3G data call APIs

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Thinker Li [:sinker]]

Implements 3G data call APIs for B2G RIL (Bug 710489).

Comment 1

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker

Comment 2

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker

--
Attachment #584251 [details] [diff] - Attachment is obsolete: true

Comment 3

[Thinker Li [:sinker]]

Attachment: WEB APIs for testing only

Comment 4

[Thinker Li [:sinker]]

(In reply to Thinker Li from comment #3)
> Created attachment 584253 [details] [diff] [review]
> WEB APIs for testing only

Although we are going to Network Manager, I still leave this patch here for testing.
Following url is a branch of gaia with simple setting UI to make a data call.
https://github.com/ThinkerYzu/gaia/tree/data-call

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 584252 [details] [diff] [review]
Support PDP data call in ril_worker

>diff --git a/dom/telephony/ril_worker.js b/dom/telephony/ril_worker.js
>--- a/dom/telephony/ril_worker.js
>+++ b/dom/telephony/ril_worker.js
>@@ -439,16 +439,17 @@ let Buf = {
>       let token = this.readUint32();
>       let error = this.readUint32();
>       length -= 2 * UINT32_SIZE;
>       request_type = this.tokenRequestMap[token];
>       if (error) {
>         //TODO
>         debug("Received error " + error + " for solicited parcel type " +
>               request_type);
>+	RIL.handleError(request_type, error);
...
>+  handleError: function handleError(request_type, error) {
>+    Phone.onError(request_type, error);
>   }
...
>+  onError: function onError(requestType, error) {
>+    this.sendDOMMessage({type: "error",
>+			 requestType: requestType,
>+			 errorCode: error});
>+  },

This is a good idea, but it's very orthogonal to this bug. Please file a separate bug and attach a separate patch. We should discuss what the error handling should look like.

Also, nit: tabs!

>@@ -614,19 +615,19 @@ let RIL = {
>    * @param uusInfo
>    *        Integer doing something XXX TODO
>    */
>   dial: function dial(address, clirMode, uusInfo) {
>     let token = Buf.newParcel(REQUEST_DIAL);
>     Buf.writeString(address);
>     Buf.writeUint32(clirMode || 0);
>     Buf.writeUint32(uusInfo || 0);
>-	// TODO Why do we need this extra 0? It was put it in to make this
>-	// match the format of the binary message.
>-	Buf.writeUint32(0);
>+        // TODO Why do we need this extra 0? It was put it in to make this
>+        // match the format of the binary message.
>+        Buf.writeUint32(0);

Good job spotting the tabs here! But please make sure you replace them with the correct number of spaces for indentation.

>+  /**
>+   * Deactivate a data call.
>+   *
>+   * @param cid
>+   *        String containing CID.
>+   * @param reason
>+   *        DATACALL_DEACTIVATE_NO_REASON => no reason,
>+   *        DATACALL_DEACTIVATE_RADIO_SHUTDOWN => radio shutdown.

This is kind of self-explanatory :). You could just say

  One of the DATACALL_DEACTIVATE_* constants.

>+RIL[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST() {
>+  var cid, active, type, apn, address;
>+  var num, i;
>+  var datacalls = [];
>+  
>+  num = Buf.readUint32();
>+  for(i = 0; i < num; i++) {

Nit: please use 'let' for variable declarations. You can declare and assign 'num' in one go:

  let num = Buf.readUint32();

And you should declare 'i' in the proper scope:

  for (let i = 0; ...)

Notice also the space after 'for'.


I haven't looked closely at the rest of the file. I would like to finalize bug 674726 first. It's been sitting in my review queue longer and I owe Ben a review by now. I suggest you study Ben's patch and rebase your work on top of his.

I did notice the rest of the patch still contains a lot of tabs and a lot of inconsistent indentation. Please fix this.

Comment 6

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v2

--
Attachment #584252 [details] [diff] - Attachment is obsolete: true

Comment 7

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v3

--
Attachment #584259 [details] [diff] - Attachment is obsolete: true

Comment 8

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v3

--
Attachment #584260 [details] [diff] - Attachment is obsolete: true

Comment 9

[Thinker Li [:sinker]]

(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> Comment on attachment 584252 [details] [diff] [review]
> Support PDP data call in ril_worker
> 
> >diff --git a/dom/telephony/ril_worker.js b/dom/telephony/ril_worker.js
> >--- a/dom/telephony/ril_worker.js
> >+++ b/dom/telephony/ril_worker.js
> >@@ -439,16 +439,17 @@ let Buf = {
> >       let token = this.readUint32();
> >       let error = this.readUint32();
> >       length -= 2 * UINT32_SIZE;
> >       request_type = this.tokenRequestMap[token];
> >       if (error) {
> >         //TODO
> >         debug("Received error " + error + " for solicited parcel type " +
> >               request_type);
> >+	RIL.handleError(request_type, error);
> ...
> >+  handleError: function handleError(request_type, error) {
> >+    Phone.onError(request_type, error);
> >   }
> ...
> >+  onError: function onError(requestType, error) {
> >+    this.sendDOMMessage({type: "error",
> >+			 requestType: requestType,
> >+			 errorCode: error});
> >+  },
> 
> This is a good idea, but it's very orthogonal to this bug. Please file a
> separate bug and attach a separate patch. We should discuss what the error
> handling should look like.
> 
I had filed bug 713451 for it.

Comment 10

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v4

--
Attachment #584261 [details] [diff] - Attachment is obsolete: true

Comment 11

[Philipp von Weitershausen [:philikon]]

Bug 674726 has landed (on inbound at least). What's the status of these patches? Are they ok for me to review? If so, please assign me a review request. Thanks!

Comment 12

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v4

--
Attachment #585121 [details] [diff] - Attachment is obsolete: true

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 587351 [details] [diff] [review]
Support PDP data call in ril_worker, v4

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

::: b2g/chrome/content/repl.js
@@ +34,5 @@
>                         .createInstance(Ci.nsIServerSocket);
>    serverSocket.init(31415, true, 5);
>    dump("Opened socket on " + serverSocket.port + '\n');
>    serverSocket.asyncListen(listener);
> +})();

Unrelated whitespace change in an unrelated file. Please remove.

::: b2g/chrome/jar.mn
@@ +6,5 @@
>  
>  * content/shell.xul                     (content/shell.xul)
>    content/shell.js                      (content/shell.js)
>    content/touch.js                      (content/touch.js)
> +  content/repl.js                       (content/repl.js)

Unrelated change. Please remove.

::: dom/telephony/nsITelephone.idl
@@ +53,5 @@
>                               in boolean isActive);
> +
> +  /**
> +   * \param dataState use DATACALL_STATE_* values from nsITelephone.
> +   */

Nit: this is not a proper JavaDoc comment. Typically they look like this:

  /**
   * General description of the method.
   *
   * @param firstparameter
   *        Description of the first parameter.
   * @param firstparameter
   *        Description of the first parameter.
   *
   * @return whatever the return value is.
   */

@@ +107,5 @@
>  
>    attribute bool microphoneMuted;
>    attribute bool speakerEnabled;
>  
> +  /* PDP APIs */

Nit: Coding style dictates // comment.

@@ +111,5 @@
> +  /* PDP APIs */
> +  void setupDataCall(in long radioTech,
> +                     in DOMString apn, in DOMString user, in DOMString passwd,
> +                     in long chappap, in DOMString pdptype);
> +  void deactivateDataCall(in DOMString cid, in DOMString reason);

Nit: please put each argument on its own new line.

::: dom/telephony/nsTelephonyWorker.js
@@ +112,5 @@
>  
> +const DATACALL_STATE_CONNECTING = "connecting";
> +const DATACALL_STATE_CONNECTED = "connected";
> +const DATACALL_STATE_DISCONNECTING = "disconnecting";
> +const DATACALL_STATE_DISCONNECTED = "disconnected";

You can access consts defined in ril_consts.js via the `RIL` namespace, e.g. RIL.GECKO_DATACALL_STATE_... (see also my comment on these constants in ril_consts.js)

@@ +129,1 @@
>    };

Bug 674726 transferred much of the voice call state machine from nsTelephonyWorker.js to ril_worker.js. I would like to follow the same pattern with data calls.

In fact, the `currentState` attribute is completely superfluous now. I will remove it in a follow-up bug.

@@ +302,5 @@
> +    "connecting": Ci.nsITelephone.DATACALL_STATE_CONNECTING,
> +    "connected": Ci.nsITelephone.DATACALL_STATE_CONNECTED,
> +    "disconnecting": Ci.nsITelephone.DATACALL_STATE_DISCONNECTING,
> +    "disconnected": Ci.nsITelephone.DATACALL_STATE_DISCONNECTED
> +  },

If we make the call state constants integers, we don't need this.

@@ +307,5 @@
> +
> +  /**
> +   * Handle data call state changes.
> +   *
> +   * It update current state, and setting DNS and default route.

IMHO we should not do DNS and route here. This should be in a network manager component of sorts.

Also, grammar nit: It update*s* current state and *sets* DNS and default route.

@@ +317,5 @@
> +      datacalls[message.cid] = message;
> +      this.setupEnvForDataCall(message);
> +    } else {
> +      delete datacalls[message.cid];
> +    }

C.f. my comment above about where data call state lives.

@@ +326,5 @@
> +                            [message.cid, ifname, state]);
> +    }
> +  },
> +
> +  setupEnvForDataCall: function setupEnvForDataCall(message) {

Please get rid of this method here. It does not belong here. Instead nsITelephone should expose an interface so that a network manager component can make the appropriate settings.

::: dom/telephony/ril_consts.js
@@ +411,5 @@
> +
> +const DATACALL_STATE_CONNECTING = "connecting";
> +const DATACALL_STATE_CONNECTED = "connected";
> +const DATACALL_STATE_DISCONNECTING = "disconnecting";
> +const DATACALL_STATE_DISCONNECTED = "disconnected";

Two things:

(a) All other states at this level are integers, so I think we should stick to integers. Then we will also have a very easy time mapping them to the constants defined in nsITelephone. In fact, we should note this down in a comment here.

(b) These constants are invented by us, not by the RIL. I think we should reflect that in the name. Perhaps GECKO_DATACALL_STATE_*.

::: dom/telephony/ril_worker.js
@@ +774,5 @@
> +  setupDataCall: function(radioTech, apn, user, passwd, chappap, pdptype) {
> +    let token = Buf.newParcel(REQUEST_SETUP_DATA_CALL);
> +    Buf.writeUint32(7);
> +    Buf.writeString("" + radioTech);
> +    Buf.writeString("" + DATACALL_PROFILE_DEFAULT);

Please use .toString()

@@ +778,5 @@
> +    Buf.writeString("" + DATACALL_PROFILE_DEFAULT);
> +    Buf.writeString(apn);
> +    Buf.writeString(user);
> +    Buf.writeString(passwd);
> +    Buf.writeString("" + chappap);

Same here.

@@ +1031,5 @@
>  RIL[REQUEST_QUERY_CLIP] = null;
>  RIL[REQUEST_LAST_DATA_CALL_FAIL_CAUSE] = null;
> +RIL[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST() {
> +  let datacalls = [];
> +  let num = Buf.readUint32();

Judging from my experience with voice calls (REQUEST_GET_CURRENT_CALLS), this uint32 can also be missing from the parcel altogether. So you want to take the `length` parameter here like all the other methods, and check that you actually have data to work with.

@@ +1034,5 @@
> +  let datacalls = [];
> +  let num = Buf.readUint32();
> +
> +  for (let i = 0; i < num; i++) {
> +    datacalls.path({

Did you mean datacalls.push()?

@@ +1558,5 @@
> +  },
> +
> +  onDataCallList: function onDataCallList(datacalls) {
> +    for (let datacall in datacalls) {
> +      if(datcall.active == DATACALL_INACTIVE) {

I'm pretty sure this loop is wrong. `for x in y` iterates over the *attributes* (in this case indexes) of `y`. You want a traditional for-i loop here.

Also, coding style nit: space between `if` and `(`.

@@ +1559,5 @@
> +
> +  onDataCallList: function onDataCallList(datacalls) {
> +    for (let datacall in datacalls) {
> +      if(datcall.active == DATACALL_INACTIVE) {
> +        this.deactivateCids.push(String(datacall.cid));

Why String()?

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 587351 [details] [diff] [review]
Support PDP data call in ril_worker, v4

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

::: dom/telephony/nsITelephone.idl
@@ +56,5 @@
> +   * \param dataState use DATACALL_STATE_* values from nsITelephone.
> +   */
> +  void dataCallStateChanged(in AString cid,
> +			    in AString interfaceName,
> +			    in unsigned short callState);

Another point: We might want keep this interface strictly about telephony and add another one for data callbacks. That way the DOM telephony stuff won't have to implement all the methods we end up adding to this, which it won't be interested in mostly likely.

Comment 15

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v5

--
Attachment #587351 [details] [diff] - Attachment is obsolete: true

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 587690 [details] [diff] [review]
Support PDP data call in ril_worker, v5

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

::: dom/telephony/nsITelephone.idl
@@ +73,5 @@
> +                            in AString interfaceName,
> +                            in unsigned short callState);
> +
> +  void receiveDataCallList([array,size_is(aLength)] in nsIDataCallInfo aDataCalls,
> +                           in unsigned long aLength);

Rather than inventing another XPCOM object and interface (nsIDataCallInfo), can we follow the example of telephony and call an enumerate callback with individual parameters?

We can also address this in a follow-up bug when we implement the network manager component.

::: dom/telephony/nsTelephonyWorker.js
@@ +41,5 @@
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/ctypes.jsm");

This is unnecessary now. Please remove.

@@ +144,1 @@
>  nsTelephonyWorker.prototype = {

Unrelated (and incorrect) whitespace change. Please remove.

@@ +326,5 @@
> +    
> +    for each (let datacall in message.datacalls)
> +      datacalls.push(new dataCallInfo(datacall.state,
> +                                      datacall.cid,
> +                                      datacall.apn));

Coding style nit: always need the braces

@@ +328,5 @@
> +      datacalls.push(new dataCallInfo(datacall.state,
> +                                      datacall.cid,
> +                                      datacall.apn));
> +    this._deliverDataCallCallback("receiveDataCallList",
> +                                    [datacalls, datacalls.length]);

Coding style nit: indention alignment

@@ +482,5 @@
> +  unregisterDataCallCallback: function unregisterDataCallCallback(callback) {
> +    debug("Unregistering callback: " + callback);
> +    let index;
> +    if (this._datacall_callbacks &&
> +        (index = this._datacall_callbacks.indexOf(callback) != -1)) {

There's a critical bug in this line that has been fixed in the original code. See bug 717156.

::: dom/telephony/ril_worker.js
@@ +1033,5 @@
> +RIL[REQUEST_DATA_CALL_LIST] = function REQUEST_DATA_CALL_LIST(length) {
> +  let datacalls = [];
> +
> +  if (!length)
> +    return;

Coding style nit: always need braces. Also, the `let datacalls = [];` line can go after this.

@@ +1223,5 @@
> +
> +  /**
> +   * Run all data call actions in sequence without interleaving.
> +   */
> +  dataCallSeq: {

This is new. Where does this requirement come from? I don't see anything like this in the Android RIL code. Do other RIL functions need the same kind of transactionality? If so, perhaps we should build something more generic. Also, is this a problem that is likely to occur, or just a rare race condition?

@@ +1234,5 @@
> +     * The data call action that is being served.
> +     *
> +     * This is a [action, options] pairs.  |action| is a function that
> +     * |options| would be passed as one and only one argument.  This
> +     * pair is the |options| argument of dataCallSeq.run().

Nit: avoid double spaces after periods.

@@ +1238,5 @@
> +     * pair is the |options| argument of dataCallSeq.run().
> +     *
> +     * See dataCallSeq.run() and dataCallSeq.finish().
> +     */
> +    doing: null,

Better name: active

@@ +1241,5 @@
> +     */
> +    doing: null,
> +    
> +    _doDataCall: function () {
> +      if (!this.doing && this.waitingQueue.length > 0) {

Nit: bail out early.

@@ +1243,5 @@
> +    
> +    _doDataCall: function () {
> +      if (!this.doing && this.waitingQueue.length > 0) {
> +        debug("waitingQueue: " + String(this.waitingQueue));
> +        this.doing = this.waitingQueue.splice(0, 1)[0];

You want to use Array.shift() here.

@@ +1265,5 @@
> +     */
> +    finish: function () {
> +      this.doing = null;
> +      this._doDataCall();
> +    },

I'd like to keep the queue a bit simpler and less prone to object allocations. The `options` object that we receive from the main thread already contains the method name, so no need to store a separate reference to a bound function.

How about:

  dataCallQueue: {
    active: null,
    pending: [],

    add: function add(options) {
      this.pending.push(options);
      this.process();
    },

    finish: function finish() {
      this.active = null;
      this.process();
    }

    process: function process() {
      if (this.active || !this.pending.length) {
        return;
      }
      let options = this.active = this.pending.shift();
      Phone.["_" + options.type](options);
    }
  }

and then you'd use it like this:

  setupDataCall: function setupDataCall(options) {
    this.dataCallQueue.add(options);
  },

  _setupDataCall: function _setupDataCall(options) {
    ...
    this.dataCallQueue.finish();
  },

@@ +1591,5 @@
>    onAcknowledgeSMS: function onAcknowledgeSMS() {
>    },
>  
> +  onSetupDataCall: function onSetupDataCall(cid, ifname, ipaddr, dns, gw) {
> +    let options = this.dataCallSeq.doing[1];

This makes me very confused. I didn't think `doing` was an array...

@@ +1619,5 @@
> +    let options = this.dataCallSeq.doing[1];
> +    let cid = options.cid;
> +
> +    if (!(cid in this.currentDataCalls))
> +      return;

Coding style nit: always need the braces.

@@ +1624,5 @@
> +    
> +    let apn = this.currentDataCalls[cid].apn;
> +    
> +    delete this.currentDataCalls[cid];
> +    

Nit: excessive whitespace

@@ +1634,5 @@
> +    this.dataCallSeq.finish();
> +  },
> +
> +  onDataCallList: function onDataCallList(datacalls) {
> +    let currentDataCalls = this.currentDataCalls;

Please avoid this. It makes it hard to see where things come from.

@@ +1638,5 @@
> +    let currentDataCalls = this.currentDataCalls;
> +    
> +    // Sync content of currentDataCalls and data call list.
> +    for each (let datacall in datacalls) {
> +      let {cid: cid, apn: apn} = datacall;

You can just do

  let {cid, apn} = datacall;

@@ +1642,5 @@
> +      let {cid: cid, apn: apn} = datacall;
> +      
> +      if (datacall.active != DATACALL_INACTIVE) {
> +        // XXX: This should be followed up.
> +        // datacall.active == DATACALL_ACTIVE_DOWN(1) for my device

What does that mean? Please file a follow-up bug if there is more to do here.

@@ +1655,5 @@
> +                               state: GECKO_DATACALL_STATE_CONNECTED,
> +                               cid: cid,
> +                               apn: apn})
> +        }
> +      } else if (cid in currentDataCalls) {

Please make this "if not in / else if in" construct a simple `if (cid in currentDataCalls) {} else {}`

@@ +1668,5 @@
> +    let (datacall_list = []) {
> +      for each (let datacall in this.currentDataCalls) {
> +        datacall_list.push({state: datacall.state,
> +                            cid: datacall.cid,
> +                            apn: datacall.apn});

Why are you making a copy of the `datacall` objects? You can just do datacall_list.push(datacall), no? They will be serialized before being sent to the main thread anyway.

@@ +1827,5 @@
> +   * setupDataCall() and deactivateDataCall() must be executed in
> +   * sequence without interleaving.  Since there is no hint in reponse
> +   * messages to tell respective requests.
> +   */
> +  setupDataCall: function (options) {

missing function name after `function` keyword.

@@ +1831,5 @@
> +  setupDataCall: function (options) {
> +    this.dataCallSeq.run(this._setupDataCallReal.bind(this), options);
> +  },
> +
> +  _setupDataCallReal: function (options) {

ditto.

@@ +1844,5 @@
> +
> +  /**
> +   * Deactivate a data call (PDP).
> +   */
> +  deactivateDataCall: function (options) {

ditto.

@@ +1848,5 @@
> +  deactivateDataCall: function (options) {
> +    this.dataCallSeq.run(this._deactivateDataCall.bind(this), options);
> +  },
> +  
> +  _deactivateDataCall: function (options) {

ditto.

Comment 17

[Thinker Li [:sinker]]

Comment on attachment 587690 [details] [diff] [review]
Support PDP data call in ril_worker, v5

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

::: dom/telephony/ril_worker.js
@@ +1223,5 @@
> +
> +  /**
> +   * Run all data call actions in sequence without interleaving.
> +   */
> +  dataCallSeq: {

We can setup multiple links concurrently.  For example, setup links for Internet and SMS service.  But, the responses of setup requests have no any information that can be use to tell corresponding request.  It is also true for activate data call request.  Although, we can send a extra "get data call list" request to get more information, but it make thing going complicate but gain nothing in efficiency.

@@ +1265,5 @@
> +     */
> +    finish: function () {
> +      this.doing = null;
> +      this._doDataCall();
> +    },

I think it is implicit and strong coupling.  I don't think it would be more efficient.  Although we can save an object, but we create another string and symbol looking up.  This symbol will be generated on the fly that is not jit friendly as I know.  (fix me if I am wrong)

@@ +1591,5 @@
>    onAcknowledgeSMS: function onAcknowledgeSMS() {
>    },
>  
> +  onSetupDataCall: function onSetupDataCall(cid, ifname, ipaddr, dns, gw) {
> +    let options = this.dataCallSeq.doing[1];

I had mentioned it in the comment block just above the declaration of 'active'.  It is a pair of <action function> and <function arg>.  I will change it into two variables to make it more clear.

@@ +1642,5 @@
> +      let {cid: cid, apn: apn} = datacall;
> +      
> +      if (datacall.active != DATACALL_INACTIVE) {
> +        // XXX: This should be followed up.
> +        // datacall.active == DATACALL_ACTIVE_DOWN(1) for my device

The value of datacall.active can be one of DATACALL_INACTIVE, DATACALL_ACTIVE_DOWN, and DATACALL_ACTIVE_UP.  At beginning, I expect to get DATA_ACTIVE_UP, but I actually get DATA_ACTIVE_DOWN (active/physical link down) and I can ping to the world.  There must to be something wrong or to be misunderstood.  So, I leave it here.  I will file a bug for this issue later.

@@ +1655,5 @@
> +                               state: GECKO_DATACALL_STATE_CONNECTED,
> +                               cid: cid,
> +                               apn: apn})
> +        }
> +      } else if (cid in currentDataCalls) {

It does not do different.  The origin structure is "if (cond1) { if(cond2) {block1} } else { /* !cond1 */ if(!cond2) {block2} }".  It equals to "if(cond1 && cond2) {block1}; if(!cond1 && !cond2) {block2};".  So, we can not just switch inner and outer and hope it to be simpler.  I will change "else if" to "else { if" to make it more clear.

@@ +1668,5 @@
> +    let (datacall_list = []) {
> +      for each (let datacall in this.currentDataCalls) {
> +        datacall_list.push({state: datacall.state,
> +                            cid: datacall.cid,
> +                            apn: datacall.apn});

Although it adds extra code, it make code more explicit and clear.  If you think it is worthless, I will remove it and leave a comment.

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to Thinker Li from comment #17)
> ::: dom/telephony/ril_worker.js
> @@ +1223,5 @@
> > +
> > +  /**
> > +   * Run all data call actions in sequence without interleaving.
> > +   */
> > +  dataCallSeq: {
> 
> We can setup multiple links concurrently.  For example, setup links for
> Internet and SMS service.  But, the responses of setup requests have no any
> information that can be use to tell corresponding request.

That's not true. There's the serial/token that let's us match a response to a request. Right now we don't take advantage of this at all, but we could add a framework to do that. If this is the only reason why we're doing this poor-man's transaction system, then I'd prefer we solve this problem at a deeper level -- eventually.

> @@ +1265,5 @@
> > +     */
> > +    finish: function () {
> > +      this.doing = null;
> > +      this._doDataCall();
> > +    },
> 
> I think it is implicit and strong coupling.  I don't think it would be more
> efficient.  Although we can save an object, but we create another string and
> symbol looking up.  This symbol will be generated on the fly that is not jit
> friendly as I know.  (fix me if I am wrong)

Strings are very cheap (much, much cheaper than objects). And I think attribute lookup can be considered free for small objects.

> @@ +1591,5 @@
> >    onAcknowledgeSMS: function onAcknowledgeSMS() {
> >    },
> >  
> > +  onSetupDataCall: function onSetupDataCall(cid, ifname, ipaddr, dns, gw) {
> > +    let options = this.dataCallSeq.doing[1];
> 
> I had mentioned it in the comment block just above the declaration of
> 'active'.  It is a pair of <action function> and <function arg>.  I will
> change it into two variables to make it more clear.

Like I wrote, I would prefer reusing `options` entirely instead of creating an extra array.

> The value of datacall.active can be one of DATACALL_INACTIVE,
> DATACALL_ACTIVE_DOWN, and DATACALL_ACTIVE_UP.  At beginning, I expect to get
> DATA_ACTIVE_UP, but I actually get DATA_ACTIVE_DOWN (active/physical link
> down) and I can ping to the world.  There must to be something wrong or to
> be misunderstood.  So, I leave it here.  I will file a bug for this issue
> later.

Please file a bug now and mention the bug in your comment. I know the rest of the code sets a bad example, but we must start tracking these issues from now on.

> It does not do different.  The origin structure is "if (cond1) { if(cond2)
> {block1} } else { /* !cond1 */ if(!cond2) {block2} }".  It equals to
> "if(cond1 && cond2) {block1}; if(!cond1 && !cond2) {block2};".  So, we can
> not just switch inner and outer and hope it to be simpler.  I will change
> "else if" to "else { if" to make it more clear.

Ah never mind, you're right. No need to change it.

> @@ +1668,5 @@
> > +    let (datacall_list = []) {
> > +      for each (let datacall in this.currentDataCalls) {
> > +        datacall_list.push({state: datacall.state,
> > +                            cid: datacall.cid,
> > +                            apn: datacall.apn});
> 
> Although it adds extra code, it make code more explicit and clear.  If you
> think it is worthless, I will remove it and leave a comment.

It needlessly creates many new objects. Please simplify it.


Also, as discussed on IRC, you can leave the signature for receiveDataCallList for the time being.

Comment 19

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v6

--
Attachment #587690 [details] [diff] - Attachment is obsolete: true

Comment 20

[Philipp von Weitershausen [:philikon]]

Comment on attachment 588412 [details] [diff] [review]
Support PDP data call in ril_worker, v6

>-[scriptable, uuid(5be6e41d-3aee-4f5c-8284-95cf529dd6fe)]
>+[scriptable, uuid(063f4a9e-3c3a-11e1-a3c6-f46d04d25bcc)]
>+interface nsIDataCallInfo : nsISupports
...
>+[scriptable, uuid(25e49098-3bec-11e1-8567-f46d04d25bcc)]
>+interface nsIPhoneDataCallCallback : nsISupports
...
>+const DATACALLINFO_CID =
>+  Components.ID("{3d3e0b6a-3c5a-11e1-9869-f46d04d25bcc}");

Those UUIDs look suspiciously similar in a lot of places. Did you randomly generate those? firebot on IRC can help you with that. Just do `/msg firebot uuid`

>+function dataCallInfo(state, cid, apn) {
>+  this.callState = state;
>+  this.cid = cid;
>+  this.apn = apn;
>+}
>+
>+dataCallInfo.protoptype = {
>+  classID:      DATACALLINFO_CID,
>+  classInfo:    XPCOMUtils.generateCI({classID: DATACALLINFO_CID,
>+                                       classDescription: "DataCallInfo",
>+                                       interfaces: [Ci.nsIDataCallInfo]}),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDataCallInfo]),
>+};

Sigh, I guess we have to keep this for now. But please capitalize the name (`DataCallInfo`) since it's a constructor and remove the empty line between the constructor and the prototype declaration.

r=me with those addressed. If you want to upload a patch with those fixes, I'll commit it to mozilla-central. Thanks!

Comment 21

[Thinker Li [:sinker]]

Attachment: Support PDP data call in ril_worker, v7

--
Attachment #588412 [details] [diff] - Attachment is obsolete: true

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 589128 [details] [diff] [review]
Support PDP data call in ril_worker, v7

>+  onSetupDataCall: function onSetupDataCall(token, cid, ifname, ipaddr,
>+                                            dns, gw) {
>+    let options = this.activeRequests[token];

I missed this in an earlier review: there's a memory leak here if we don't delete the stuff from `activeRequests` again. Also, there were a lot of styling nits still unaddressed and several (non-critical) syntax/style mistakes (missing semicolons, braces, etc.). I addressed these and pushed the patch to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b105fa66bb37

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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