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.

Steal ... :)

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

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

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

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 on attachment 8356504 [details] [diff] [review] Part 1: Add DataConnectionManager and DataConnectionHandler, v3 Please see comment #35.

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

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 on attachment 8356510 [details] [diff] [review] Part 2-3: Move apnSettings, v3 Please see comment #35.

Comment on attachment 8356513 [details] [diff] [review] Part 3-1: Move DataCallCallback, v3 Please see comment #35.

Comment on attachment 8356514 [details] [diff] [review] Part 3-2: Handle WorkerMessage, v3 Please see comment #35.

Comment on attachment 8356515 [details] [diff] [review] Part 3-3: Move {get|setup|deactive}DataCallByType, v4 Please see comment #35.

Comment on attachment 8356516 [details] [diff] [review] Part 3-4: Move deactiveDataCalls, v3 Please see comment #35.

Comment on attachment 8356517 [details] [diff] [review] Part 3-5: Move updateRILNetworkInterface, v2 Please see comment #35.

Comment on attachment 8356518 [details] [diff] [review] Part 4: Modify RILNetworkInterface, v2 Please see comment #35.

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

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 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 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 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 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 on attachment 8356514 [details] [diff] [review] Part 3-2: Handle WorkerMessage, v3 Review of attachment 8356514 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you!

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 on attachment 8356516 [details] [diff] [review] Part 3-4: Move deactiveDataCalls, v3 Review of attachment 8356516 [details] [diff] [review]: ----------------------------------------------------------------- Great.

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

Comment on attachment 8356518 [details] [diff] [review] Part 4: Modify RILNetworkInterface, v2 Review of attachment 8356518 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, thank you.

(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?

(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!

(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.

(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 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); }

(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.

(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.

(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.

Rebase

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

Rebase

Rebase

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.

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

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

Rebase

1). Rebase 2). Address comment #61

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

Rebase

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

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

(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|.

Address comment #87.

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

(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/

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. :)

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

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