905568 - B2G RIL: Handling data connection in a separated module

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Currently data connection related things are handled in RadioInterfaceLayer, like monitor apn setting, handle shared apn type connection, communicate with NetworkManager .. etc. To improve modulation, we could move those handling into a separated module, something like DataConnectionManager. And RadioInterfaceLayer only needs to handle the request of active/de-active data connection and response the result.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: WIP

Comment 2

[Edgar Chen [:edgar]]

Steal ... :)

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Add DataConnectionManager

Comment 4

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: Monitor ril.data.apnSettings in DataConnectionManager

Comment 5

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-1: Move apnSettings

Comment 6

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-2: Move DataCallCallback

Comment 7

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-3: Handle WorkerMessage

Comment 8

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-4: Move getDataCallStateByType

Comment 9

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-4: Move getDataCallStateByType, v2

Comment 10

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-5: Move {setup|deactive}DataCallByType, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-6: Move deactiveDataCalls, v1

Comment 12

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2-7: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v1

Comment 13

[Edgar Chen [:edgar]]

Summarize what I am going to do in this bug:
1). Consider Multiple sim situation, I would like to create below two moudles,
    - DataConnectionHandler: Each instance maps to different radioInterface. Taking care about share apn,
                             networkInterface registration, activate/deactivate data call ....
    - DataConnectionManager: Manage DataConnectionHandlers and monitor setting's change. (ex, ril.data.apnSettings,
                             ...)
2). Move data call related code to DataConnectionHandler/DataConnectionManager.
3). To minimize the risk, I will keep the original logic as much as possible.
    (For improvement, I would like to do it in other bugs)
4). For some interface is already used by other modules, like [1][2][3] .., I will still keep these interfaces
    until we come out a good solution in NetworkManager enhancement.

[1] setupDataCallByType(): http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIRadioInterfaceLayer.idl?from=nsIRadioInterfaceLayer.idl#108
[2] deactivateDataCallByType(): http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIRadioInterfaceLayer.idl?from=nsIRadioInterfaceLayer.idl#109
[3] getDataCallStateByType(): http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIRadioInterfaceLayer.idl?from=nsIRadioInterfaceLayer.idl#110

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 1: Add DataConnectionManager and DataConnectionHandler, v2

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v2

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v2

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 2-3: Move apnSettings, v2

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Move DataCallCallback, v2

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Handle WorkerMessage, v2

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 3-3: Move {get|setup|deactive}DataCallByType, v3

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 3-4: Move deactiveDataCalls, v2

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 3-5: Move updateRILNetworkInterface, v1

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 4: RILNetworkInterface, v1

Comment 24

[Edgar Chen [:edgar]]

Some codes and logic are changed in bug 943191, need to rebase.

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 1: Add DataConnectionManager and DataConnectionHandler, v3

Rebase

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v3

Rebase

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v3

Rebase

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 2-3: Move apnSettings, v3

Rebase

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Move DataCallCallback, v3

Rebase

Comment 30

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Handle WorkerMessage, v3

Rebase

Comment 31

[Edgar Chen [:edgar]]

Attachment: Part 3-3: Move {get|setup|deactive}DataCallByType, v4

Rebase

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 3-4: Move deactiveDataCalls, v3

Rebase

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 3-5: Move updateRILNetworkInterface, v2

Rebase

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 4: Modify RILNetworkInterface, v2

Rebase

Comment 35

[Edgar Chen [:edgar]]

Before requesting review, I would like to leave some notes about these patches,
1). For the detailed information about what I am going to do in this bug, please see comment #13.
2). I separated the changes into several patches to make it easier to review them.
3). I also did below tests:
    - Enable/Disable data call by setting.
    - Enable/Disable radio with data call enabled.
    - Enable/Disable wifi with data call enabled.
    - Send/Receive MMS with active network is wifi.
    - Send/Receive MMS with active network is mobile.
    - Update Apn Settings.
4). After r+, I will squash part 2-1, 2-2, 2-3 together
5). After r+, I will squash part 3-1, 3-2, 3-3, 3-4, 3-5 together
6). Thanks reviewer in advance :)

Comment 36

[Edgar Chen [:edgar]]

Comment on attachment 8356504 [details] [diff] [review]
Part 1: Add DataConnectionManager and DataConnectionHandler, v3

Please see comment #35.

Comment 37

[Edgar Chen [:edgar]]

Comment on attachment 8356506 [details] [diff] [review]
Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v3

Please see comment #35.

Comment 38

[Edgar Chen [:edgar]]

Comment on attachment 8356508 [details] [diff] [review]
Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v3

Please see comment #35.

Comment 39

[Edgar Chen [:edgar]]

Comment on attachment 8356510 [details] [diff] [review]
Part 2-3: Move apnSettings, v3

Please see comment #35.

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8356513 [details] [diff] [review]
Part 3-1: Move DataCallCallback, v3

Please see comment #35.

Comment 41

[Edgar Chen [:edgar]]

Comment on attachment 8356514 [details] [diff] [review]
Part 3-2: Handle WorkerMessage, v3

Please see comment #35.

Comment 42

[Edgar Chen [:edgar]]

Comment on attachment 8356515 [details] [diff] [review]
Part 3-3: Move {get|setup|deactive}DataCallByType, v4

Please see comment #35.

Comment 43

[Edgar Chen [:edgar]]

Comment on attachment 8356516 [details] [diff] [review]
Part 3-4: Move deactiveDataCalls, v3

Please see comment #35.

Comment 44

[Edgar Chen [:edgar]]

Comment on attachment 8356517 [details] [diff] [review]
Part 3-5: Move updateRILNetworkInterface, v2

Please see comment #35.

Comment 45

[Edgar Chen [:edgar]]

Comment on attachment 8356518 [details] [diff] [review]
Part 4: Modify RILNetworkInterface, v2

Please see comment #35.

Comment 46

[Edgar Chen [:edgar]]

It seems that bug 956255 will be landed soon.
I guess I need to rebase for it. :p

Comment 47

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

Comment on attachment 8356504 [details] [diff] [review]
Part 1: Add DataConnectionManager and DataConnectionHandler, v3

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

It looks amazing! Thanks.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +628,5 @@
> +
> +      Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> +    },
> +
> +    getConnectionHandler: function(clientId) {

I am feeling that if we'd want to take this chance to follow the mozilla coding style, i.e. with prefix "a" for argument, e.g. aClientId in this case.

We are going to have several variables in a function in some following parts. It might help improve readability within the prefix. What do you think about this?

Comment 48

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

Comment on attachment 8356506 [details] [diff] [review]
Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v3

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

Good!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +656,5 @@
> +          if (DEBUG) {
> +            this.debug("'ril.data.apnSettings' is now " +
> +                       JSON.stringify(result));
> +          }
> +          if (result) {

bail-out earlier:

if (!result) {
    break;
}

Comment 49

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

Comment on attachment 8356508 [details] [diff] [review]
Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v3

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

Please see my last question, thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +670,5 @@
> +
> +      if (this._currentDataClientId == -1) {
> +        // This is to handle boot up stage.
> +        this._currentDataClientId = this._dataDefaultClientId;
> +        let connectionHandler = this._connectionHandlers[this._currentDataClientId];

nit: how about s/connectionHandler/connHandler/?  connectionHandler is really loooong ;) It's a nit, so I am fine with either but still want to provide my opinion. :)

@@ +684,5 @@
> +        }
> +        return;
> +      }
> +
> +      let oldConnectionHandler = this._connectionHandlers[this._currentDataClientId];

ditto.

@@ +685,5 @@
> +        return;
> +      }
> +
> +      let oldConnectionHandler = this._connectionHandlers[this._currentDataClientId];
> +      let oldRadioInterface = oldConnectionHandler.radioInterface;

nit: how about s/oldRadioInterface/oldIface/?

@@ +686,5 @@
> +      }
> +
> +      let oldConnectionHandler = this._connectionHandlers[this._currentDataClientId];
> +      let oldRadioInterface = oldConnectionHandler.radioInterface;
> +      let oldDataCallSettings = oldConnectionHandler.dataCallSettings;

nit: how about s/oldDataCallSettings/oldSettings/?

@@ +687,5 @@
> +
> +      let oldConnectionHandler = this._connectionHandlers[this._currentDataClientId];
> +      let oldRadioInterface = oldConnectionHandler.radioInterface;
> +      let oldDataCallSettings = oldConnectionHandler.dataCallSettings;
> +      let newConnectionHandler = this._connectionHandlers[this._dataDefaultClientId];

ditto.

@@ +688,5 @@
> +      let oldConnectionHandler = this._connectionHandlers[this._currentDataClientId];
> +      let oldRadioInterface = oldConnectionHandler.radioInterface;
> +      let oldDataCallSettings = oldConnectionHandler.dataCallSettings;
> +      let newConnectionHandler = this._connectionHandlers[this._dataDefaultClientId];
> +      let newRadioInterface = newConnectionHandler.radioInterface;

ditto.

@@ +689,5 @@
> +      let oldRadioInterface = oldConnectionHandler.radioInterface;
> +      let oldDataCallSettings = oldConnectionHandler.dataCallSettings;
> +      let newConnectionHandler = this._connectionHandlers[this._dataDefaultClientId];
> +      let newRadioInterface = newConnectionHandler.radioInterface;
> +      let newDataCallSettings = newConnectionHandler.dataCallSettings;

ditto.

@@ +785,5 @@
> +            // We haven't got the default id for data from db.
> +            break;
> +          }
> +
> +          let connectionHandler = this._connectionHandlers[this._dataDefaultClientId];

ditto.

@@ +798,5 @@
> +          }
> +          for (let connectionHandler of this._connectionHandlers) {
> +            let dataCallSettings = connectionHandler.dataCallSettings;
> +            dataCallSettings.roamingEnabled = result;
> +            connectionHandler.updateRILNetworkInterface();

Do we really need to updateRILNetworkInterface() on every connectionHandler? I thoughy we need that only for _dataDefaultCleintId, though I would say here is a bug for DSDS... we need to separate roaming preference for each client.

Comment 50

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

Comment on attachment 8356510 [details] [diff] [review]
Part 2-3: Move apnSettings, v3

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

Thank you, thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +973,5 @@
> +            apnSetting.types.length);
> +  },
> +
> +  /**
> +   * Check is there any activated data connection.

nit: /is there/if there is/

@@ +1035,5 @@
> +      if (!this.apnSettings.byApn[apnKey]) {
> +        this.apnSettings.byApn[apnKey] = inputApnSetting;
> +      } else {
> +        this.apnSettings.byApn[apnKey].types =
> +          this.apnSettings.byApn[apnKey].types.concat(inputApnSetting.types);

We might have duplicate 'type' in .types[] after concat(). However, the cost seems larger than benefit if we attempt to remove duplicates. So, looks okay here.

Comment 51

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

Comment on attachment 8356513 [details] [diff] [review]
Part 3-1: Move DataCallCallback, v3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +929,5 @@
>      oldEnabled: false,
>      enabled: false,
>      roamingEnabled: false
>    };
> +  this._datacall_callbacks = [];

It's a good chance to make the coding style more consistent, i.e. removing underlines. Let's rename it dataCallbacks, perhaps?! Are we going to have other kinds of callbacks here?

@@ +3685,1 @@
>    registerDataCallCallback: function registerDataCallCallback(callback) {

This is intended to be moved out of RadioInterface in bug 928861, right?

@@ +3692,1 @@
>    unregisterDataCallCallback: function unregisterDataCallCallback(callback) {

ditto.

Comment 52

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

Comment on attachment 8356514 [details] [diff] [review]
Part 3-2: Handle WorkerMessage, v3

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

Great, thank you!

Comment 53

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

Comment on attachment 8356515 [details] [diff] [review]
Part 3-3: Move {get|setup|deactive}DataCallByType, v4

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

Great!

Comment 54

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

Comment on attachment 8356516 [details] [diff] [review]
Part 3-4: Move deactiveDataCalls, v3

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

Great.

Comment 55

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

Comment on attachment 8356517 [details] [diff] [review]
Part 3-5: Move updateRILNetworkInterface, v2

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

Thanks!!!

Comment 56

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

Comment on attachment 8356518 [details] [diff] [review]
Part 4: Modify RILNetworkInterface, v2

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

Thank you, thank you.

Comment 57

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
> Comment on attachment 8356504 [details] [diff] [review]
> Part 1: Add DataConnectionManager and DataConnectionHandler, v3
> 
> Review of attachment 8356504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks amazing! Thanks.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +628,5 @@
> > +
> > +      Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> > +    },
> > +
> > +    getConnectionHandler: function(clientId) {
> 
> I am feeling that if we'd want to take this chance to follow the mozilla
> coding style, i.e. with prefix "a" for argument, e.g. aClientId in this case.
> 

Hmmm, please forget about this.
As ConnectionManager still stays in RadioInterfaceLayer.js, it's better to keep the same style. :)

> We are going to have several variables in a function in some following
> parts. It might help improve readability within the prefix. What do you
> think about this?

Comment 58

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

(In reply to Edgar Chen [:edgar][:echen] from comment #35)
> Before requesting review, I would like to leave some notes about these
> patches,
> 1). For the detailed information about what I am going to do in this bug,
> please see comment #13.
> 2). I separated the changes into several patches to make it easier to review
> them.
> 3). I also did below tests:
>     - Enable/Disable data call by setting.
>     - Enable/Disable radio with data call enabled.
>     - Enable/Disable wifi with data call enabled.
>     - Send/Receive MMS with active network is wifi.
>     - Send/Receive MMS with active network is mobile.
>     - Update Apn Settings.
> 4). After r+, I will squash part 2-1, 2-2, 2-3 together
> 5). After r+, I will squash part 3-1, 3-2, 3-3, 3-4, 3-5 together

As we create several new functions for ConnectionManager in part2 and we get rid of those old functions in part3, it's also nicer to squash part2 and part3 together for landing.

> 6). Thanks reviewer in advance :)

Ha, thanks for the huge movement!

Comment 59

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #57)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
> > Comment on attachment 8356504 [details] [diff] [review]
> > Part 1: Add DataConnectionManager and DataConnectionHandler, v3
> > 
> > Review of attachment 8356504 [details] [diff] [review]:
> > 
> > I am feeling that if we'd want to take this chance to follow the mozilla
> > coding style, i.e. with prefix "a" for argument, e.g. aClientId in this case.
> > 
> 
> Hmmm, please forget about this.
> As ConnectionManager still stays in RadioInterfaceLayer.js, it's better to
> keep the same style. :)
> 
> > We are going to have several variables in a function in some following
> > parts. It might help improve readability within the prefix. What do you
> > think about this?

Yes, totally agree with you, using prefix 'a' for argument is much clear for readability.
But just like you suggest, it seems better to keep the same style with RadioInterfaceLayer.js for now.
Thank you.

Comment 60

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #49)
> Comment on attachment 8356508 [details] [diff] [review]
> Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in
> DataConnectionManager, v3
> 
> Review of attachment 8356508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my last question, thank you!
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +670,5 @@
> > +
> > +      if (this._currentDataClientId == -1) {
> > +        // This is to handle boot up stage.
> > +        this._currentDataClientId = this._dataDefaultClientId;
> > +        let connectionHandler = this._connectionHandlers[this._currentDataClientId];
> 
> nit: how about s/connectionHandler/connHandler/?  connectionHandler is
> really loooong ;) It's a nit, so I am fine with either but still want to
> provide my opinion. :)
> 

Aha, it's really looooong :p, I will use the short one you suggested, thank you.

> @@ +798,5 @@
> > +          }
> > +          for (let connectionHandler of this._connectionHandlers) {
> > +            let dataCallSettings = connectionHandler.dataCallSettings;
> > +            dataCallSettings.roamingEnabled = result;
> > +            connectionHandler.updateRILNetworkInterface();
> 
> Do we really need to updateRILNetworkInterface() on every connectionHandler?
> I thoughy we need that only for _dataDefaultCleintId, though I would say
> here is a bug for DSDS... we need to separate roaming preference for each
> client.

Yes, only call updateRILNetworkInterface() for _dataDefaultClientId seems easier, will address this in next patch.
I will file a new bug for separating roaming preference for each client.
Thank you.

Comment 61

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

Comment on attachment 8356515 [details] [diff] [review]
Part 3-3: Move {get|setup|deactive}DataCallByType, v4

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1093,5 @@
> +    }
> +    return apnSetting.iface.state;
> +  },
> +
> +  setupDataCallByType: function(apnType) {

Hi Edgar,

I'd like to add some more log messages here. I believe they benefit debugging! (I was helping debug a MMS connection issue, then realized that the logs are missing :( )

if (DEBUG) this.debug("setupDataCallByType: " + apnType);

@@ +1095,5 @@
> +  },
> +
> +  setupDataCallByType: function(apnType) {
> +    let apnSetting = this.apnSettings.byType[apnType];
> +    if (!apnSetting) {

Here,

if (DEBUG) {
  this.debug("No apn setting for type: " + apnType);
}

Comment 62

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #51)
> Comment on attachment 8356513 [details] [diff] [review]
> Part 3-1: Move DataCallCallback, v3
> 
> Review of attachment 8356513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +929,5 @@
> >      oldEnabled: false,
> >      enabled: false,
> >      roamingEnabled: false
> >    };
> > +  this._datacall_callbacks = [];
> 
> It's a good chance to make the coding style more consistent, i.e. removing
> underlines. Let's rename it dataCallbacks, perhaps?! Are we going to have
> other kinds of callbacks here?

Nope, we won't have other callbacks here.
Let's rename it to '_dataCallbacks'.
Thank you.

> 
> @@ +3685,1 @@
> >    registerDataCallCallback: function registerDataCallCallback(callback) {
> 
> This is intended to be moved out of RadioInterface in bug 928861, right?

Yes, after we come out a good interface for connect/disconnect in NetworkManager, we don't need to monitor datacall status via RIL directly, but through NetworkManager in a more generic way.

> 
> @@ +3692,1 @@
> >    unregisterDataCallCallback: function unregisterDataCallCallback(callback) {
> 
> ditto.

Comment 63

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #58)
> (In reply to Edgar Chen [:edgar][:echen] from comment #35)
> > Before requesting review, I would like to leave some notes about these
> > patches,
> > 1). For the detailed information about what I am going to do in this bug,
> > please see comment #13.
> > 2). I separated the changes into several patches to make it easier to review
> > them.
> > 3). I also did below tests:
> >     - Enable/Disable data call by setting.
> >     - Enable/Disable radio with data call enabled.
> >     - Enable/Disable wifi with data call enabled.
> >     - Send/Receive MMS with active network is wifi.
> >     - Send/Receive MMS with active network is mobile.
> >     - Update Apn Settings.
> > 4). After r+, I will squash part 2-1, 2-2, 2-3 together
> > 5). After r+, I will squash part 3-1, 3-2, 3-3, 3-4, 3-5 together
> 
> As we create several new functions for ConnectionManager in part2 and we get
> rid of those old functions in part3, it's also nicer to squash part2 and
> part3 together for landing.

Okay, will squash whole part2 and part3 together for landing.

Comment 64

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #61)
> Comment on attachment 8356515 [details] [diff] [review]
> Part 3-3: Move {get|setup|deactive}DataCallByType, v4
> 
> Review of attachment 8356515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1093,5 @@
> > +    }
> > +    return apnSetting.iface.state;
> > +  },
> > +
> > +  setupDataCallByType: function(apnType) {
> 
> Hi Edgar,
> 
> I'd like to add some more log messages here. I believe they benefit
> debugging! (I was helping debug a MMS connection issue, then realized that
> the logs are missing :( )
> 
> if (DEBUG) this.debug("setupDataCallByType: " + apnType);
> 
> @@ +1095,5 @@
> > +  },
> > +
> > +  setupDataCallByType: function(apnType) {
> > +    let apnSetting = this.apnSettings.byType[apnType];
> > +    if (!apnSetting) {
> 
> Here,
> 
> if (DEBUG) {
>   this.debug("No apn setting for type: " + apnType);
> }

Sure, will add them.

Comment 65

[Edgar Chen [:edgar]]

Attachment: Part 1: Add DataConnectionManager and DataConnectionHandler, v4, r=hsinyi

Rebase

Comment 66

[Edgar Chen [:edgar]]

Attachment: Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v4, r=hsinyi

1). Rebase
2). Address review comment #48.

Comment 67

[Edgar Chen [:edgar]]

Attachment: Part 1: Add DataConnectionManager and DataConnectionHandler, v5, r=hsinyi

Rebase

Comment 68

[Edgar Chen [:edgar]]

Attachment: Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v5, r=hsinyi

Rebase

Comment 69

[Edgar Chen [:edgar]]

Attachment: Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v4, r=hsinyi

1). Rebase
2). Address review comment #49.
- s/connectionHandler/connHandler/
- s/dataCallSettings/settings
- s/oldConnectionHandler/oldConnHandler
- s/oldRadioInterface/oldIface
- s/oldDataCallSettings/oldSettings
- s/newConnectionHandler/newConnHandler
- s/newRadioInterface/newIface
- s/newDataCallSettings/newSettings
- When roaming preference changed, only need to updateRILNetworkInterface() for _dataDefaultCleintId.

Comment 70

[Edgar Chen [:edgar]]

Attachment: Part 2-3: Move apnSettings, v4, r=hsinyi

1). Rebase
2). Address review comment #50.

Comment 71

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Move DataCallCallback, v4, r=hsinyi

1). Rebase
2). Address review comment#51.

Comment 72

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Handle WorkerMessage, v4, r=hsinyi

Rebase

Comment 73

[Edgar Chen [:edgar]]

Attachment: Part 3-3: Move {get|setup|deactive}DataCallByType, v5, r=hsinyi

1). Rebase
2). Address comment #61

Comment 74

[Edgar Chen [:edgar]]

Attachment: Part 3-4: Move deactiveDataCalls, v4, r=hsinyi

Rebase

Comment 75

[Edgar Chen [:edgar]]

Attachment: Part 3-5: Move updateRILNetworkInterface, v3, r=hsinyi

Rebase

Comment 76

[Edgar Chen [:edgar]]

Attachment: Part 4: Modify RILNetworkInterface, v3, r=hsinyi

Rebase

Comment 77

[Edgar Chen [:edgar]]

Attachment: Part 2-1: monitor ril.data.apnSettings in DataConnectionManager, v6, r=hsinyi

Rebase

Comment 78

[Edgar Chen [:edgar]]

Attachment: Part 2-2: monitor ril.data.{enabled|roaming_enabled|defaultServiceId} in DataConnectionManager, v5, r=hsinyi

Rebase

Comment 79

[Edgar Chen [:edgar]]

Attachment: Part 2-3: Move apnSettings, v5, r=hsinyi

Rebase

Comment 80

[Edgar Chen [:edgar]]

Attachment: Part 3-1: Move DataCallCallback, v5, r=hsinyi

Rebase

Comment 81

[Edgar Chen [:edgar]]

Attachment: Part 3-2: Handle WorkerMessage, v5, r=hsinyi

Rebase

Comment 82

[Edgar Chen [:edgar]]

Attachment: Part 3-3: Move {get|setup|deactive}DataCallByType, v6, r=hsinyi

Rebase

Comment 83

[Edgar Chen [:edgar]]

Attachment: Part 3-5: Move updateRILNetworkInterface, v4, r=hsinyi

Comment 84

[Edgar Chen [:edgar]]

Attachment: Part 4: Modify RILNetworkInterface, v4, r=hsinyi

Rebase

Comment 85

[Edgar Chen [:edgar]]

Attachment: Part 2: Move data connection related code to DataConnectionManager/DataConnectionHandler, v7, r=hsinyi

Squash whole part2-x, part3-x and part4 together for landing.

Comment 86

[Edgar Chen [:edgar]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=70890b73b917

Comment 87

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #86)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=70890b73b917

Marionette fail in test_mobile_data_location.js caused by following error,

22:36:51  WARNING -  02-06 06:26:09.783    45    45 E GeckoConsole: [JavaScript Error: "dataCallSettings is not defined" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 1262}]

Should use |this.dataCallSettings|.

Comment 88

[Edgar Chen [:edgar]]

Attachment: Part 2: Move data connection related code to DataConnectionManager/DataConnectionHandler, v8, r=hsinyi

Address comment #87.

Comment 89

[Edgar Chen [:edgar]]

Try server again: https://tbpl.mozilla.org/?tree=Try&rev=598be14fc859 (All green \o/)

Comment 90

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

(In reply to Edgar Chen [:edgar][:echen] from comment #89)
> Try server again: https://tbpl.mozilla.org/?tree=Try&rev=598be14fc859 (All
> green \o/)

YAYA \o/

Comment 91

[Edgar Chen [:edgar]]

I had test following scenario again:
- Enable/Disable data call by setting.
- Enable/Disable radio with data call enabled.
- Enable/Disable wifi with data call enabled.
- Send/Receive MMS with active network is wifi.
- Send/Receive MMS with active network is mobile.
- Update Apn Settings then send MMS. (bug 931348)

I think it is safe to land. :)

Comment 92

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f3d1d2027561
https://hg.mozilla.org/integration/b2g-inbound/rev/304e60934b69

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3d1d2027561
https://hg.mozilla.org/mozilla-central/rev/304e60934b69