1114901 - [B2G] [RIL] Move data connection related code out of RadioInterfaceLayer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

+++ This bug was initially created as a clone of Bug #1058308 +++

Please see bug #1058308 comment #3. And we could have clearer definitions and APIs in nsIRaiodInterface for data connection.

Comment 1

[Jessica Jong [:jessica] (inactive)]

I have moved |gDataConnectionManager|, |DataConnectionHandler|, |RILNetworkInterface| and |DataCall| from |RadioInterfaceLayer| to a separate file successfully, using export/import for now. However, |DataConnectionHandler| still needs to communicate with |gRadioEnabledController|, for example, |DataConnectionHandler| needs to notify |gRadioEnabledController| when all data calls are disconnected, or in some cases, DataConnectionHandler needs to know whether it is in the process of turning radio off. I am not sure if exporting |gRadioEnabledController| is a good idea, maybe we should just pass callbacks and maintain the state internally in |DataConnectionHandler|? Do we have better options?

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes.

We are adding two new interfaces: nsIDataCallManager and nsIDataCallInterface.

nsIDataCallManager handles all the data call related stuff, and the implementation was mostly moved from RadioInterfaceLayes.js. In short, there is one DataCallManager that handles DataCallHandler(s) (one per client id).

nsIDataCallInterface is just a abstract layer for communicating with lower layer ril, so it is only responsible of sending ril requests and receiving solicited/unsolicited responses.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8587271 [details] [diff] [review]
Part 1: interface changes.

Hsinyi, Edgar, may I have your early feedback on this? Thanks!

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8587271 [details] [diff] [review]
Part 1: interface changes.

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

Please see my questions and comments below, thank you.

::: dom/system/gonk/nsIDataCallInterface.idl
@@ +104,5 @@
> +   *
> +   * @param clientId
> +   *        ID of the ril client.
> +   */
> +  void init(in long clientId);

The owner of DataCallInterface instance should be response for the initialization, unless the DataCallInterface instance and its owner are not lived in the same scope (e.g. same file ..), we won't need to define a init() in interface. So who owns the DataCallInterface instance?

@@ +175,5 @@
> +  /**
> +   * Called by RadioInterface or lower layer to notify about data call list
> +   * changed.
> +   */
> +  void handleDataCallListChanged(in uint32_t count,

I don't really quite understand the purpose of this function. Who will call this function to notify the nsIDataCallInterface?

::: dom/system/gonk/nsIDataCallManager.idl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +#include "nsIDataCallInterface.idl"

Use forward declaration seems enough.
interface nsIDataCallInterface;

@@ +37,5 @@
> +   *        Mobile network type, that is,
> +   *        nsINetworkInterface.NETWORK_TYPE_MOBILE or one of the
> +   *        nsINetworkInterface.NETWORK_TYPE_MOBILE_* values.
> +   */
> +  void setupDataCallByType(in long networkType);

Does this function need a callback or returning something for the executing result?

@@ +44,5 @@
> +
> +  /*
> +   * Get the corresponding data call interface.
> +   */
> +  nsIDataCallInterface getDataCallInterface();

nsIDataCallInterface is the underlying implementation (partner probably has it's own implementation), why need this getter function to expose it? I thought the user of nsIDataCallHandler should not need to care about the underlying implementation or call it directly. Or do I misunderstand anything?

@@ +72,5 @@
> +   *
> +   * @param numRadioInterfaces
> +   *        Number of radio interfaces.
> +   */
> +  void init(in long numRadioInterfaces);

Could the DataCallManager init itself by referencing |nsIMobileConnectionService.numItems|? Then we probably don't need this init() function.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8587271 [details] [diff] [review]
Part 1: interface changes.

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

::: dom/system/gonk/nsIDataCallInterface.idl
@@ +9,5 @@
> +{
> +  /*
> +   * Data call fail cause, 0 if there is no error.
> +   */
> +  readonly attribute long status;

Can we have the attribute name better reflecting the comment "fail cause?" 

Ideally, we should also define fail cause values that we support as constants.
The constant issue isn't major at this stage, but I'd still like to point it out since I've noticed it :)

@@ +12,5 @@
> +   */
> +  readonly attribute long status;
> +
> +  /*
> +   * If staus != 0, this fields indicates the suggested retry back-off timer.

nit: s/fields/field/

@@ +23,5 @@
> +   */
> +  readonly attribute long cid;
> +
> +  /*
> +   * 0=inactive, 1=active/physical link down, 2=active/physical link up.

Let's define constants for these.

@@ +28,5 @@
> +   */
> +  readonly attribute long active;
> +
> +  /*
> +   * One of the protocol types: "IP", "IPV4" or "IPV4V6".

ditto.

@@ +53,5 @@
> +   */
> +  readonly attribute DOMString gateways;
> +
> +  /*
> +   * Gecko network state, it is either 1=CONNECTED or 3=DISCONNECTED.

Can we define constants here? 
Ideally, user of this interface wouldn't have to know "gecko network state."

@@ +175,5 @@
> +  /**
> +   * Called by RadioInterface or lower layer to notify about data call list
> +   * changed.
> +   */
> +  void handleDataCallListChanged(in uint32_t count,

I believe the need of having this function is coming from that in mozril implementation, we need a channel for RaioInterface/ril_worker to notify DataCallInterface. However, I rethink about the solution. Apparently, this function will be moz needed only. Then could we choose to expose it via other interface or methods but not nsIDataCallInterface?  Would that be better?

::: dom/system/gonk/nsIDataCallManager.idl
@@ +47,5 @@
> +   */
> +  nsIDataCallInterface getDataCallInterface();
> +
> +  /*
> +   * Deactivate all data calls.

Please document the meaning of the return value.

@@ +54,5 @@
> +
> +  /*
> +   * Called to reconsider data call state.
> +   */
> +  void updateRILNetworkInterface();

Are we missing |updateApnSettings()|?

Comment 6

[Jessica Jong [:jessica] (inactive)]

Sorry for not being clearer, here are some clarifications.

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> Comment on attachment 8587271 [details] [diff] [review]
> Part 1: interface changes.
> 
> Review of attachment 8587271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my questions and comments below, thank you.
> 
> ::: dom/system/gonk/nsIDataCallInterface.idl
> @@ +104,5 @@
> > +   *
> > +   * @param clientId
> > +   *        ID of the ril client.
> > +   */
> > +  void init(in long clientId);
> 
> The owner of DataCallInterface instance should be response for the
> initialization, unless the DataCallInterface instance and its owner are not
> lived in the same scope (e.g. same file ..), we won't need to define a
> init() in interface. So who owns the DataCallInterface instance?

My plan is for DataCallHandler to own DataCallInterface, it makes sense, since DataCallHandler will be the main consumer of DataCallInterface. They are initialized by DataCallManager and passed to DataCallHandler, e.g.:

  this._connectionHandlers.push(
      new DataCallHandler(clientId, new DataCallInterface(clientId)));

> 
> @@ +175,5 @@
> > +  /**
> > +   * Called by RadioInterface or lower layer to notify about data call list
> > +   * changed.
> > +   */
> > +  void handleDataCallListChanged(in uint32_t count,
> 
> I don't really quite understand the purpose of this function. Who will call
> this function to notify the nsIDataCallInterface?

As Hsinyi mentioned, this is for ril_worker/RadioInterface to notify DataCallInterface about unsolicited data call list changed. If this is mozril only, maybe we should move it to an extended interface?

> 
> ::: dom/system/gonk/nsIDataCallManager.idl
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#include "nsISupports.idl"
> > +#include "nsIDataCallInterface.idl"
> 
> Use forward declaration seems enough.
> interface nsIDataCallInterface;

Sure, will do.

> 
> @@ +37,5 @@
> > +   *        Mobile network type, that is,
> > +   *        nsINetworkInterface.NETWORK_TYPE_MOBILE or one of the
> > +   *        nsINetworkInterface.NETWORK_TYPE_MOBILE_* values.
> > +   */
> > +  void setupDataCallByType(in long networkType);
> 
> Does this function need a callback or returning something for the executing
> result?

Hmm, following our current design, we do not have a callback or something for these functions. We may do it in follow-up, what do you think?

> 
> @@ +44,5 @@
> > +
> > +  /*
> > +   * Get the corresponding data call interface.
> > +   */
> > +  nsIDataCallInterface getDataCallInterface();
> 
> nsIDataCallInterface is the underlying implementation (partner probably has
> it's own implementation), why need this getter function to expose it? I
> thought the user of nsIDataCallHandler should not need to care about the
> underlying implementation or call it directly. Or do I misunderstand
> anything?

You are right. However, We need it in the case for RadioInterface to notify unsolicited data call list changed using |handleDataCallListChanged()|. We can wrap it in DataCallHandler, but it would be a problem if caf is going to reuse DataCallHanlder and they don't need the |handleDataCallListChanged()|.

> 
> @@ +72,5 @@
> > +   *
> > +   * @param numRadioInterfaces
> > +   *        Number of radio interfaces.
> > +   */
> > +  void init(in long numRadioInterfaces);
> 
> Could the DataCallManager init itself by referencing
> |nsIMobileConnectionService.numItems|? Then we probably don't need this
> init() function.

Yes, we could do that.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Thanks Hsinyi for the feedback.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Comment on attachment 8587271 [details] [diff] [review]
> Part 1: interface changes.
> 
> Review of attachment 8587271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIDataCallInterface.idl
> @@ +9,5 @@
> > +{
> > +  /*
> > +   * Data call fail cause, 0 if there is no error.
> > +   */
> > +  readonly attribute long status;
> 
> Can we have the attribute name better reflecting the comment "fail cause?" 
> 
> Ideally, we should also define fail cause values that we support as
> constants.
> The constant issue isn't major at this stage, but I'd still like to point it
> out since I've noticed it :)

These attribute names come from rild, but of course we change it to better names.
As for the constant issue, I agree, it would be clearer to have defined constants.

> 
> @@ +12,5 @@
> > +   */
> > +  readonly attribute long status;
> > +
> > +  /*
> > +   * If staus != 0, this fields indicates the suggested retry back-off timer.
> 
> nit: s/fields/field/

Thanks for catching this.

> 
> @@ +23,5 @@
> > +   */
> > +  readonly attribute long cid;
> > +
> > +  /*
> > +   * 0=inactive, 1=active/physical link down, 2=active/physical link up.
> 
> Let's define constants for these.

Sure, will do.

> 
> @@ +28,5 @@
> > +   */
> > +  readonly attribute long active;
> > +
> > +  /*
> > +   * One of the protocol types: "IP", "IPV4" or "IPV4V6".
> 
> ditto.

Sure, will do.

> 
> @@ +53,5 @@
> > +   */
> > +  readonly attribute DOMString gateways;
> > +
> > +  /*
> > +   * Gecko network state, it is either 1=CONNECTED or 3=DISCONNECTED.
> 
> Can we define constants here? 
> Ideally, user of this interface wouldn't have to know "gecko network state."

Actually, the states are defined in nsINetworkInterface, we can use them directly.

> 
> @@ +175,5 @@
> > +  /**
> > +   * Called by RadioInterface or lower layer to notify about data call list
> > +   * changed.
> > +   */
> > +  void handleDataCallListChanged(in uint32_t count,
> 
> I believe the need of having this function is coming from that in mozril
> implementation, we need a channel for RaioInterface/ril_worker to notify
> DataCallInterface. However, I rethink about the solution. Apparently, this
> function will be moz needed only. Then could we choose to expose it via
> other interface or methods but not nsIDataCallInterface?  Would that be
> better?

Yep, I was trying to figure out where to put this last time we were discussing on the white board. We can discuss again now that Edgar is back. :)

> 
> ::: dom/system/gonk/nsIDataCallManager.idl
> @@ +47,5 @@
> > +   */
> > +  nsIDataCallInterface getDataCallInterface();
> > +
> > +  /*
> > +   * Deactivate all data calls.
> 
> Please document the meaning of the return value.

Sure, will do.

> 
> @@ +54,5 @@
> > +
> > +  /*
> > +   * Called to reconsider data call state.
> > +   */
> > +  void updateRILNetworkInterface();
> 
> Are we missing |updateApnSettings()|?

It seems that we don't need |updateApnSettings()| since it is called by DataCallManager only, and DataCallManager owns DataCallHandler and they are on the same scope.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v2.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8592690 [details] [diff] [review]
Part 1: interface changes, v2.

This version is based on the previous comments and our offline discussion. Major changes include:
- Introduce 'DataCallInterfaceService' which is in charge of creating DataCallInterfaces. Consumers can use this service to get a DataCallInterface.
- Introduce 'GonkDataCallInterfaceService' which inherits from 'DataCallInterfaceService'. This is where we can notify about unsolicited data call list change.
- Define constants for some attributes. I did not add constants for protocol type, since I don't see many constants for strings attributes.

Edgar, Hsinyi, may I have your feedback again? Thanks.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8592690 [details] [diff] [review]
Part 1: interface changes, v2.

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

The overall direction looks good, thank you.

::: dom/system/gonk/nsIDataCallInterfaceService.idl
@@ +6,5 @@
> +
> +[scriptable, uuid(5b1aa55c-4c83-4a95-8e9f-d928492122d6)]
> +interface nsIDataCall : nsISupports
> +{
> +  /*

nit: the multiple commands usually starts with `/**`, here and elsewhere.

e.g.

/**
 * ....
 */

@@ +32,5 @@
> +
> +  /*
> +   * Data call protocol type, e.g. "IP", "IPV4" or "IPV4V6".
> +   */
> +  readonly attribute DOMString type;

Please remember to define constants for this. :)

@@ +148,5 @@
> +   *        Called when request is finished.
> +   */
> +  void setupDataCall(in AString apn, in AString username,
> +                     in AString password, in long authType,
> +                     in long pdpType,

And also for authType and pdpType.

@@ +162,5 @@
> +   * @param nsIDataCallCallback
> +   *        Called when request is finished.
> +   */
> +  void deactivateDataCall(in long cid,
> +                          in long reason,

And for reason as well, maybe?

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8592690 [details] [diff] [review]
Part 1: interface changes, v2.

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

The overall structure looks good to me! Let's move!!!

::: dom/system/gonk/nsIDataCallInterfaceService.idl
@@ +13,5 @@
> +   */
> +  readonly attribute long failCause;
> +
> +  /*
> +   * If staus != 0, this field indicates the suggested retry back-off timer.

nit: s/status/failCause/ and s/0/nsIDataCallInterface.DATACALL_FAIL_NONE/

@@ +141,5 @@
> +   * @param password
> +   *        Password for apn.
> +   * @param authType
> +   *        Authentication type.
> +   * @param pdpType

Considering the original request from bug 1058308, I'd encourage to define authType and pdpType as constants with int types (it's because the restrictions of defining string constants in xpidl). Yes, I understand that we will need more mappings b/w  string and int ... ...

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8592690 [details] [diff] [review]
Part 1: interface changes, v2.

Sorry... some comment conflicts that made Edgar's f+ disappear.

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> Comment on attachment 8592690 [details] [diff] [review]
> Part 1: interface changes, v2.
> 
> Review of attachment 8592690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The overall direction looks good, thank you.
> 
> ::: dom/system/gonk/nsIDataCallInterfaceService.idl
> @@ +6,5 @@
> > +
> > +[scriptable, uuid(5b1aa55c-4c83-4a95-8e9f-d928492122d6)]
> > +interface nsIDataCall : nsISupports
> > +{
> > +  /*
> 
> nit: the multiple commands usually starts with `/**`, here and elsewhere.
> 
> e.g.
> 
> /**
>  * ....
>  */
> 
> @@ +32,5 @@
> > +
> > +  /*
> > +   * Data call protocol type, e.g. "IP", "IPV4" or "IPV4V6".
> > +   */
> > +  readonly attribute DOMString type;
> 
> Please remember to define constants for this. :)
> 
> @@ +148,5 @@
> > +   *        Called when request is finished.
> > +   */
> > +  void setupDataCall(in AString apn, in AString username,
> > +                     in AString password, in long authType,
> > +                     in long pdpType,
> 
> And also for authType and pdpType.
> 
> @@ +162,5 @@
> > +   * @param nsIDataCallCallback
> > +   *        Called when request is finished.
> > +   */
> > +  void deactivateDataCall(in long cid,
> > +                          in long reason,
> 
> And for reason as well, maybe?

Thanks for the feedback. I've added constants for authType, pdpType and reason. For nsIDataCall's type, it seems that xpidl does not support string constants [1]. Do we need to convert it into a int attribute? If so, we'll need to parse each datacall coming from ril_worker and do the conversion.

[1] http://www-archive.mozilla.org/scriptable/faq.html#i8

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v3.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: DataCallInterfaceService impl, v1.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: move data call related code to DataCallManager, v1.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: move data call related code to DataCallManager, v1.

Forgot to add some files.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8595897 [details] [diff] [review]
Part 1: interface changes, v3.

Revised base on comment 10 and 11.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8595898 [details] [diff] [review]
Part 2: DataCallInterfaceService impl, v1.

This is mozril implementation of (Gonk)DataCallInterfaceService. DataCallInterface is a service that instantiates and handles DataCallInterfaces. DataCallInterface uses RadioInteraface's sendWorkerMessage to send the requests to ril_worker.

Edgar, may I have your review on this? Thanks.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8595905 [details] [diff] [review]
Part 3: move data call related code to DataCallManager, v1.

Data call related code are moved from RadioInterfaceLayer to DataCallManager. 

Most of the code logic remains the same, except that we make use of  DataCallInterface instead of RadioInterface for sending commands to ril_worker. 

Another thing is that we added a un/registerDataCallListener() to notify about all-data-disconnected event, so now consumers that would like to know about this event will use this listener.

Edgar, may I have your review on this as well? Thanks.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: move data call related code to DataCallManager, v2.

Changes since v1:
- Convert strings to ints when sending REQUEST_DEACTIVATE_DATA_CALL to rild
- Ignore unsolicited data call changes when state is connecting/disconnecting
- rebase based on bug 1149829

Comment 22

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8596432 [details] [diff] [review]
Part 3: move data call related code to DataCallManager, v2.

try passes with this patch.

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8595897 [details] [diff] [review]
Part 1: interface changes, v3.

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

Looks good, r=me with comment addressed, thank you.

::: dom/system/gonk/nsIDataCallInterfaceService.idl
@@ +81,5 @@
> +   * Called when getDataCallList() returns succesfully.
> +   */
> +  void notifyGetDataCallListSuccess(in uint32_t count,
> +                                    [array, size_is(count)] in nsIDataCall
> +                                    dataCalls);

Nit: Indentation.

@@ +122,5 @@
> +  const long DATACALL_FAIL_SIGNAL_LOST                   = -3;
> +  const long DATACALL_FAIL_PREF_RADIO_TECH_CHANGED       = -4;
> +  const long DATACALL_FAIL_RADIO_POWER_OFF               = -5;
> +  const long DATACALL_FAIL_TETHERED_CALL_ACTIVE          = -6;
> +  const long DATACALL_FAIL_ERROR_UNSPECIFIED             = 0xffff;

s/0xffff/0xFFFF/

::: dom/system/gonk/nsIDataCallManager.idl
@@ +55,5 @@
> +
> +  /**
> +   * Register/unregister to receive or stop receiving data call events.
> +   */
> +  void registerDataCallListener(in nsIDataCallListener listener);

s/registerDataCallListener/registerListener/

@@ +56,5 @@
> +  /**
> +   * Register/unregister to receive or stop receiving data call events.
> +   */
> +  void registerDataCallListener(in nsIDataCallListener listener);
> +  void unregisterDataCallListener(in nsIDataCallListener listener);

s/unregisterDataCallListener/unregisterListener/

::: dom/system/gonk/nsIGonkDataCallInterfaceService.idl
@@ +13,5 @@
> +   */
> +  void notifyDataCallListChanged(in unsigned long clientId,
> +                                 in uint32_t count,
> +                                 [array, size_is(count)] in nsIDataCall
> +                                 dataCalls);

Nit: Indentation.

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8595898 [details] [diff] [review]
Part 2: DataCallInterfaceService impl, v1.

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

Please see my in-line comments, thank you.

::: dom/system/gonk/DataCallInterfaceService.js
@@ +32,5 @@
> +                                   "nsIRadioInterfaceLayer");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gMobileConnectionService",
> +                                   "@mozilla.org/mobileconnection/mobileconnectionservice;1",
> +                                   "nsIGonkMobileConnectionService");

Since we don't call the notifying methods of nsIGonkMobileConnectionService, queryInterface to nsIMobileconnectionService seems enough.

s/nsIGonkMobileConnectionService/nsIMobileConnectionService/

@@ +91,5 @@
> +
> +  // nsIGonkDataCallInterfaceService
> +
> +  notifyDataCallListChanged: function(aClientId, aCount, aDataCalls) {
> +    let dataCallInterface = this._dataCallInterfaces[aClientId];

let dataCallInterface = this.getDataCallInterface(aClientId);

@@ +97,5 @@
> +  },
> +
> +  // nsIObserver
> +
> +  observe: function(subject, topic, data) {

Nit: Having 'a' prefix for arguments.

@@ +158,5 @@
> +      chappap: aAuthType,
> +      pdptype: pdpType
> +    }, (aResponse) => {
> +      if (aResponse.errorMsg) {
> +        aCallback.notifyError(aResponse);

aCallback.notifyError(aResponse.errorMsg);

@@ +162,5 @@
> +        aCallback.notifyError(aResponse);
> +      } else {
> +        // Convert pdp type into constant int value.
> +        aResponse.pdpType = RIL.RIL_DATACALL_PDP_TYPES.indexOf(aResponse.type);
> +        aCallback.notifySetupDataCallSuccess(aResponse);

Wrap |aResponse| into a nsIDataCall object, otherwise it may get some problems if someone calls QueryInterface on it.

@@ +173,5 @@
> +      cid: aCid,
> +      reason: aReason
> +    }, (aResponse) => {
> +      if (aResponse.errorMsg) {
> +        aCallback.notifyError(aResponse);

Ditto, aResponse.errorMsg

@@ +184,5 @@
> +  getDataCallList: function(aCallback) {
> +    this.radioInterface.sendWorkerMessage("getDataCallList", null,
> +      (aResponse) => {
> +        if (aResponse.errorMsg) {
> +          aCallback.notifyError(aResponse);

Ditto, aResponse.errorMsg

@@ +187,5 @@
> +        if (aResponse.errorMsg) {
> +          aCallback.notifyError(aResponse);
> +        } else {
> +          for (let i = 0; i < aResponse.length; i++) {
> +            let dataCall = aResponse[i];

Should this be `let dataCall = aResponse.datacalls[i];` [1]?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#5232-5250

@@ +189,5 @@
> +        } else {
> +          for (let i = 0; i < aResponse.length; i++) {
> +            let dataCall = aResponse[i];
> +            // Convert pdp type into constant int value.
> +            dataCall.pdpType = RIL.RIL_DATACALL_PDP_TYPES.indexOf(dataCall.type);

Ditto, wrap |dataCall| into a nsIDataCall object.

@@ +205,5 @@
> +        return;
> +      }
> +
> +      if (aResponse.errorMsg) {
> +        aCallback.notifyError(aResponse);

Ditto, aResponse.errorMsg

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8596432 [details] [diff] [review]
Part 3: move data call related code to DataCallManager, v2.

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

Please see my comments below, thank you.

::: dom/system/gonk/DataCallManager.js
@@ +1,1 @@
> +"use strict";

Please remember to add MPL licence.

@@ +19,5 @@
> +                                   "nsIGonkMobileConnectionService");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gRil",
> +                                   "@mozilla.org/ril;1",
> +                                   "nsIRadioInterfaceLayer");

nsIRadioInterfaceLayer is moz-only implementation, we should not call it directly, because DataCallManager plans to be reused for both moz ril and partner's ril.

@@ +89,5 @@
> +
> +function DataCallManager() {
> +  this._connectionHandlers = [];
> +
> +  let numRadioInterfaces = gRil.numRadioInterfaces;

Use nsIMobileConnectionService.numItems instead.

@@ +140,5 @@
> +  debug: function(s) {
> +    dump("-*- DataCallManager: " + s + "\n");
> +  },
> +
> +  getDataCallHandler: function(clientId) {

It is a good chance to correct some coding style, like "a" prefix for arguments.

@@ +141,5 @@
> +    dump("-*- DataCallManager: " + s + "\n");
> +  },
> +
> +  getDataCallHandler: function(clientId) {
> +    return this._connectionHandlers[clientId];

Throw an exception if unable to get a valid DataCallHandler instance.

@@ +192,5 @@
> +              newSettings.enabled = true;
> +            }
> +            self._currentDataClientId = self._dataDefaultClientId;
> +
> +            newIface.setDataRegistration(true, {

How about wrapping setDataRegistration with Promise?

@@ +221,5 @@
> +      oldSettings.oldEnabled = oldSettings.enabled;
> +      oldSettings.enabled = false;
> +    }
> +
> +    if (oldConnHandler.deactivateDataCalls()) {

Since there are two places will use deactivateDataCalls, changing default client and changing apn setting.
How about putting the common codes into a utility function (maybe returns a promise).

for example,

function _deactivateDataCalls() {
  if (connHandler.deactivateDataCalls()) {
    return new Promise(function(aResolve, aReject) {
      let callback = {
        ...
        notifyAllDataDisconnected: function() {
          connHandler.unRegisterListener(callback);
          aResolve();
        }
      };
      connHandler.registerListener(callback);
    };
  }

  return Promise.resolve();
}

_deactiveDataCalls().then(() => {
  applyPendingDataSettings();
});

@@ +254,5 @@
> +  /**
> +   * nsISettingsServiceCallback
> +   */
> +  handle: function(name, result) {
> +    switch(name) {

Nit: Space between `switch` and `(`

@@ +374,5 @@
> +  classInfo: XPCOMUtils.generateCI({classID: DATACALLHANDLER_CID,
> +                                    classDescription: "Data Call Handler",
> +                                    interfaces: [Ci.nsIDataCallHandler]}),
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDataCallHandler]),

Ci.nsIDataCallInterfaceListener should be in QueryInterface list.

@@ +419,5 @@
> +            apnSetting.types.length);
> +  },
> +
> +  _convertApnType: function(apnType) {
> +    switch(apnType) {

Nit: space between `switch` and `(`.

@@ +541,5 @@
> +      this._pengingApnSettings = newApnSettings;
> +      return;
> +    }
> +
> +    if (this.deactivateDataCalls()) {

Ditto, how about putting the common codes into a utility function?

@@ +820,5 @@
> +      // If there is a cid, compare cid; otherwise it is probably an error on
> +      // data call setup.
> +      if (message.cid !== undefined) {
> +        if (message.linkInfo.cid == dataCall.linkInfo.cid) {
> +          gMobileConnectionService.notifyDataError(this.clientId, message);

DataCallManager is plan to be reused for both moz ril and partner's ril, nsIGonkMobileConnectionService is a moz-only interface,  we should not call it directly.
Maybe we could handle this in DataCallInterfaceService, something like

this.radioInterface.sendWorkerMessage("setupDataCall", options, (aResponse) => {
  if (aResponse.errorMsg) {
    ...
    gMobileConnectionService.notifyDataError(...);
  }
  .... 
};

@@ +1388,5 @@
> +    this.onSetupDataCallResult(aDataCall);
> +  },
> +
> +  notifyGetDataCallListSuccess: function(aCount, aDataCalls) {
> +    // Not implemented.

throw Cr.NS_ERROR_NOT_IMPLEMENTED ?

@@ +1524,5 @@
> +  },
> +
> +  get iccId() {
> +    let radioInterface = gRil.getRadioInterface(this.dataConnectionHandler.clientId);
> +    let iccInfo = radioInterface.rilContext.iccInfo;

Get iccInfo from nsIIccService instead.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1112,5 @@
>      return null;
>    },
>  
>    handleUnsolicitedWorkerMessage: function(message) {
> +    let connHandler = gDataCallManager.getDataCallHandler(this.clientId);

I guess you still keeps this is for handling "networkinfochanged" and "dataregistrationstatechange".
But in theory, RadioInterfaceLayer shouldn't call DataCallManager/Handler directly. Could DataCallManager/Handler monitor those status via registering listener to MobileConnectionService?

@@ +1137,5 @@
>        case "datacalllistchanged":
> +        for (let i = 0; i < message.datacalls.length; i++) {
> +          let dataCall = message.datacalls[i];
> +          // Convert pdp type into constant int value.
> +          dataCall.pdpType = RIL.RIL_DATACALL_PDP_TYPES.indexOf(dataCall.type);

Wrap dataCall to nsIDataCall.

Comment 26

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review!

(In reply to Edgar Chen [:edgar][:echen] from comment #25)
> Comment on attachment 8596432 [details] [diff] [review]
> Part 3: move data call related code to DataCallManager, v2.
> 
> Review of attachment 8596432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below, thank you.
> 
> ::: dom/system/gonk/DataCallManager.js
> @@ +1,1 @@
> > +"use strict";
> 
> Please remember to add MPL licence.

Sure, will do.

> 
> @@ +19,5 @@
> > +                                   "nsIGonkMobileConnectionService");
> > +
> > +XPCOMUtils.defineLazyServiceGetter(this, "gRil",
> > +                                   "@mozilla.org/ril;1",
> > +                                   "nsIRadioInterfaceLayer");
> 
> nsIRadioInterfaceLayer is moz-only implementation, we should not call it
> directly, because DataCallManager plans to be reused for both moz ril and
> partner's ril.

Agree, we should avoid using nsIRadioInterfaceLayer stuff.

> 
> @@ +89,5 @@
> > +
> > +function DataCallManager() {
> > +  this._connectionHandlers = [];
> > +
> > +  let numRadioInterfaces = gRil.numRadioInterfaces;
> 
> Use nsIMobileConnectionService.numItems instead.

Will do.

> 
> @@ +140,5 @@
> > +  debug: function(s) {
> > +    dump("-*- DataCallManager: " + s + "\n");
> > +  },
> > +
> > +  getDataCallHandler: function(clientId) {
> 
> It is a good chance to correct some coding style, like "a" prefix for
> arguments.

Will do.

> 
> @@ +141,5 @@
> > +    dump("-*- DataCallManager: " + s + "\n");
> > +  },
> > +
> > +  getDataCallHandler: function(clientId) {
> > +    return this._connectionHandlers[clientId];
> 
> Throw an exception if unable to get a valid DataCallHandler instance.

Will do.

> 
> @@ +192,5 @@
> > +              newSettings.enabled = true;
> > +            }
> > +            self._currentDataClientId = self._dataDefaultClientId;
> > +
> > +            newIface.setDataRegistration(true, {
> 
> How about wrapping setDataRegistration with Promise?

Sure, it will make the code clearer. Thanks.

> 
> @@ +221,5 @@
> > +      oldSettings.oldEnabled = oldSettings.enabled;
> > +      oldSettings.enabled = false;
> > +    }
> > +
> > +    if (oldConnHandler.deactivateDataCalls()) {
> 
> Since there are two places will use deactivateDataCalls, changing default
> client and changing apn setting.
> How about putting the common codes into a utility function (maybe returns a
> promise).
> 
> for example,
> 
> function _deactivateDataCalls() {
>   if (connHandler.deactivateDataCalls()) {
>     return new Promise(function(aResolve, aReject) {
>       let callback = {
>         ...
>         notifyAllDataDisconnected: function() {
>           connHandler.unRegisterListener(callback);
>           aResolve();
>         }
>       };
>       connHandler.registerListener(callback);
>     };
>   }
> 
>   return Promise.resolve();
> }
> 
> _deactiveDataCalls().then(() => {
>   applyPendingDataSettings();
> });

I think it's a good idea, I'll add it in the next version.

> 
> @@ +254,5 @@
> > +  /**
> > +   * nsISettingsServiceCallback
> > +   */
> > +  handle: function(name, result) {
> > +    switch(name) {
> 
> Nit: Space between `switch` and `(`
> 
> @@ +374,5 @@
> > +  classInfo: XPCOMUtils.generateCI({classID: DATACALLHANDLER_CID,
> > +                                    classDescription: "Data Call Handler",
> > +                                    interfaces: [Ci.nsIDataCallHandler]}),
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDataCallHandler]),
> 
> Ci.nsIDataCallInterfaceListener should be in QueryInterface list.

Will do.

> 
> @@ +419,5 @@
> > +            apnSetting.types.length);
> > +  },
> > +
> > +  _convertApnType: function(apnType) {
> > +    switch(apnType) {
> 
> Nit: space between `switch` and `(`.
> 
> @@ +541,5 @@
> > +      this._pengingApnSettings = newApnSettings;
> > +      return;
> > +    }
> > +
> > +    if (this.deactivateDataCalls()) {
> 
> Ditto, how about putting the common codes into a utility function?

Will do.

> 
> @@ +820,5 @@
> > +      // If there is a cid, compare cid; otherwise it is probably an error on
> > +      // data call setup.
> > +      if (message.cid !== undefined) {
> > +        if (message.linkInfo.cid == dataCall.linkInfo.cid) {
> > +          gMobileConnectionService.notifyDataError(this.clientId, message);
> 
> DataCallManager is plan to be reused for both moz ril and partner's ril,
> nsIGonkMobileConnectionService is a moz-only interface,  we should not call
> it directly.
> Maybe we could handle this in DataCallInterfaceService, something like
> 
> this.radioInterface.sendWorkerMessage("setupDataCall", options, (aResponse)
> => {
>   if (aResponse.errorMsg) {
>     ...
>     gMobileConnectionService.notifyDataError(...);
>   }
>   .... 
> };

We could do that, however notifyDataError was supposed to be called only for default data. In DataCallInterfaceService, we have no way to know whether the data call is the default data or not.

> 
> @@ +1388,5 @@
> > +    this.onSetupDataCallResult(aDataCall);
> > +  },
> > +
> > +  notifyGetDataCallListSuccess: function(aCount, aDataCalls) {
> > +    // Not implemented.
> 
> throw Cr.NS_ERROR_NOT_IMPLEMENTED ?

Will do.

> 
> @@ +1524,5 @@
> > +  },
> > +
> > +  get iccId() {
> > +    let radioInterface = gRil.getRadioInterface(this.dataConnectionHandler.clientId);
> > +    let iccInfo = radioInterface.rilContext.iccInfo;
> 
> Get iccInfo from nsIIccService instead.

Will do.

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1112,5 @@
> >      return null;
> >    },
> >  
> >    handleUnsolicitedWorkerMessage: function(message) {
> > +    let connHandler = gDataCallManager.getDataCallHandler(this.clientId);
> 
> I guess you still keeps this is for handling "networkinfochanged" and
> "dataregistrationstatechange".
> But in theory, RadioInterfaceLayer shouldn't call DataCallManager/Handler
> directly. Could DataCallManager/Handler monitor those status via registering
> listener to MobileConnectionService?

We could do that but I thought DataCallManager is suppose to let other modules use data call functions easily. I'm missing why RadioInterfaceLayer shouldn't call DataCallManager/Handler directly, isn't this the same as RadioInterfaceLayer calling other modules to notify about unsolicited updates?

> 
> @@ +1137,5 @@
> >        case "datacalllistchanged":
> > +        for (let i = 0; i < message.datacalls.length; i++) {
> > +          let dataCall = message.datacalls[i];
> > +          // Convert pdp type into constant int value.
> > +          dataCall.pdpType = RIL.RIL_DATACALL_PDP_TYPES.indexOf(dataCall.type);
> 
> Wrap dataCall to nsIDataCall.

Will do.

Comment 27

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> Thanks Edgar for the review!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #25)
> > Comment on attachment 8596432 [details] [diff] [review]
> > Part 3: move data call related code to DataCallManager, v2.
> > 
> > Review of attachment 8596432 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please see my comments below, thank you.
> > 
> > ::: dom/system/gonk/DataCallManager.js
> > @@ +820,5 @@
> > > +      // If there is a cid, compare cid; otherwise it is probably an error on
> > > +      // data call setup.
> > > +      if (message.cid !== undefined) {
> > > +        if (message.linkInfo.cid == dataCall.linkInfo.cid) {
> > > +          gMobileConnectionService.notifyDataError(this.clientId, message);
> > 
> > DataCallManager is plan to be reused for both moz ril and partner's ril,
> > nsIGonkMobileConnectionService is a moz-only interface,  we should not call
> > it directly.
> > Maybe we could handle this in DataCallInterfaceService, something like
> > 
> > this.radioInterface.sendWorkerMessage("setupDataCall", options, (aResponse)
> > => {
> >   if (aResponse.errorMsg) {
> >     ...
> >     gMobileConnectionService.notifyDataError(...);
> >   }
> >   .... 
> > };
> 
> We could do that, however notifyDataError was supposed to be called only for
> default data. In DataCallInterfaceService, we have no way to know whether
> the data call is the default data or not.

Hmm, you are right, DataCallInterfaceService doesn't know whether the `setupDataCall` request is for default data or not. However, calling moz-only interface in a common module isn't good, it makes common module has direct dependence with moz-only implementation. We still need to find a proper way to handle this event, though I haven't other idea now.

Comment 28

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #27)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> > Thanks Edgar for the review!
> > 
> > (In reply to Edgar Chen [:edgar][:echen] from comment #25)
> > > Comment on attachment 8596432 [details] [diff] [review]
> > > Part 3: move data call related code to DataCallManager, v2.
> > > 
> > > Review of attachment 8596432 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Please see my comments below, thank you.
> > > 
> > > ::: dom/system/gonk/DataCallManager.js
> > > @@ +820,5 @@
> > > > +      // If there is a cid, compare cid; otherwise it is probably an error on
> > > > +      // data call setup.
> > > > +      if (message.cid !== undefined) {
> > > > +        if (message.linkInfo.cid == dataCall.linkInfo.cid) {
> > > > +          gMobileConnectionService.notifyDataError(this.clientId, message);
> > > 
> > > DataCallManager is plan to be reused for both moz ril and partner's ril,
> > > nsIGonkMobileConnectionService is a moz-only interface,  we should not call
> > > it directly.
> > > Maybe we could handle this in DataCallInterfaceService, something like
> > > 
> > > this.radioInterface.sendWorkerMessage("setupDataCall", options, (aResponse)
> > > => {
> > >   if (aResponse.errorMsg) {
> > >     ...
> > >     gMobileConnectionService.notifyDataError(...);
> > >   }
> > >   .... 
> > > };
> > 
> > We could do that, however notifyDataError was supposed to be called only for
> > default data. In DataCallInterfaceService, we have no way to know whether
> > the data call is the default data or not.
> 
> Hmm, you are right, DataCallInterfaceService doesn't know whether the
> `setupDataCall` request is for default data or not. However, calling
> moz-only interface in a common module isn't good, it makes common module has
> direct dependence with moz-only implementation. We still need to find a
> proper way to handle this event, though I haven't other idea now.

We can let MobileConnection register to nsIDataCallHandler listener, and add a "notifyDataError" in nsIDataCallListener. What do you think?

Comment 29

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> (In reply to Edgar Chen [:edgar][:echen] from comment #25)
> > Comment on attachment 8596432 [details] [diff] [review]
> > Part 3: move data call related code to DataCallManager, v2.
> > 
> > Review of attachment 8596432 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +1112,5 @@
> > >      return null;
> > >    },
> > >  
> > >    handleUnsolicitedWorkerMessage: function(message) {
> > > +    let connHandler = gDataCallManager.getDataCallHandler(this.clientId);
> > 
> > I guess you still keeps this is for handling "networkinfochanged" and
> > "dataregistrationstatechange".
> > But in theory, RadioInterfaceLayer shouldn't call DataCallManager/Handler
> > directly. Could DataCallManager/Handler monitor those status via registering
> > listener to MobileConnectionService?
> 
> We could do that but I thought DataCallManager is suppose to let other
> modules use data call functions easily. I'm missing why RadioInterfaceLayer
> shouldn't call DataCallManager/Handler directly, isn't this the same as
> RadioInterfaceLayer calling other modules to notify about unsolicited
> updates?

Not exactly the same. For other modules, RadioInterfaceLayer calls nsIGonkFooService.notifyBar() to notify about unsolicited updates, they are both moz-only interface/implementation, it's fine. But DataCallManager/Handler is a common module now, calling it directly creates some dependence between RIL code and the reset of gecko code. Besides, |updateRILNetworkInterface| will be deprecated after bug 911713, it will be good if we could use it as less as possible.

Comment 30

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #29)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #26)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #25)
> > > Comment on attachment 8596432 [details] [diff] [review]
> > > Part 3: move data call related code to DataCallManager, v2.
> > > 
> > > Review of attachment 8596432 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > ::: dom/system/gonk/RadioInterfaceLayer.js
> > > @@ +1112,5 @@
> > > >      return null;
> > > >    },
> > > >  
> > > >    handleUnsolicitedWorkerMessage: function(message) {
> > > > +    let connHandler = gDataCallManager.getDataCallHandler(this.clientId);
> > > 
> > > I guess you still keeps this is for handling "networkinfochanged" and
> > > "dataregistrationstatechange".
> > > But in theory, RadioInterfaceLayer shouldn't call DataCallManager/Handler
> > > directly. Could DataCallManager/Handler monitor those status via registering
> > > listener to MobileConnectionService?
> > 
> > We could do that but I thought DataCallManager is suppose to let other
> > modules use data call functions easily. I'm missing why RadioInterfaceLayer
> > shouldn't call DataCallManager/Handler directly, isn't this the same as
> > RadioInterfaceLayer calling other modules to notify about unsolicited
> > updates?
> 
> Not exactly the same. For other modules, RadioInterfaceLayer calls
> nsIGonkFooService.notifyBar() to notify about unsolicited updates, they are
> both moz-only interface/implementation, it's fine. But
> DataCallManager/Handler is a common module now, calling it directly creates
> some dependence between RIL code and the reset of gecko code. Besides,
> |updateRILNetworkInterface| will be deprecated after bug 911713, it will be
> good if we could use it as less as possible.

I get it now, thanks!

Comment 31

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8595897 [details] [diff] [review]
Part 1: interface changes, v3.

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

Thank you for the effort :)

Comment 32

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes. r=hsinyi,echen

Addressed review comment 23.

Comment 33

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: DataCallInterfaceService impl, v2.

Edgar, could you take a look again? Thanks.

Addressed review comment 24:
- Use nsIMobileconnectionService instead of nsIGonkMobileConnectionService
- Have 'a' prefix for arguments
- notifyError with aResponse.errorMsg
- Wrap response into a nsIDataCall object
- Get the correct attribute in getDataCallList response

Comment 34

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: move data call related code to DataCallManager, v3.

Edgar, could you take a look again? Thanks.

Major changes compared to v2:
- Get rid of nsIRadioInterfaceLayer and use nsIMobileConnectionService and nsIIccService instead
- Have 'a' prefix for argument
- Wrap deactivateDataCalls into a utility function returning a promise
- Wrap setDataRegistration into a utility function returning a promise
- Let DataCallManager monitor data registration state by registering listener to MobileConnectionService
- Use notifyObservers to notify MobileConnectionService about default data error
- Wrap response into nsIDataCall object

Comment 35

[Edgar Chen [:edgar]]

Comment on attachment 8603128 [details] [diff] [review]
Part 2: DataCallInterfaceService impl, v2.

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

Mostly looks good, but I would like to review a revised version again. Thank you.

::: dom/system/gonk/DataCallInterfaceService.js
@@ +44,5 @@
> +    debugPref = Services.prefs.getBoolPref(PREF_RIL_DEBUG_ENABLED);
> +  } catch (e) {
> +    debugPref = false;
> +  }
> +  DEBUG = RIL.DEBUG_RIL || debugPref;

Nit: DEBUG = debugPref || RIL.DEBUG_RIL;

@@ +68,5 @@
> +  cid: -1,
> +  active: -1,
> +  pdpType: -1,
> +  ifname: null,
> +  addreses: [],

address: null

The address is defined as a string type in idl.

@@ +69,5 @@
> +  active: -1,
> +  pdpType: -1,
> +  ifname: null,
> +  addreses: [],
> +  dnses: [],

Ditto.

@@ +70,5 @@
> +  pdpType: -1,
> +  ifname: null,
> +  addreses: [],
> +  dnses: [],
> +  gateways: []

Ditto.

@@ +158,5 @@
> +  debug: function(aMessage) {
> +    dump("-*- DataCallInterface[" + this.clientId + "]: " + aMessage + "\n");
> +  },
> +
> +  clientId: -1,

s/clientId/_clientId/

@@ +160,5 @@
> +  },
> +
> +  clientId: -1,
> +
> +  radioInterface: null,

s/radioInterface/_radioInterface/

@@ +213,5 @@
> +        if (aResponse.errorMsg) {
> +          aCallback.notifyError(aResponse.errorMsg);
> +        } else {
> +          let dataCalls = [];
> +          for (let i = 0; i < aResponse.length; i++) {

aResponse.datacalls.length?

Or use for .. of

for (let datacall of aResponse.datacalls) { ..

@@ +225,5 @@
> +  setDataRegistration: function(aAttach, aCallback) {
> +    this.radioInterface.sendWorkerMessage("setDataRegistration", {
> +      attach: aAttach
> +    }, (aResponse) => {
> +      if (!aCallback) {

Since aCallback is not a optional argument in idl, it seems we don't need to have this check.

Comment 36

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar!

(In reply to Edgar Chen [:edgar][:echen] from comment #35)
> Comment on attachment 8603128 [details] [diff] [review]
> Part 2: DataCallInterfaceService impl, v2.
> 
> Review of attachment 8603128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good, but I would like to review a revised version again. Thank
> you.
> 
> ::: dom/system/gonk/DataCallInterfaceService.js
> @@ +44,5 @@
> > +    debugPref = Services.prefs.getBoolPref(PREF_RIL_DEBUG_ENABLED);
> > +  } catch (e) {
> > +    debugPref = false;
> > +  }
> > +  DEBUG = RIL.DEBUG_RIL || debugPref;
> 
> Nit: DEBUG = debugPref || RIL.DEBUG_RIL;
> 
> @@ +68,5 @@
> > +  cid: -1,
> > +  active: -1,
> > +  pdpType: -1,
> > +  ifname: null,
> > +  addreses: [],
> 
> address: null
> 
> The address is defined as a string type in idl.
> 
> @@ +69,5 @@
> > +  active: -1,
> > +  pdpType: -1,
> > +  ifname: null,
> > +  addreses: [],
> > +  dnses: [],
> 
> Ditto.
> 
> @@ +70,5 @@
> > +  pdpType: -1,
> > +  ifname: null,
> > +  addreses: [],
> > +  dnses: [],
> > +  gateways: []
> 
> Ditto.

Agrrr, sorry for these careless mistakes.

> 
> @@ +158,5 @@
> > +  debug: function(aMessage) {
> > +    dump("-*- DataCallInterface[" + this.clientId + "]: " + aMessage + "\n");
> > +  },
> > +
> > +  clientId: -1,
> 
> s/clientId/_clientId/

Will do.

> 
> @@ +160,5 @@
> > +  },
> > +
> > +  clientId: -1,
> > +
> > +  radioInterface: null,
> 
> s/radioInterface/_radioInterface/

Will do.

> 
> @@ +213,5 @@
> > +        if (aResponse.errorMsg) {
> > +          aCallback.notifyError(aResponse.errorMsg);
> > +        } else {
> > +          let dataCalls = [];
> > +          for (let i = 0; i < aResponse.length; i++) {
> 
> aResponse.datacalls.length?
> 
> Or use for .. of
> 
> for (let datacall of aResponse.datacalls) { ..

I am thinking in using aResponse.datacalls.map() instead...

> 
> @@ +225,5 @@
> > +  setDataRegistration: function(aAttach, aCallback) {
> > +    this.radioInterface.sendWorkerMessage("setDataRegistration", {
> > +      attach: aAttach
> > +    }, (aResponse) => {
> > +      if (!aCallback) {
> 
> Since aCallback is not a optional argument in idl, it seems we don't need to
> have this check.

Actually, it is set as optional now, but I guess we don't need the optional anymore, I'll get rid of it.

Comment 37

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #36)
> (In reply to Edgar Chen [:edgar][:echen] from comment #35)
> > Comment on attachment 8603128 [details] [diff] [review]
> > Part 2: DataCallInterfaceService impl, v2.
> > 
> > Review of attachment 8603128 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/system/gonk/DataCallInterfaceService.js
> > 
> > @@ +225,5 @@
> > > +  setDataRegistration: function(aAttach, aCallback) {
> > > +    this.radioInterface.sendWorkerMessage("setDataRegistration", {
> > > +      attach: aAttach
> > > +    }, (aResponse) => {
> > > +      if (!aCallback) {
> > 
> > Since aCallback is not a optional argument in idl, it seems we don't need to
> > have this check.
> 
> Actually, it is set as optional now, but I guess we don't need the optional
> anymore, I'll get rid of it.

You are right, it is optional now actually. And I am fine with either way (having a check for aCallback or getting ril of optional). :)

Comment 38

[Edgar Chen [:edgar]]

Comment on attachment 8603133 [details] [diff] [review]
Part 3: move data call related code to DataCallManager, v3.

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

Please see my comments, thank you.

::: dom/system/gonk/DataCallManager.js
@@ +87,5 @@
> +    debugPref = Services.prefs.getBoolPref(PREF_RIL_DEBUG_ENABLED);
> +  } catch (e) {
> +    debugPref = false;
> +  }
> +  DEBUG = RIL.DEBUG_RIL || debugPref;

Nit: move the fall-back value backward, e.g. DEBUG = debugPref || RIL.DEBUG_RIL;

@@ +161,5 @@
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsIDataCallCallback]),
> +        notifySuccess: function() {
> +          aResolve();
> +        },
> +        notifyError: function(aResponse) {

s/aResponse/aErrorMsg/

@@ +195,5 @@
> +      return;
> +    }
> +
> +    let oldConnHandler = this._connectionHandlers[this._currentDataClientId];
> +    let oldIface = oldConnHandler.dataCallInterface;

Some places use `dataConnectionHandler.dataCallInterface` and some use `dataConnectionHandler.getDataCallInterface()` to get DataCallInterface.
Please have a consistent style.

@@ +417,5 @@
> +    this.dataCallinterface.unregisterListener(this);
> +    this.dataCallInterface = null;
> +  },
> +
> +  getDataCallInterface: function() {

Some places `dataConnectionHandler.dataCallInterface` and some use `dataConnectionHandler.getDataCallInterface()` to get DataCallInterface.
Please have a consistent style.

@@ +554,5 @@
> +            this.unregisterListener(callback);
> +            aResolve();
> +          }
> +        };
> +        this.registerListener(callback);

Is there anyone else will register `notifyAllDataDisconnected` listener?
If no, it seems only be used internally, then we probably don't need to define the listener in idl.

@@ +1197,5 @@
> +  },
> +
> +  // Helpers
> +
> +  debug: function(s) {

s/s/aMsg/

@@ +1409,5 @@
> +        // the state probably was notified already or need not to be notified.
> +        if (aNetworkInterface.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED) {
> +          aNetworkInterface.notifyRILNetworkInterface();
> +          // Clear link info after notifying NetworkManager.
> +          this.resetLinkInfo();

Remember to include the fix of bug 1162865.

@@ +1448,5 @@
> +    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> +  },
> +
> +  notifySuccess: function() {
> +    if (this.state === NETWORK_STATE_DISCONNECTING) {

Is there any potential racing problem (e.g. state was changed before notifySuccess is fired)

@@ +1458,5 @@
> +    }
> +  },
> +
> +  notifyError: function(aErrorMsg) {
> +    if (this.state === NETWORK_STATE_CONNECTING) {

Ditto.

@@ +1460,5 @@
> +
> +  notifyError: function(aErrorMsg) {
> +    if (this.state === NETWORK_STATE_CONNECTING) {
> +      this.onSetupDataCallResult({errorMsg: aErrorMsg});
> +    } else if (this.state === NETWORK_STATE_DISCONNECTING) {

Ditto.
And is it possible don't share the callback among requests?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +643,5 @@
> +  pdpType: -1,
> +  ifname: null,
> +  addreses: [],
> +  dnses: [],
> +  gateways: []

The addresses, dnses and gateways are string type actually.

Comment 39

[Jessica Jong [:jessica] (inactive)]

Thanks for the review effort, Edgar.

(In reply to Edgar Chen [:edgar][:echen] from comment #38)
> Comment on attachment 8603133 [details] [diff] [review]
> Part 3: move data call related code to DataCallManager, v3.
> 
> Review of attachment 8603133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments, thank you.
> 
> ::: dom/system/gonk/DataCallManager.js
> @@ +87,5 @@
> > +    debugPref = Services.prefs.getBoolPref(PREF_RIL_DEBUG_ENABLED);
> > +  } catch (e) {
> > +    debugPref = false;
> > +  }
> > +  DEBUG = RIL.DEBUG_RIL || debugPref;
> 
> Nit: move the fall-back value backward, e.g. DEBUG = debugPref ||
> RIL.DEBUG_RIL;

Will do.

> 
> @@ +161,5 @@
> > +        QueryInterface: XPCOMUtils.generateQI([Ci.nsIDataCallCallback]),
> > +        notifySuccess: function() {
> > +          aResolve();
> > +        },
> > +        notifyError: function(aResponse) {
> 
> s/aResponse/aErrorMsg/

Will do.

> 
> @@ +195,5 @@
> > +      return;
> > +    }
> > +
> > +    let oldConnHandler = this._connectionHandlers[this._currentDataClientId];
> > +    let oldIface = oldConnHandler.dataCallInterface;
> 
> Some places use `dataConnectionHandler.dataCallInterface` and some use
> `dataConnectionHandler.getDataCallInterface()` to get DataCallInterface.
> Please have a consistent style.

Will do.

> 
> @@ +417,5 @@
> > +    this.dataCallinterface.unregisterListener(this);
> > +    this.dataCallInterface = null;
> > +  },
> > +
> > +  getDataCallInterface: function() {
> 
> Some places `dataConnectionHandler.dataCallInterface` and some use
> `dataConnectionHandler.getDataCallInterface()` to get DataCallInterface.
> Please have a consistent style.

Will do.

> 
> @@ +554,5 @@
> > +            this.unregisterListener(callback);
> > +            aResolve();
> > +          }
> > +        };
> > +        this.registerListener(callback);
> 
> Is there anyone else will register `notifyAllDataDisconnected` listener?
> If no, it seems only be used internally, then we probably don't need to
> define the listener in idl.

Yes, gRadioEnabledController listens to this to proceed with radio disable.

> 
> @@ +1197,5 @@
> > +  },
> > +
> > +  // Helpers
> > +
> > +  debug: function(s) {
> 
> s/s/aMsg/

Will do.

> 
> @@ +1409,5 @@
> > +        // the state probably was notified already or need not to be notified.
> > +        if (aNetworkInterface.state == RIL.GECKO_NETWORK_STATE_DISCONNECTED) {
> > +          aNetworkInterface.notifyRILNetworkInterface();
> > +          // Clear link info after notifying NetworkManager.
> > +          this.resetLinkInfo();
> 
> Remember to include the fix of bug 1162865.

Thanks for the remind :)

> 
> @@ +1448,5 @@
> > +    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> > +  },
> > +
> > +  notifySuccess: function() {
> > +    if (this.state === NETWORK_STATE_DISCONNECTING) {
> 
> Is there any potential racing problem (e.g. state was changed before
> notifySuccess is fired)

Not that I know, this is just to make sure we are calling the right function. Maybe we should not share callback among requests, as mentioned below, I'll fix this in the next version. Thanks.

> 
> @@ +1458,5 @@
> > +    }
> > +  },
> > +
> > +  notifyError: function(aErrorMsg) {
> > +    if (this.state === NETWORK_STATE_CONNECTING) {
> 
> Ditto.
> 
> @@ +1460,5 @@
> > +
> > +  notifyError: function(aErrorMsg) {
> > +    if (this.state === NETWORK_STATE_CONNECTING) {
> > +      this.onSetupDataCallResult({errorMsg: aErrorMsg});
> > +    } else if (this.state === NETWORK_STATE_DISCONNECTING) {
> 
> Ditto.
> And is it possible don't share the callback among requests?
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +643,5 @@
> > +  pdpType: -1,
> > +  ifname: null,
> > +  addreses: [],
> > +  dnses: [],
> > +  gateways: []
> 
> The addresses, dnses and gateways are string type actually.

Will fix these in the next version, thanks.

Comment 40

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #39)
> (In reply to Edgar Chen [:edgar][:echen] from comment #38)
> > Comment on attachment 8603133 [details] [diff] [review]
> > Part 3: move data call related code to DataCallManager, v3.
> > 
> > Review of attachment 8603133 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/DataCallManager.js
> > @@ +554,5 @@
> > > +            this.unregisterListener(callback);
> > > +            aResolve();
> > > +          }
> > > +        };
> > > +        this.registerListener(callback);
> > 
> > Is there anyone else will register `notifyAllDataDisconnected` listener?
> > If no, it seems only be used internally, then we probably don't need to
> > define the listener in idl.
> 
> Yes, gRadioEnabledController listens to this to proceed with radio disable.

Oh, I missed that one. :p

Comment 41

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v4.

Per offline discussion, I've changed the interface a little.
- Add a callback for deactivateDataCalls(), so that registerListener/unregisterListener can be removed in nsIDataCallManager
- Make the callback of setDataRegistration() as _not_ optional
- Add some more comments for nsIDataCallInterfaceService functions

Edgar, since some interfaces are changed, could you review it again? Thanks.

Comment 42

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: DataCallInterfaceService impl, v3.

Edgar, I've addressed the review comments in comment 35, could you take a look again? Thanks.

Comment 43

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: move data call related code to DataCallManager, v4.

Addressed review comment 38, major changes include:
- remove getDataCallInterface() function and use DataConnectionHandler.dataCallInterface directly
- process callback for deactivateDataCalls()
- not share callbacks among requests, e.g. have a dedicated callback for setupDataCall()

Edgar, do you mind reviewing again? Thanks.

Comment 44

[Edgar Chen [:edgar]]

Comment on attachment 8605080 [details] [diff] [review]
Part 1: interface changes, v4.

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

r=me with comment addressed, thank you.

::: dom/system/gonk/nsIDataCallManager.idl
@@ +22,5 @@
> +{
> +  /**
> +   * Callback function used to notify when all data calls are disconnected.
> +   */
> +  void notifyDataCallsDisconnected();

Since there is only one function in this interface, you could add `function` attribute, e.g. [..., function, ...]
Then in javascript implementation, you could simply write as

DataCallHandler.deactivateDataCalls(function() {
 ....
});

Comment 45

[Edgar Chen [:edgar]]

Comment on attachment 8605081 [details] [diff] [review]
Part 2: DataCallInterfaceService impl, v3.

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

Perfect!! Thank you.

Comment 46

[Edgar Chen [:edgar]]

Comment on attachment 8605103 [details] [diff] [review]
Part 3: move data call related code to DataCallManager, v4.

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

Nice!! And I am going to do some basic test and give r+ then.
Thanks for the effort, Jessica.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +559,5 @@
>        return function() {
>          let deferred = _deactivatingDeferred[clientId] = Promise.defer();
> +        let dataCallHandler = gDataCallManager.getDataCallHandler(clientId);
> +
> +        dataCallHandler.deactivateDataCalls({

Add 'function' attribute in nsIDeactivateDataCallsCallback (see comment #44), then you could simply write as,

dataCallHandler.deactivateDataCalls(function() {
  deferred.resolve();
});

Comment 47

[Edgar Chen [:edgar]]

Comment on attachment 8605103 [details] [diff] [review]
Part 3: move data call related code to DataCallManager, v4.

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

I did some basic test, it works perfect. Thank you.

::: dom/system/gonk/DataCallManager.js
@@ +1060,5 @@
> +        if (DEBUG) this.debug("This DataCall is not requested anymore.");
> +        return;
> +      }
> +
> +      // Let DataConnectionHandler notify MobileConnectionService

s/DataConnectionHandler/DataCallHandler/

@@ +1092,5 @@
> +    this.linkInfo.gateways = aDataCall.gateways ? aDataCall.gateways.split(" ") : [];
> +    this.linkInfo.dnses = aDataCall.dnses ? aDataCall.dnses.split(" ") : [];
> +    this.state = this._getGeckoDataCallState(aDataCall);
> +
> +    // Notify DataConnectionHandler about data call connected.

Ditto

@@ +1114,5 @@
> +      this.setup();
> +      return;
> +    }
> +
> +    // Notify DataConnectionHandler about data call disconnected.

Ditto

@@ +1194,5 @@
> +    }
> +
> +    this.state = dataCallState;
> +
> +    // Notify DataConnectionHandler about data call changed.

Ditto

@@ +1477,5 @@
> +    }
> +  }
> +};
> +
> +function RILNetworkInterface(aDataConnectionHandler, aType, aApnSetting, aDataCall) {

s/aDataConnectionHandler/aDataCallHandler/

@@ +1482,5 @@
> +  if (!aDataCall) {
> +    throw new Error("No dataCall for RILNetworkInterface: " + type);
> +  }
> +
> +  this.dataConnectionHandler = aDataConnectionHandler;

Ditto, s/dataConnectionHandler/dataCallHandler/

Comment 48

[Jessica Jong [:jessica] (inactive)]

Yay! Thanks a lot, Edgar. :)

Will address the review comments and upload the latest patches.

Comment 49

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes. r=hsinyi,echen

Comment 50

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: DataCallInterfaceService impl. r=echen

Comment 51

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: move data call related code to DataCallManager. r=echen

Comment 52

[Jessica Jong [:jessica] (inactive)]

try looks good!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00544609aa74

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/60d63339c5dc
https://hg.mozilla.org/integration/b2g-inbound/rev/0761a99f4191
https://hg.mozilla.org/integration/b2g-inbound/rev/5eb1458333c8

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60d63339c5dc
https://hg.mozilla.org/mozilla-central/rev/0761a99f4191
https://hg.mozilla.org/mozilla-central/rev/5eb1458333c8