Part of v1.3 DSDS feature.

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, Call settings, slide #52.

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #2) > 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, Call settings, slide #52. Antonio, could you please point me to the location of the spec so I know what to expect?

Hi, I'm the UX of DSDS and I've attached the spec here. Please take a look. Thanks!

Thank you very much cawang and Antonio?

First WIP patch. More or less working. Needs more test once the DSDS support for mozIccManager API gets landed. I still need to deal with the FDN part.

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

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

Arthur, would you mind to review this patch please? This should be applied on top of bug 928297. This patch has the same design that the one from bug 928297 so you will find the review easier. I've not touched the FDN part as I'm not familiar with it and I'm not sure if it is part of this bug. IMHO we should adapt the FDN feature to multi ICC card devices in another bug since it make thing easier. This bug has a close relationship with bug 921391, so what's going on that bug? Is it planned to fix it for v1.3?

Arthur, would you mind to review this patch please? This should be applied on top of bug 928297. This patch has the same design that the one from bug 928297 so you will find the review easier. I've not touched the FDN part as I'm not familiar with it and I'm not sure if it is part of this bug. IMHO we should adapt the FDN feature to multi ICC card devices in another bug since it make thing easier.

Arthur, when can you finish the review?

Kevin, I'll finish the first pass of the review today.

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.

Comment on attachment 8343811 [details] [diff] [review] v2 Review of attachment 8343811 [details] [diff] [review]: ----------------------------------------------------------------- Still reviewing call.js and call_iccs.js. ::: apps/settings/js/connectivity.js @@ +200,5 @@ > > + // Only show the description for single ICC card devices. In case of multi > + // ICC card device the description to show for the ICC cards will be handled > + // in the call_iccs.js file. > + if (mobileConnections.length === 2) { Use mobileConnections.length > 1 to align the "updateCellAndDataDescription" function. @@ +211,5 @@ > + > + if (!mobileConnections[0].iccId || > + (mobileConnections[0].radioState !== 'enabled')) { > + var airplaneMode = (mobileConnection.radioState === 'disabled'); > + var cardState = airplaneMode ? 'null' : 'absent'; We cannot use `absent` here as when iccId is null, the actual card state maybe `pinRequired` or other card locked states. @@ +221,5 @@ > + var cardState = iccCard.cardState; > + localize(callDesc, kCardStateL10nId[cardState || 'null']); > + } > + > + function handleUIAndAddListeners() { Sorry I didn't notice this function design when reviewed the patch of 928297. I would suggest not to add a calling to `showCallDescription` here (we did it in the `updateCellAndDataDescription` function) and rename the function to `addListeners`, which makes the code more readable. @@ +231,5 @@ > + showCallDescription); > + } > + > + if (!mobileConnections[0].iccId) { > + showCallDescription(); Same reason here. We should call to this function no matter we have iccId or not. @@ +256,1 @@ > nit: remove this line. ::: apps/settings/js/dsds_settings.js @@ +58,5 @@ > + var callItem = document.getElementById('menuItem-callSettings'); > + callItem.setAttribute('href', '#call-iccs'); > + return; > + } > + nit: remove this line.

Comment on attachment 8343811 [details] [diff] [review] v2 Review of attachment 8343811 [details] [diff] [review]: ----------------------------------------------------------------- Jose, thank you for the patch! Please check my comments inline. One major issue was if we click on call settings or cellular & data items right after settings app launched, the SIM card items are grayed out and never become normal. Logs for your reference. The null problem of _mobileConnections occurs on both fugu and unagi. E/GeckoConsole( 532): [JavaScript Error: "TypeError: _mobileConnections is null" {file: "app://settings.gaiamobile.org/js/dsds_settings.js" line: 35}] E/GeckoConsole( 532): [JavaScript Error: "TypeError: _mobileConnections is null" {file: "app://settings.gaiamobile.org/js/dsds_settings.js" line: 35}] E/GeckoConsole( 532): [JavaScript Error: "TypeError: desc is undefined" {file: "app://settings.gaiamobile.org/js/carrier_iccs.js" line: 137}] E/GeckoConsole( 532): [JavaScript Error: "TypeError: desc is undefined" {file: "app://settings.gaiamobile.org/js/carrier_iccs.js" line: 137}] E/GeckoConsole( 532): [JavaScript Error: "TypeError: desc is undefined" {file: "app://settings.gaiamobile.org/js/carrier_iccs.js" line: 137}] ::: apps/settings/js/call.js @@ +76,5 @@ > + cs_initVoiceMailSettings(); > + cs_initVoicePrivacyMode(); > + cs_initCallWaiting(); > + cs_initCallerId(); > + cs_initCallForwarding(); We should call to cs_displayInfoForAll(_('callSettingsQuery')); as we removed it from "cs_initCallForwarding". @@ +376,5 @@ > if (_cfReasonStates[_cfReasonMapping[key]] == event.settingValue) { > return; > } > + // // Bails out in case of airplane mode. > + // if (IccHelper.cardState !== 'ready') { Use _iccManager and _mobileConnection to get the current mozIcc object and check the card state. @@ +475,5 @@ > } > + cs_displayRule(cfOptions['unconditional'], 'cfu-desc', 'unconditional'); > + cs_displayRule(cfOptions['mobilebusy'], 'cfmb-desc', 'mobilebusy'); > + cs_displayRule(cfOptions['noreply'], 'cfnrep-desc', 'noreply'); > + cs_displayRule(cfOptions['notreachable'], 'cfnrea-desc', 'notreachable'); nit: indent error. ::: apps/settings/js/call_iccs.js @@ +79,5 @@ > + for (var i = 0; i < numberOfIccCards; i++) { > + var mobileConnection = _mobileConnections[i]; > + if (!mobileConnection.iccId) { > + var airplaneMode = (mobileConnection.radioState === 'disabled'); > + var state = airplaneMode ? 'null' : 'absent'; Use the current card state instead of 'absent'. Please also modify the code in "carrier_iccs.js". @@ +87,5 @@ > + } > + handleUIAndAddListeners(mobileConnection.iccId); > + } > + > + // This code might not be needed. Please remove the comment @@ +94,5 @@ > + var iccId = evt.iccId; > + handleUIAndAddListeners(iccId); > + }); > + > + // This code might not be needed. Please remove the comment

New version of the patch per comments #15 and #16.

Comment on attachment 8345631 [details] [diff] [review] v3 Review of attachment 8345631 [details] [diff] [review]: ----------------------------------------------------------------- There are chances that we enter the call settings panel directly by clicking on "Call settings" on a multi-sim device. We need to disable "Call settings" and "Cellular & Data" items by default and re-enable them when ready. ::: apps/settings/js/connectivity.js @@ +259,5 @@ > + > + iccManager.addEventListener('iccundetected', > + function iccUndetectedHandler(evt) { > + if (iccId === evt.iccId) { > + var iccCard = iccManager.getIccById(iccId); "iccCard" is not used in this scope.

Comment on attachment 8345631 [details] [diff] [review] v3 Review of attachment 8345631 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/call.js @@ +35,5 @@ > > + var _ = window.navigator.mozL10n.get; > + var _settings = window.navigator.mozSettings; > + var _mobileConnections = window.navigator.mozMobileConnections; > + var _iccManager = window.navigator.mozIccManager; Remove _iccManager as we don't use it now.

Call settingsitem 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!

Comment on attachment 8346133 [details] [diff] [review] v4 Review of attachment 8346133 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/elements/call_iccs.html @@ +7,5 @@ > + </header> > + > + <div> > + <ul> > + <li id="menuItem-sim1" aria-disabled="true" class="hint"> We use the same ID in carrier_iccs.html. Suggest to use class name.

New version of the patch. I'll create the PR in a bit.

Comment on attachment 8346454 [details] [diff] [review] v5 Review of attachment 8346454 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Thank you for the effort!

Travis went green for all the jobs except for the gaia_ui_tests one. The failure I was seeing has been deactivated in bug 949897. https://github.com/mozilla-b2g/gaia/commit/b350c6d43154fddf7b4e6b83e6dfb8de6e28e4b5

removing status-b2g-v1.3 as affected till going through the approval process and the confirmation from Fabrice that it can be uplifted to v1.3 branch

Fabrice, would you mind to take a look at this PR please? This is somehow a request for approval. Find below the responses you usually do for other approvals that I have seen. > - 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? Wesley to provide an answer here. Requesting ni? then.

Travis went green after landing the patch. https://travis-ci.org/mozilla-b2g/gaia/builds/15400995

Approval to land on 1.3 granted.

Per comment 29, setting status-b2g-v1.3 as affected to track the uplift process to v1.3 branch. Thanks!

> > - do we have strong partner expectations for the feature? > > Wesley to provide an answer here. Requesting ni? then. Yes. This patch blocks vendor's testing.

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?

This patch should apply cleanly on v1.3 branch after laning bug 928297. 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 #35) > This patch should apply cleanly on v1.3 branch after laning bug 928297. 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!

Hey Antonio, I was now able to pull in this change but "Select a SIM card" menu shows that both the SIM cards aren't ready even though the phone has service on both the SIM cards. I am debugging it right now, but wanted to let you know what this change isn't working with commercial RIL.

Antonio, I think it is the same issue as I pointed in bug 951337 where iccid is not available just yet and connectivity.js doesn't register for icc-detected event.

(In reply to Anshul from comment #37) > Hey Antonio, I was now able to pull in this change but "Select a SIM card" > menu shows that both the SIM cards aren't ready even though the phone has > service on both the SIM cards. I am debugging it right now, but wanted to > let you know what this change isn't working with commercial RIL. Hey Anshul, this is working fine with the reference RIL implementation and the devices we have been working on for the development of DSDS features. If the commercial RIL implementation doesn't behaves the same this is really bad. Moreover we dont have access to the commercial RIL implementation for master branch and the multi ICC card devices you are using for the development of DSDS features in the commercial RIL implementation.

[v1.3 23170dd] Merge pull request #14604 from jaoo/928294

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