Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Description • 11 years ago

      No description provided.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 1 • 11 years ago

Part of v1.3 DSDS feature.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 2 • 11 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 3 • 11 years ago

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

	Anshul
	Comment 4 • 11 years ago

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

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 5 • 11 years ago

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

	Anshul
	Comment 6 • 11 years ago

Thank you very much cawang and Antonio?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 7 • 11 years ago

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.

	Noemí Freire (:noemi)
	Comment 8 • 11 years ago

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

	Kai-Chih Hu [:khu]
	Comment 9 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 10 • 11 years ago

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?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 11 • 11 years ago

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.

	Kai-Chih Hu [:khu]
	Comment 12 • 11 years ago

Arthur, when can you finish the review?

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 13 • 11 years ago

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

	Kai-Chih Hu [:khu]
	Comment 14 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 15 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 16 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 17 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 18 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 19 • 11 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 20 • 11 years ago

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!

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 21 • 11 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 22 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Reporter
	Comment 24 • 11 years ago

Comment on attachment 8346454 [details] [diff] [review]
v5

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

r=me. Thank you for the effort!

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 25 • 11 years ago

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

	Noemí Freire (:noemi)
	Comment 26 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 27 • 11 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 28 • 11 years ago

Travis went green after landing the patch.

https://travis-ci.org/mozilla-b2g/gaia/builds/15400995

	[:fabrice] Fabrice Desré
	Comment 29 • 11 years ago

Approval to land on 1.3 granted.

	Noemí Freire (:noemi)
	Comment 30 • 11 years ago

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

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 31 • 11 years ago

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

	Enpei
	Comment 32 • 11 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 33 • 11 years ago

Ok to land on 1.3

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 34 • 11 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 35 • 11 years ago

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.

	Noemí Freire (:noemi)
	Comment 36 • 11 years ago

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

	Anshul
	Comment 37 • 11 years ago

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.

	Anshul
	Comment 38 • 11 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 39 • 11 years ago

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

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 40 • 11 years ago

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

	Enpei
	Comment 41 • 11 years ago

Verified on Fugu, passed.

Gaia      a119a0692c24c5ed7c55bab838bae3ecdb9dbec9
Gecko     15ee4e78431b45922b41dea882464b0ccb6b4fac
BuildID   20140110174141
Version   28.0a2