Users should be able to set cellular and data settings for multiple sim cards.

Stealing. I'll probably resolve this bug as duplicated of bug 926372. I'll let you know guys.

Isn't this a duplicate of 926350?

The work to be done is the related to the changes in the call setting UI according the DSDS specs provided by UX team. See `[SPEC]DSDS v0.5.pdf`, Other SIM related settings, Cellular & Data, slide #53.

Blocked by Bug 814637, clearing target milestone till WebIccManager api lands

WIP branch at https://github.com/jaoo/gaia/tree/928297

First WIP patch. More or less working. Needs more test once the DSDS support for mozIccManager API gets landed.

Plan to land it in the 1st week of sprint 6.

Second version of a still WIP patch. Since this is almost completed it's time for requesting some feedback. Arthur, this patch follows the UX recommendations, I've added a new panel for selecting the ICC card to handle and the rest of panels keep the same. I've added a new object factory to create the object in charge of handling the cell and data calls for each ICC card. I hope you understand the design but I've tried to avoid big changes. Your feedback is welcome. Thanks.

Jose, is it possible for you to push the patch to github? I am not able to apply the patch to the current gaia master. Thanks.

Latest version of the patch. The most up to date patch usually lives at https://github.com/jaoo/gaia/tree/928297/

Comment on attachment 8336662 [details] [diff] [review] WIP v3 Nice patch! I left some comments in github.

(In reply to Arthur Chen [:arthurcc] from comment #11) > Comment on attachment 8336662 [details] [diff] [review] > WIP v3 > > Nice patch! I left some comments in github. New version of the patch. This version addresses the comment Arthur made in the 3rd version of the patch. Need to deal with some minor things and will request review ASAP.

First version of the patch ready for review. Arthur, I've changed a bit the design you saw. I've addressed the comment you made about hiding the panel for choosing the ICC card in case of single ICC card devices and the comment you made about loading the javascript file with the corresponding html file. The patch is huge but some changes are only for cleaning purposes. Please, let me know if you have any doubt. I'll try to do more testing on a real device and will update the patch if needed. Thanks.

Comment on attachment 8338628 [details] [diff] [review] v1 Redirect the request to Kaze. Thank you Kaze!!

FYI with the patch in this bug the user should be able to select the APNs for the ICC cards inserted in the device and an array of APNs should be passed to the RIL plumbing (I mean the ril.data.apnSettings should be already an array).

I would recommend to check out the most recent version of the patch at https://github.com/jaoo/gaia/tree/928297/ if anyone is interested on testing it.

This is the second version of the patch ready for review. I've fixed some issues the first version had while testing on fugu device. Fabien, the patch is huge but you already understand the big picture of the cell and data settings. If you have any doubt ping me on IRC please. Jessica, there is no need for waiting bug 933203 to be finished since you should be able to test data calls on multi ICC card devices with this patch. Something is fishy with data calls since I've not been able to setup data calls on the second ICC card. Even being the second ICC card selected as the active one for data calls the data call is always set up whit the first ICC card. FYI I'm not seeing the `ril.data.defaultServiceId` setting changing when the user changes the active ICC card for data calls. Your feedback is also appreciate. As a general comment IMHO the fugu device (I mean the porting, the build, whatever) is still pretty unstable for development. It makes the development process hard and puts the commitments at risk.

Thank you Jose for your patch, it's a big change in such a short time. I have applied the patch to test data calls and the followings are my test results: - On boot up, ril.data.apnSettings array has sim2 apn as the first item and the second item is empty, I think this is expected, since bug 933203 will take care of it. - When I go to 'Cellular & Data' in Settings page, both sim's APN looks correct in the page. But if I go to one of the apn settings, say 'Data settings', and tap OK without changing anything, ril.data.apnSettings then becomes: [[{"apn":"","user":"","password":"","proxy":"","port":"","authtype":"notDefined","types":["default"]},{"carrier":"台灣大哥大(TW Mobile) (Internet)","apn":"internet","types":["supl"]},{"carrier":"台灣大哥大(TW Mobile) (MMS)","apn":"mms","mmsc":"http://mms.catch.net.tw","mmsproxy":"10.1.1.2","mmsport":"80","types":["mms"]}],[{"apn":"","user":"","password":"","proxy":"","port":"","authtype":"notDefined","types":["default"]}]]. Sim2's apn is still in the first item, and it's default apn is empty; the second item exists but has empty values. Do you know what could went wrong? - We still can not setup data call on the second sim cause it's DataInfo is always in searching state, please see bug 944231. - I do see the 'ril.data.defaultServiceId' setting change, did you have the patch in bug 932731? Thank you again! :) PS: My sim1 is CHT and sim2 is TW Mobile.

(In reply to Jessica Jong [:jjong] [:jessica] from comment #18) > Thank you Jose for your patch, it's a big change in such a short time. Thanks for your feedback. > - On boot up, ril.data.apnSettings array has sim2 apn as the first item and > the second item is empty, I think this is expected, since bug 933203 will > take care of it. Yes, until we finish bug 933203 this is the best we can do. > - When I go to 'Cellular & Data' in Settings page, both sim's APN looks > correct in the page. But if I go to one of the apn settings, say 'Data > settings', and tap OK without changing anything, ril.data.apnSettings then > becomes: > [[{"apn":"","user":"","password":"","proxy":"","port":"","authtype": > "notDefined","types":["default"]},{"carrier":"台灣大哥大(TW Mobile) > (Internet)","apn":"internet","types":["supl"]},{"carrier":"台灣大哥大(TW Mobile) > (MMS)","apn":"mms","mmsc":"http://mms.catch.net.tw","mmsproxy":"10.1.1.2", > "mmsport":"80","types":["mms"]}],[{"apn":"","user":"","password":"","proxy": > "","port":"","authtype":"notDefined","types":["default"]}]]. > Sim2's apn is still in the first item, and it's default apn is empty; the > second item exists but has empty values. Do you know what could went wrong? Yep, there is an issue in the function http://goo.gl/7gvzwC I'll have a fix for this ASAP. > - We still can not setup data call on the second sim cause it's DataInfo is > always in searching state, please see bug 944231. Well I'll add this bug as dependency. > - I do see the 'ril.data.defaultServiceId' setting change, did you have the > patch in bug 932731? My patch lives on top of that bug (932731), that way I'm able to switch ICC cards. In my case 'ril.data.defaultServiceId' setting is not changing :(. > PS: My sim1 is CHT and sim2 is TW Mobile. Could you provide the MCC anc MNC codes from CHT ICC card please? Thanks.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #18) > > Thank you Jose for your patch, it's a big change in such a short time. > > Thanks for your feedback. > > > - On boot up, ril.data.apnSettings array has sim2 apn as the first item and > > the second item is empty, I think this is expected, since bug 933203 will > > take care of it. > > Yes, until we finish bug 933203 this is the best we can do. > > > - When I go to 'Cellular & Data' in Settings page, both sim's APN looks > > correct in the page. But if I go to one of the apn settings, say 'Data > > settings', and tap OK without changing anything, ril.data.apnSettings then > > becomes: > > [[{"apn":"","user":"","password":"","proxy":"","port":"","authtype": > > "notDefined","types":["default"]},{"carrier":"台灣大哥大(TW Mobile) > > (Internet)","apn":"internet","types":["supl"]},{"carrier":"台灣大哥大(TW Mobile) > > (MMS)","apn":"mms","mmsc":"http://mms.catch.net.tw","mmsproxy":"10.1.1.2", > > "mmsport":"80","types":["mms"]}],[{"apn":"","user":"","password":"","proxy": > > "","port":"","authtype":"notDefined","types":["default"]}]]. > > Sim2's apn is still in the first item, and it's default apn is empty; the > > second item exists but has empty values. Do you know what could went wrong? > > Yep, there is an issue in the function http://goo.gl/7gvzwC I'll have a fix > for this ASAP. > > > - We still can not setup data call on the second sim cause it's DataInfo is > > always in searching state, please see bug 944231. > > Well I'll add this bug as dependency. > > > - I do see the 'ril.data.defaultServiceId' setting change, did you have the > > patch in bug 932731? > > My patch lives on top of that bug (932731), that way I'm able to switch ICC > cards. In my case 'ril.data.defaultServiceId' setting is not changing :(. > > > PS: My sim1 is CHT and sim2 is TW Mobile. > > Could you provide the MCC anc MNC codes from CHT ICC card please? Thanks. Sure. CHT's mcc mcn: 466 92 TW Mobile's mcc mnc: 466 93

Logs for your reference.

Comment on attachment 8339613 [details] [diff] [review] v2 Bug 933203 got r+ so we should rebase this patch on top of the patch of that bug and test the whole functionality again. Moreover we need to take a decision on what to do on bug 933203 (I mean land it or wait) (see https://bugzilla.mozilla.org/show_bug.cgi?id=933203#c25). Having said that I'm cancelling this request. Sorry for the inconveniences.

New version of the patch. Seems to be working fine on my tests. Jessica, could you give it a try please? If so, I would recommend you to install the WIP branch at https://github.com/jaoo/gaia/tree/928297 since it has bug 933203 as a dependency.

New version of the patch. Seems to be working fine on my tests. Fabien, I think this version is ready for review. You already took at look at previous WIP patches so you are already aware of the changes. Would you review this patch please?. Thanks. Jessica, could you give it a try please? If so, I would recommend you to install the WIP branch at https://github.com/jaoo/gaia/tree/928297 since it has bug 933203 as a dependency. Your feedback is more than welcome. Thanks. Jessica I've noticed that the RIL plumbing restarts the data call even if the user selects a new APN for the ICC card that is not the active one for data calls. I guess this should not happen, take a look at this please. Thanks.

Thank you José. I have applied your changes in bug 933203 and this bug. These are my test results: - On bootup, RIL will receive the the apn settings of both SIMs eventually. This is working fine. - When entering Settings -> SIM 1 -> Cellular & Data -> Data Settings, I see that the "custom settings" option is selected, and if I select the first preloaded apn, RIL will receive more than one apn for each type: 'ril.data.apnSettings' is now [[{"apn":"internet","user":"","password":"","proxy":"","port":"","authtype":"notDefined","types":["default"]},{"carrier":"中華電信(Chunghwa) (Internet)","apn":"internet","types":["supl"]},{"apn":"internet","user":"","password":"","proxy":"","port":"","authtype":"notDefined","types":["default"]},{"carrier":"中華電信(Chunghwa)","apn":"emome","mmsc":"http://mms.emome.net:8002","mmsproxy":"10.1.1.1","mmsport":"8080","types":["supl","mms"]}],[{"carrier":"台灣大哥大(TW Mobile) (Internet)","apn":"internet","types":["default","supl"]},{"carrier":"台灣大哥大(TW Mobile) (MMS)","apn":"mms","mmsc":"http://mms.catch.net.tw","mmsproxy":"10.1.1.2","mmsport":"80","types":["mms"]}]] - If I go to the "Data settings" of SIM1 and then go to the "Data settings" on SIM2 (or viceversa), I always see that the "custom settings" is selected, not the one that I selected. But If I re-enter the Data settings of a same SIM, my selected option is remembered. - The times 'ril.data.apnSettings' is set increments each time I edit the apns. For example, the first time that I go to Data Settings and tap OK, RIL observes one 'ril.data.apnSettings' change, the second time that I go to Data Settings and tap OK, RIL observes two 'ril.data.apnSettings' change (with the same values), and so on. - About the issue you mentioned in Comment 24, we have marked as a todo in bug 939046 comment 1 item 4. Thanks again!

(In reply to Jessica Jong [:jjong] [:jessica] from comment #25) > Thank you José. I have applied your changes in bug 933203 and this bug. > These are my test results: Thanks for the feedback Jessica. Your coments are really useful. > - When entering Settings -> SIM 1 -> Cellular & Data -> Data Settings, I see > that the "custom settings" option is selected, and if I select the first > preloaded apn, RIL will receive more than one apn for each type: > 'ril.data.apnSettings' is now > [[{"apn":"internet","user":"","password":"","proxy":"","port":"","authtype": > "notDefined","types":["default"]},{"carrier":"中華電信(Chunghwa) > (Internet)","apn":"internet","types":["supl"]},{"apn":"internet","user":"", > "password":"","proxy":"","port":"","authtype":"notDefined","types": > ["default"]},{"carrier":"中華電信(Chunghwa)","apn":"emome","mmsc":"http://mms. > emome.net:8002","mmsproxy":"10.1.1.1","mmsport":"8080","types":["supl", > "mms"]}],[{"carrier":"台灣大哥大(TW Mobile) > (Internet)","apn":"internet","types":["default","supl"]},{"carrier": > "台灣大哥大(TW Mobile) > (MMS)","apn":"mms","mmsc":"http://mms.catch.net.tw","mmsproxy":"10.1.1.2", > "mmsport":"80","types":["mms"]}]] I didn't notice that on my side. I'll double check it and see what's going on. > - If I go to the "Data settings" of SIM1 and then go to the "Data settings" > on SIM2 (or viceversa), I always see that the "custom settings" is selected, > not the one that I selected. But If I re-enter the Data settings of a same > SIM, my selected option is remembered. This is a minor issue IMHO, I'll take a look at it. > - The times 'ril.data.apnSettings' is set increments each time I edit the > apns. For example, the first time that I go to Data Settings and tap OK, RIL > observes one 'ril.data.apnSettings' change, the second time that I go to > Data Settings and tap OK, RIL observes two 'ril.data.apnSettings' change > (with the same values), and so on. This is really weird and I guess this is not very good for the RIL plumbing. I'll fix it. Given these comments I'll cancel the request for review until I shed some light into the issues found.

Comment on attachment 8341925 [details] [diff] [review] v4 Cancel request for review per comment #c26. Sorry for the noise Fabien!

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #26) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #25) > > - When entering Settings -> SIM 1 -> Cellular & Data -> Data Settings, I see > > that the "custom settings" option is selected, and if I select the first > > preloaded apn, RIL will receive more than one apn for each type: > > 'ril.data.apnSettings' is now > > [[{"apn":"internet","user":"","password":"","proxy":"","port":"","authtype": > > "notDefined","types":["default"]},{"carrier":"中華電信(Chunghwa) > > (Internet)","apn":"internet","types":["supl"]},{"apn":"internet","user":"", > > "password":"","proxy":"","port":"","authtype":"notDefined","types": > > ["default"]},{"carrier":"中華電信(Chunghwa)","apn":"emome","mmsc":"http://mms. > > emome.net:8002","mmsproxy":"10.1.1.1","mmsport":"8080","types":["supl", > > "mms"]}],[{"carrier":"台灣大哥大(TW Mobile) > > (Internet)","apn":"internet","types":["default","supl"]},{"carrier": > > "台灣大哥大(TW Mobile) > > (MMS)","apn":"mms","mmsc":"http://mms.catch.net.tw","mmsproxy":"10.1.1.2", > > "mmsport":"80","types":["mms"]}]] > > I didn't notice that on my side. I'll double check it and see what's going > on. Go it, the ICC cards you use in your tests have more than one APN for the `default` type. See [1] please. I'll take this into account. Fixing it. [1] http://goo.gl/QkciYl

Addressed comments/issues provided as feedback by Jessica in comment #25.

Comment on attachment 8342322 [details] [diff] [review] v5 I was told that kaze cannot review this patch so requesting review at Arthur.

Comment on attachment 8342322 [details] [diff] [review] v5 Review of attachment 8342322 [details] [diff] [review]: ----------------------------------------------------------------- I saw you’ve switched the review to Arthur, but here are my first comments. Long story short, there are l10n details to take care of and a few console.log messages that I’d prefer to silent. ::: apps/settings/elements/carrier_iccs.html @@ +2,5 @@ > + <template> > + > + <header> > + <a href="#root"><span class="icon icon-back">back</span></a> > + <h1 data-l10n-id="selectASIMCard"> Select a SIM card </h1> please add this l10n entity to the settings.en-US.properties file @@ +9,5 @@ > + <div> > + <ul> > + <li id="menuItem-sim1" aria-disabled="true" class="hint"> > + <small></small> > + <a data-href="#carrier" data-l10n-id="sim1">SIM 1</a> same here @@ +13,5 @@ > + <a data-href="#carrier" data-l10n-id="sim1">SIM 1</a> > + </li> > + <li id="menuItem-sim2" aria-disabled="true" class="hint"> > + <small></small> > + <a data-href="#carrier" data-l10n-id="sim2">SIM 2</a> same here ::: apps/settings/js/carrier.js @@ +138,5 @@ > + cs_getMccMncCodes(function getMccMncCodesCb() { > + console.log('IccCardIndexForCellAndDataSettings is ' + > + DsdsSettings.getIccCardIndexForCellAndDataSettings()); > + console.log('Loading APNs for MCC/MNC ' + > + JSON.stringify(_mccMncCodes)); If you can’t remove these console logs, please put them behind an “if (DEBUG)” statement (or equivalent). The console is already too noisy imho. @@ +269,5 @@ > + // show user friendly network mode names > + if (gsm && cdma) { > + if (type in NETWORK_DUALSTACK_MAP) { > + option.textContent = > + localize(option, NETWORK_DUALSTACK_MAP[type]); The `localize()' function already does not return anything, but it sets the .textContent of the first argument. In other words, the following line is enough: localize(option, NETWORK_DUALSTACK_MAP[type]); @@ +274,5 @@ > } > + } else if (gsm) { > + if (type in NETWORK_GSM_MAP) { > + option.textContent = > + localize(option, NETWORK_GSM_MAP[type]); same here @@ +279,5 @@ > + } > + } else if (cdma) { > + if (type in NETWORK_CDMA_MAP) { > + option.textContent = > + localize(option, NETWORK_CDMA_MAP[type]); same here @@ +281,5 @@ > + if (type in NETWORK_CDMA_MAP) { > + option.textContent = > + localize(option, NETWORK_CDMA_MAP[type]); > + } > + } else { //failback only nit: space after the double slash please As I guess this situation is not supposed to happen (right?), this might be a good place to put a console.warn(). @@ +778,5 @@ > + var lastItem = apnList.querySelector('.apnSettings-custom'); > + > + var kUsageMapping = {'data': 'default', > + 'mms': 'mms', > + 'supl': 'supl'}; Nit, indentation please: var kUsageMapping = { 'data': 'default', 'mms': 'mms', 'supl': 'supl' }; ::: apps/settings/js/carrier_iccs.js @@ +85,5 @@ > + var numberOfIccCards = _mobileConnections.length; > + for (var i = 0; i < numberOfIccCards; i++) { > + var mobileConnection = _mobileConnections[i]; > + if (!mobileConnection.iccId) { > + console.log('ICC card not detected on load.'); do we really need a console.log() here, or can it be put behind an “if (DEBUG)” statement? @@ +92,5 @@ > + localize(_menuItemDescriptions[i], CARDSTATE_MAPPING[state]); > + ihfcs_disableItems(_menuItemIds[i], true); > + continue; > + } > + console.log('ICC card ' + mobileConnection.iccId + ' detected on load.'); same here @@ +105,5 @@ > + > + // This code might not be needed. > + _iccManager.addEventListener('iccdetected', > + function iccDetectedHandler(evt) { > + console.log('ICC card ' + evt.iccId + ' becomes detected.'); same here @@ +122,5 @@ > + > + // This code might not be needed. > + _iccManager.addEventListener('iccundetected', > + function iccUndetectedHandler(evt) { > + console.log('ICC card ' + evt.iccId + ' becomes detected.'); same here ::: apps/settings/js/connectivity.js @@ +203,4 @@ > } > > + if (!IccHelper) > + return; nit, please add braces @@ +273,5 @@ > + } > + > + showCellAndDataDescription(); > + if (!mobileConnections[0].iccId) { > + console.log('ICC card not detected on load.'); another console.log @@ +279,5 @@ > + } > + > + console.log('ICC card ' + > + mobileConnections[0].iccId + > + ' detected on load.'); another console.log ::: apps/settings/js/dsds_settings.js @@ +9,5 @@ > + var _settings = window.navigator.mozSettings; > + var _iccManager = window.navigator.mozIccManager; > + var _mobileConnections = null; > + > + /** */ nit, maybe we don’t need this empty comment block? ::: apps/settings/js/settings.js @@ +818,5 @@ > } > } > > + if (!mobileConnections[0].iccId) { > + console.log('ICC card not detected on load.'); another console.log @@ +823,5 @@ > + disableSIMRelatedSubpanels(true); > + } else { > + console.log('ICC card ' + > + mobileConnections[0].iccId + > + ' detected on load.'); another console.log

This version addresses the comments made by kaze. Fabien, thanks for your comments.

Comment on attachment 8342645 [details] [diff] [review] v6 Thank you José, works well now! I did find another issue, I am not sure if it's in the scope of this bug. When I remove one of the SIMs, the apn for that SIM slot is cached and not cleared. It seems that you will wait for icc's mcc/mnc to be detected to update the apn, but if it's never detected the apn would just stay there. It is not causing any problem though, since we can't do anything without a SIM.

(In reply to Jessica Jong [:jjong] [:jessica] from comment #33) > Comment on attachment 8342645 [details] [diff] [review] > v6 > > Thank you José, works well now! Thanks for the feedback Jessica! > I did find another issue, I am not sure if it's in the scope of this bug. > When I remove one of the SIMs, the apn for that SIM slot is cached and not > cleared. It seems that you will wait for icc's mcc/mnc to be detected to > update the apn, but if it's never detected the apn would just stay there. It > is not causing any problem though, since we can't do anything without a SIM. How did you managed to enter to the cell and data settings for the SIM you removed. That shouldn't be possible since the menu item for entering to the settings of that ICC card should be disabled.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #34) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #33) > > Comment on attachment 8342645 [details] [diff] [review] > > v6 > > > > Thank you José, works well now! > > Thanks for the feedback Jessica! > > > I did find another issue, I am not sure if it's in the scope of this bug. > > When I remove one of the SIMs, the apn for that SIM slot is cached and not > > cleared. Oh, I guess you see the APN not clear in the RIL logs. Anyway, my device/build doesn't detect ICC cards unless there are tow of them in the device. I mean with only one ICC card in the device, both ICC cards are absent. > > It seems that you will wait for icc's mcc/mnc to be detected to > > update the apn, but if it's never detected the apn would just stay there. It > > is not causing any problem though, since we can't do anything without a SIM. > > How did you managed to enter to the cell and data settings for the SIM you > removed. That shouldn't be possible since the menu item for entering to the > settings of that ICC card should be disabled.

Comment on attachment 8342645 [details] [diff] [review] v6 Review of attachment 8342645 [details] [diff] [review]: ----------------------------------------------------------------- Haven't reviewed all of the files. ::: apps/settings/js/dsds_settings.js @@ +28,5 @@ > + * Get number of ICC cards. > + * > + * @return {Numeric} Number of ICC cards. > + */ > + function ds_getNumberOfIccCards() { nit: The function name is misleading as mozMobileConnection is more like a sim slot. @@ +74,5 @@ > + > +/** > + * Startup. > + */ > +navigator.mozL10n.ready(DsdsSettings.init.bind(DsdsSettings)); Initializing mozMobileConnections takes time. We need to add the following logic to delay the initialization of this module: // starting when we get a chance navigator.mozL10n.ready(function loadWhenIdle() { var idleObserver = { time: 3, onidle: function() { DsdsSettings.init(); navigator.removeIdleObserver(idleObserver); } }; navigator.addIdleObserver(idleObserver); }); ::: apps/settings/js/settings.js @@ +760,3 @@ > function handleRadioAndCardState() { > + var MAX_NUMBER_OF_ICC_CARDS = 2; > + var iccIdIccCard = 0; nit: Suggest to use iccId. @@ +788,4 @@ > > + // Disable SIM security item in case of SIM absent or airplane mode. > + if (!mobileConnections[0].iccId || > + (mobileConnections[0].radioState === 'enabled')) { Should be 'disabled'. @@ +845,5 @@ > + iccIdIccCard = mobileConnections[0].iccId; > + var iccCard = iccManager.getIccById(iccIdIccCard); > + iccCard.addEventListener('cardstatechange', > + cardStateHandler); > + mobileConnections[0].addEventListener('radiostatechange', We can use the original event listener registered in the first place. Please also see my comments below. @@ +860,5 @@ > + if (iccCard) { > + iccCard.removeEventListener('cardstatechange', > + cardStateHandler); > + } > + mobileConnections[0].removeEventListener('radiostatechange', We don't need to remove the listener here.

Addresses comments made in comment #36. Patch also at https://github.com/jaoo/gaia/tree/928297.

Comment on attachment 8343104 [details] [diff] [review] v7 Review of attachment 8343104 [details] [diff] [review]: ----------------------------------------------------------------- carrier.js is the last file to be reviewed. Please check my comments for other parts. The main concern is that we have duplicate codes for updating the SIM card UI items. Suggest to extract the logic. ::: apps/settings/js/carrier_iccs.js @@ +25,5 @@ > + }; > + > + var DATA_TYPE_MAPPING = { > + 'lte' : '4G LTE', > + 'ehrpd': 'CDMA', Not related to this bug. Can we call it "4G CDMA"? Which aligns to what we defined in system app (system/js/statusbar.js). @@ +61,5 @@ > + _mobileConnections = window.navigator.mozMobileConnections; > + if (!_settings || !_mobileConnections || !_iccManager) { > + return; > + } > + if (DsdsSettings.getNumberOfIccSlots() < MAX_NUMBER_OF_ICC_SLOTS) { If this is used to check of we are on a multi-sim device, DsdsSettings.getNumberOfIccSlots() <= 1 seems more straight forward. @@ +159,5 @@ > + > + var iccCard = _iccManager.getIccById(iccId); > + var cardState = iccCard.cardState; > + if (cardState !== 'ready') { > + localize(desc, CARDSTATE_MAPPING[!cardState ? 'null' : cardState]); We can use cardState || 'null'. @@ +177,5 @@ > + } else { > + carrier = iccInfo.spn; > + } > + } > + desc.textContent = carrier; We need to clear the l10n id when setting textContent directly or it is localized when changing languages. @@ +230,5 @@ > + * @param {String} iccId The iccId code form the ICC card. > + * > + * @return {mozMobileConnection} mozMobileConnection object. > + */ > + function ihfcs_getMobileConnectionFromIccCard(iccId) { nit: suggest to use "ihfcs_getMobileConnectionFromIccId". ::: apps/settings/js/connectivity.js @@ +215,4 @@ > */ > > + function updateCellAndDataDescription() { > + var iccIdIccCard = 0; It seems that we have many duplicate code for registering events and updating UIs based on the card states. Can we extract the logic to a function like the following? function initIccCardUI(listElement, index) { // register events to mozIccManager // register events to mozIcc corresponding to the index // register events to mozMobileConnection corresponding to the index // update UI... } Then we can simply do something like: listElements.forEach(initIccCardUI.bind(this)); ::: apps/settings/js/dsds_settings.js @@ +53,5 @@ > + * Hide or show the cell and data settings panel in which we show the ICC > + * cards. > + */ > + function ds_handleCellAndDataSettingSimPanel() { > + if (ds_getNumberOfIccSlots() === MAX_NUMBER_OF_ICC_SLOTS) { Suggest to use ds_getNumberOfIccSlots() > 1. ::: apps/settings/js/settings.js @@ +838,2 @@ > > + if (mobileConnections.length < MAX_NUMBER_OF_ICC_SLOTS) { Suggest to use mobileConnections.length <= 1. @@ +901,5 @@ > + iccManager.addEventListener('iccundetected', > + function iccUndetectedHandler(evt) { > + if (iccId === evt.iccId) { > + disableSIMRelatedSubpanels(true); > + mobileConnections[0].removeEventListener('radiostatechange', We don't need to remove the listener when "iccundetected".

Comment on attachment 8343104 [details] [diff] [review] v7 Review of attachment 8343104 [details] [diff] [review]: ----------------------------------------------------------------- It seems most of the functions on carrier.js are the same as before, so that I focused on the different parts. Carrier.js looks good and I have just one main question and a few nits. Please check my comments inline. Thank you for the great work!! ::: apps/settings/js/carrier.js @@ +125,5 @@ > + cs_showCarrierName(); > + cs_disabeEnableDataCallCheckbox(); > + return; > + } > + if (!e.detail.current.startsWith('#carrier-') || Suggest to move this check before checking mobile connection as there is a higher chance of early return resulted from these conditions. nit: Use a variable to store the current hash. @@ +828,3 @@ > > + // maps for UI fields(current settings key) to new apn setting keys. > + var kKeyMappings = {'passwd': 'password', nit: indention. var kKeyMappings = { 'passwd': 'password', .... @@ +876,5 @@ > + * RIL plumbing for setting up the data call. It also stores the setting > + * into the settings database. > + * > + * @param {String} type APN type affected. > + * @param {Array} apns Array contaning the APN for the ICC cards. typo: containing @@ +878,5 @@ > + * > + * @param {String} type APN type affected. > + * @param {Array} apns Array contaning the APN for the ICC cards. > + * @param {Numeric} iccCardAffected Index of the ICC card affected. The > + * we are building the new APNs for. Missing ICC card? @@ +914,5 @@ > + var apnsForIccCard = apns[iccCardIndex]; > + for (var apnIndex = 0; apnIndex < apnsForIccCard.length; apnIndex++) { > + var apn = apnsForIccCard[apnIndex]; > + if (apn.types.indexOf(type) !== -1) { > + if (apn.types.length > 1) { If the new apn setting has exactly the same value as the original settings but with different type, we only need to add a new type to the original apn item instead of creating a new one. And it would be better to have a comment describing the behaviors here.

(In reply to Arthur Chen [:arthurcc] from comment #38) > Comment on attachment 8343104 [details] [diff] [review] > v7 > > Review of attachment 8343104 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for your review! > > Please check my comments for other parts. The main concern is that we have > > duplicate codes for updating the SIM card UI items. Suggest to extract the > > logic. Logic extracted as much as possible everywhere. > @@ +177,5 @@ > > + } else { > > + carrier = iccInfo.spn; > > + } > > + } > > + desc.textContent = carrier; > > We need to clear the l10n id when setting textContent directly or it is > localized when changing languages. The element does not have l10n id, there is no need to clear it. > @@ +901,5 @@ > > + iccManager.addEventListener('iccundetected', > > + function iccUndetectedHandler(evt) { > > + if (iccId === evt.iccId) { > > + disableSIMRelatedSubpanels(true); > > + mobileConnections[0].removeEventListener('radiostatechange', > > We don't need to remove the listener when "iccundetected". We do. `mobileConnectons[0]` exists even when the ICC card gets undetected so we need to remove the event listener we added previously. (In reply to Arthur Chen [:arthurcc] from comment #39) > Comment on attachment 8343104 [details] [diff] [review] > v7 > > Review of attachment 8343104 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for the review! > ::: apps/settings/js/carrier.js > @@ +125,5 @@ > > + cs_showCarrierName(); > > + cs_disabeEnableDataCallCheckbox(); > > + return; > > + } > > + if (!e.detail.current.startsWith('#carrier-') || > > Suggest to move this check before checking mobile connection as there is a > higher chance of early return resulted from these conditions. I do prefer to keep the logic a it is right now. It seems easier to understand. > @@ +914,5 @@ > > + var apnsForIccCard = apns[iccCardIndex]; > > + for (var apnIndex = 0; apnIndex < apnsForIccCard.length; apnIndex++) { > > + var apn = apnsForIccCard[apnIndex]; > > + if (apn.types.indexOf(type) !== -1) { > > + if (apn.types.length > 1) { > > If the new apn setting has exactly the same value as the original settings > but with different type, we only need to add a new type to the original apn > item instead of creating a new one. > > And it would be better to have a comment describing the behaviors here. Well handling APNs it's a bit tricky. I've added as many comments as possible.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #40) > Created attachment 8343704 [details] [diff] [review] > v8 > > (In reply to Arthur Chen [:arthurcc] from comment #38) > > Comment on attachment 8343104 [details] [diff] [review] > > v7 > > > > Review of attachment 8343104 [details] [diff] [review]: > > ----------------------------------------------------------------- > > Thanks for your review! > > > > Please check my comments for other parts. The main concern is that we have > > duplicate codes for updating the SIM card UI items. Suggest to extract the > > > logic. > > Logic extracted as much as possible everywhere. > > > @@ +177,5 @@ > > > + } else { > > > + carrier = iccInfo.spn; > > > + } > > > + } > > > + desc.textContent = carrier; > > > > We need to clear the l10n id when setting textContent directly or it is > > localized when changing languages. > > The element does not have l10n id, there is no need to clear it. > > > @@ +901,5 @@ > > > + iccManager.addEventListener('iccundetected', > > > + function iccUndetectedHandler(evt) { > > > + if (iccId === evt.iccId) { > > > + disableSIMRelatedSubpanels(true); > > > + mobileConnections[0].removeEventListener('radiostatechange', > > > > We don't need to remove the listener when "iccundetected". > > We do. `mobileConnectons[0]` exists even when the ICC card gets undetected > so we need to remove the event listener we added previously. Is there anything bad happens if we don't remove the listener? We use the event to enable/disable the menu items no matter the ICC card is available or not. It seems we can only add the listener once and no need to remove it. > > (In reply to Arthur Chen [:arthurcc] from comment #39) > > Comment on attachment 8343104 [details] [diff] [review] > > v7 > > > > Review of attachment 8343104 [details] [diff] [review]: > > ----------------------------------------------------------------- > > Thanks for the review! > > > ::: apps/settings/js/carrier.js > > @@ +125,5 @@ > > > + cs_showCarrierName(); > > > + cs_disabeEnableDataCallCheckbox(); > > > + return; > > > + } > > > + if (!e.detail.current.startsWith('#carrier-') || > > > > Suggest to move this check before checking mobile connection as there is a > > higher chance of early return resulted from these conditions. > > I do prefer to keep the logic a it is right now. It seems easier to > understand. The first thing to do in almost all listeners of the 'panelready' event is to check if the event is for the current panel. Not doing this in the first place may have slight performance impact on navigating panels. > > > @@ +914,5 @@ > > > + var apnsForIccCard = apns[iccCardIndex]; > > > + for (var apnIndex = 0; apnIndex < apnsForIccCard.length; apnIndex++) { > > > + var apn = apnsForIccCard[apnIndex]; > > > + if (apn.types.indexOf(type) !== -1) { > > > + if (apn.types.length > 1) { > > > > If the new apn setting has exactly the same value as the original settings > > but with different type, we only need to add a new type to the original apn > > item instead of creating a new one. > > > > And it would be better to have a comment describing the behaviors here. > > Well handling APNs it's a bit tricky. I've added as many comments as > possible.

Comment on attachment 8343704 [details] [diff] [review] v8 Review of attachment 8343704 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/carrier.js @@ +77,3 @@ > > + // Set the navigation correctly when on a multi ICC card device. > + if (DsdsSettings.getNumberOfIccSlots() === MAX_NUMBER_OF_ICC_SLOTS) { Use DsdsSettings.getNumberOfIccSlots() > 1. @@ +926,5 @@ > + // APNs. We need to keep the existing APN for those types. > + // Delete the type of APN that we are modifying from the existing > + // APN and create a new APN for the type we need to modify and add > + // it to the set of APNs for the ICC card. > + var tmpApn = JSON.parse(JSON.stringify(apn)); If we have an APN setting with type "default", and users try to set exactly the same APN setting bug with type "supl", the result of the current logic seems two APN settings. The correct result should be an APN setting with types "default" and "supl", right?

(In reply to Arthur Chen [:arthurcc] from comment #42) > Comment on attachment 8343704 [details] [diff] [review] > v8 > > Review of attachment 8343704 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +926,5 @@ > > + // APNs. We need to keep the existing APN for those types. > > + // Delete the type of APN that we are modifying from the existing > > + // APN and create a new APN for the type we need to modify and add > > + // it to the set of APNs for the ICC card. > > + var tmpApn = JSON.parse(JSON.stringify(apn)); > > If we have an APN setting with type "default", and users try to set exactly > the same APN setting bug with type "supl", the result of the current logic > seems two APN settings. The correct result should be an APN setting with > types "default" and "supl", right? Two APN set with different types are also valid APNs.

Comment on attachment 8344219 [details] [diff] [review] v9 Review of attachment 8344219 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Thank you for the great effort! Please land the code with a green travis (after gaia tree is reopen :( ) Please also ni? fabrice for approval.

Fabrice, would you mind to take a look at this PR please? If you are OK with it I'll land it once Travis goes green and the tree is open. Thanks!

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #46) > Created attachment 8344380 [details] > Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/14489 > > Fabrice, would you mind to take a look at this PR please? If you are OK with > it I'll land it once Travis goes green and the tree is open. Thanks! I can't take a decision base on the PR only, especially with so many changes. Please help me with the following information: - is the code fairly isolated? ("I want to refactor the RIL worker" would not pass). - has this code caused churn or pain before? - is it easy to turn the feature off? - do we have good test coverage? - do we have good QA coverage, ie in smoketests? - do we have strong partner expectations for the feature?

We really need to know when the bug can be landed, because DSDS is a committed feature. Can you help to provide the ETA? Thank you.

(In reply to Fabrice Desré [:fabrice] from comment #47) > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #46) > > Created attachment 8344380 [details] > > Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/14489 > > > > Fabrice, would you mind to take a look at this PR please? If you are OK with > > it I'll land it once Travis goes green and the tree is open. Thanks! > > I can't take a decision base on the PR only, especially with so many > changes. Please help me with the following information: > - is the code fairly isolated? ("I want to refactor the RIL worker" > would not pass). > - has this code caused churn or pain before? > - is it easy to turn the feature off? @Jose, could you reply above three questions? > - do we have good test coverage? > - do we have good QA coverage, ie in smoketests? @Enpei, could you reply above two questions? > - do we have strong partner expectations for the feature? Yes. this is blocker to partner's testing

(In reply to Wesley Huang [:wesley_huang] from comment #49) > > - do we have good test coverage? > > - do we have good QA coverage, ie in smoketests? > @Enpei, could you reply above two questions? Hi, this bug will relate to bug 926350 user story. There are test cases in moztrap for it. But the user story is ready, so related test cases have not been executed yet. QA has daily smoke test on v1.3 on Buri, that could capture regression on single SIM.

(In reply to Fabrice Desré [:fabrice] from comment #47) > - is the code fairly isolated? ("I want to refactor the RIL worker" > would not pass). Changes living in the setting app. Big patch to be honest. > - has this code caused churn or pain before? Not specially, just a few changes motivated by the new use cases. > - is it easy to turn the feature off? We can just back it out to turn it off. > - do we have good test coverage? To be honest no unit test at all. This part of the setting app has never had unit tests. > - do we have good QA coverage, ie in smoketests? No idea, ni? Enpei. > - do we have strong partner expectations for the feature? Kevin to provide an answer here. Requesting ni? then.

For QA part already replied in comment 50.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #51) > (In reply to Fabrice Desré [:fabrice] from comment #47) > > > - do we have strong partner expectations for the feature? > > Kevin to provide an answer here. Requesting ni? then. This is a serious blocker for DSDS. I would say yes.

Can we have a QA smoketest before deciding whether to land on 1.3? thanks! (ni? me again once it's done).

New version of the patch per comments at [1] [2]. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=928294#c15 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=928294#c16

(In reply to Fabrice Desré [:fabrice] from comment #54) > Can we have a QA smoketest before deciding whether to land on 1.3? thanks! > (ni? me again once it's done). Hi Fabrice, do you mean that having smoketest on master(not v1.3) with the fix on both single and dual SIM projects?

Please land the fixes on master build then test will be arranged.

(In reply to Enpei from comment #56) > (In reply to Fabrice Desré [:fabrice] from comment #54) > > Can we have a QA smoketest before deciding whether to land on 1.3? thanks! > > (ni? me again once it's done). > > Hi Fabrice, do you mean that having smoketest on master(not v1.3) with the > fix on both single and dual SIM projects? Yes, master is close enough to 1.3 these days to get a good idea of where we are.

Comment on attachment 8345630 [details] [diff] [review] v10 Review of attachment 8345630 [details] [diff] [review]: ----------------------------------------------------------------- There are still chance to get null mobileConnection with this patch. A more robust way is disabling the items by default. Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=928294#c18.

Cell and data item is enabled once the navigation is set. I've not seen mozMobileConnection being null and I even tried to be fast for clicking on the item to check whether mozMobileConnection could be null. Anyway, I did my test with Peak device, not sure about different behaviors between devices on this matter. No problem at all for fugu device. Could you give a try please? Thanks Arthur!

I'm not able to apply the patch to the current master. Could you rebase to master and create a pull request so that we can test it on travis?

Comment on attachment 8346132 [details] [diff] [review] v11 Review of attachment 8346132 [details] [diff] [review]: ----------------------------------------------------------------- This patch fixed my problem. However, I just found another critical issue. It should be easy to fix. After that, I think we are ready to land! ::: apps/settings/elements/carrier_iccs.html @@ +7,5 @@ > + </header> > + > + <div> > + <ul> > + <li id="menuItem-sim1" aria-disabled="true" class="hint"> We use the same ID in call_iccs.html. Suggest to use class name.

(In reply to Arthur Chen [:arthurcc] from comment #62) > Comment on attachment 8346132 [details] [diff] [review] > v11 > > Review of attachment 8346132 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks. > ::: apps/settings/elements/carrier_iccs.html > @@ +7,5 @@ > > + </header> > > + > > + <div> > > + <ul> > > + <li id="menuItem-sim1" aria-disabled="true" class="hint"> > > We use the same ID in call_iccs.html. Suggest to use class name. I'll take care of it.

New version of the patch. PR updated also.

Comment on attachment 8346445 [details] [diff] [review] v12 Review of attachment 8346445 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Thanks!

Travis goes green with the PR and everything seems to work fine for both peak and fugu devices. https://github.com/mozilla-b2g/gaia/commit/bf7b651dd3c734e68b73f394649733d07c421d14

removing status-b2g-v1.3 as affected till performing QA smoketest in master (ni to QA since the patch is already ready there) and the confirmation from Fabrice that it can be uplifted to v1.3 branch

Smoke test has been executed on Gaia 59b89715c92d0383fb3d09941db93af5dfd05bc8 Gecko http://hg.mozilla.org/mozilla-central/rev/0fdbcbffc79e BuildID 20131215040201 Version 29.0a1 No setting related blockers. Found several major bugs but don't feel it's related to this landed feature. Here are those major bugs: bug 950581(camera), bug 950589(youtube), bug 950558(cost control). Based on the test result, it should be fine to be laned in v1.3 as long as no listed bugs above are related.

Ok to land on 1.3

Heard that this patch is not cleanly applied to 1.3, could you rebase to make it into 1.3 today?

(In reply to Wesley Huang [:wesley_huang] from comment #71) > Heard that this patch is not cleanly applied to 1.3, could you rebase to > make it into 1.3 today? This patch should apply cleanly on v1.3 branch. It seems there was an issue while uplifting bug 933203 see https://bugzilla.mozilla.org/show_bug.cgi?id=933203#c80 please.

(In reply to José Antonio Olivera Ortega [:jaoo](PTO 12/20 ~01/07)) from comment #72) > (In reply to Wesley Huang [:wesley_huang] from comment #71) > > Heard that this patch is not cleanly applied to 1.3, could you rebase to > > make it into 1.3 today? > > This patch should apply cleanly on v1.3 branch. It seems there was an issue > while uplifting bug 933203 see > https://bugzilla.mozilla.org/show_bug.cgi?id=933203#c80 please. Hi John, just remarking that this patch should be uplifted to v1.3 branch after uplifting bug 933203. Thanks!

[v1.3 f00177c] Merge pull request #14489 from jaoo/928297

Verified on Fugu, passed. Gaia a119a0692c24c5ed7c55bab838bae3ecdb9dbec9 Gecko 15ee4e78431b45922b41dea882464b0ccb6b4fac BuildID 20140110174141 Version 28.0a2