Please see https://bugzilla.mozilla.org/show_bug.cgi?id=944225#c15

Hey Antonio, would you be able to take this bug?

(In reply to Anshul from comment #1) > Hey Antonio, would you be able to take this bug? Yes, I would. Since we have been refactoring the gaia part where we would add the bits for this bug and that part has not landed yet I'll set the dependency accordingly. Once that part get landed I'll start working on this bug. Thanks!

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

Fist version, I'll request review for it once bug 928297 gets landed.

Comment on attachment 8346645 [details] Pointer to Github PR: https://github.com/mozilla-b2g/gaia/pull/14616 Oops, wrong bug. Sorry for the noise.

Comment on attachment 8346551 [details] [diff] [review] v1 Tested and it seems to work fine. See some log from the RIL plumbing at [1] please. Arthur, would you mind to have a look please? Thanks. [1] http://pastebin.mozilla.org/3769220

Hey Antonio, since Moz is going to be shutting down or might already have, wondering if you can push to get this change merged on 1.3 today?

(In reply to Anshul from comment #9) > Hey Antonio, since Moz is going to be shutting down or might already have, > wondering if you can push to get this change merged on 1.3 today? I'd love it but it depends on the review and the approval for 1.3. I'll do my best to address any review comment if needed but I cannot do anything else.

I'm reviewing the patch. However, we still need time to get the approval for v1.3.

Jessica, could you give a try and provide some feedback please? I'm seeing the network type doesn't change at all after calling `setPreferredNetworkType` function. The flow seems correct bug If I try to get the preferred network type after a change it gets the original one and not the new one. Give a try please, thanks.

Comment on attachment 8346551 [details] [diff] [review] v1 Please check my github comment. There is a major issue when updating the selector.

(In reply to Arthur Chen [:arthurcc] from comment #13) > Comment on attachment 8346551 [details] [diff] [review] > v1 > > Please check my github comment. There is a major issue when updating the > selector. Done, thanks!

Jessica, see comment #12 please.

More information. I'm only able to set certain types of network on the sim slots: sim 1: gsm, wcdma, wcdma/gsm-auto sim 2: gsm

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15) > Jessica, see comment #12 please. José, sorry I was OOO today and I have no device with me right now, I can test it tomorrow morning. Or could you provide logs with RIL log enabled so that I can take a look at it first? Thanks. Keeping the needinfo flag for tracking.

|adb logcat| output during a test (only the set/getPreferredNetworkType flow).

|adb logcat -b radio| output during a test (only the set/getPreferredNetworkType flow).

(In reply to Jessica Jong [:jjong] [:jessica] from comment #17) > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #15) > José, sorry I was OOO today and I have no device with me right now, I can > test it tomorrow morning. Or could you provide logs with RIL log enabled so > that I can take a look at it first? Thanks. Everything works correctly apparently. It seems the mode doesn't perform the change as the parcel requests. Weird, because it doesn't perform the change and there is no error or something telling that the change couldn't be performed. Hope it helps.

José, from the logs I see that you are changing the preferred network type of SIM2, have you tried it on SIM1? SIM2 has GSM support only, but it doesn't return error if you select another network type. So I think the results in Comment 16 are as expected.

(In reply to Arthur Chen [:arthurcc] from comment #11) > I'm reviewing the patch. However, we still need time to get the approval for > v1.3. What more approval is needed as this bug is already v1.3+?

(In reply to Jessica Jong [:jjong] [:jessica] from comment #21) > José, from the logs I see that you are changing the preferred network type > of SIM2, have you tried it on SIM1? SIM2 has GSM support only, but it > doesn't return error if you select another network type. So I think the > results in Comment 16 are as expected. Yes, that was with SIM2. SIM1 works correctly as I've seen. So, if what Arthur and I saw is correct that's it. Sorry for the noise.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #22) > (In reply to Arthur Chen [:arthurcc] from comment #11) > > I'm reviewing the patch. However, we still need time to get the approval for > > v1.3. > > What more approval is needed as this bug is already v1.3+? AFAIK all these late landings need to be approved by Fabrice.

Hey Fabrice, can you please clarify?

Arthur, Please provide your review for the same.

Everything works well except for we are not able to set the network type to wcdma/gsm on sim 1. I will wait for the feedback from the gecko side before giving r+ to the patch.

Comment on attachment 8347960 [details] [diff] [review] v2 Review of attachment 8347960 [details] [diff] [review]: ----------------------------------------------------------------- As gecko still observes the change of "ril.radio.preferredNetworkType", we should remove "name=ril.radio.preferredNetworkType" from the select element. And we should also modify system/js/emergency_callback_manager.js accordingly.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #23) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #21) > > José, from the logs I see that you are changing the preferred network type > > of SIM2, have you tried it on SIM1? SIM2 has GSM support only, but it > > doesn't return error if you select another network type. So I think the > > results in Comment 16 are as expected. > > Yes, that was with SIM2. SIM1 works correctly as I've seen. So, if what > Arthur and I saw is correct that's it. Sorry for the noise. We think modem should return error if it doesn't support a type or when the request fails. I have filed Bug 951083 for this. Thank you.

(In reply to Arthur Chen [:arthurcc] from comment #28) > Comment on attachment 8347960 [details] [diff] [review] > v2 > > Review of attachment 8347960 [details] [diff] [review]: > ----------------------------------------------------------------- > > As gecko still observes the change of "ril.radio.preferredNetworkType", we > should remove "name=ril.radio.preferredNetworkType" from the select element. > And we should also modify system/js/emergency_callback_manager.js > accordingly. Wow, I wasn't aware that the system app needed to know about preferred network type at all. New version of the patch coming. Thanks Arthur!

(In reply to Arthur Chen [:arthurcc] from comment #28) > Comment on attachment 8347960 [details] [diff] [review] > v2 > > Review of attachment 8347960 [details] [diff] [review]: > ----------------------------------------------------------------- > > As gecko still observes the change of "ril.radio.preferredNetworkType", we > should remove "name=ril.radio.preferredNetworkType" from the select element. Done, that part was easy. > And we should also modify system/js/emergency_callback_manager.js > accordingly. I got rid of the 'ril.radio.preferredNetworkType' setting. I left a message in the code about the change made, read it please. My concern is about the emergency callback manager has not support for multi ICC card devices so doing more or anything else is out of the context of this bug. Moreover I've added some bits for handling errors (basically to show an alert) while setting a new network type as the modem will return an error if it doesn't support a type or when the request fails.

Jessica, I tested this on unagi and found that if we set preferred network type to CDMA related types it still returns success. Just FYI that the problem seems not only limited to fugu.

Comment on attachment 8349005 [details] [diff] [review] v3 Review of attachment 8349005 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! r=me with the comment addressed. And I would like the newly added string be reviewed before merging. Francesco, could you help review the string, "Network type could not be set."? Thanks! ::: apps/settings/elements/carrier_operator_settings.html @@ +58,5 @@ > + <strong id="preferredNetworkTypeAlertMessage"></strong> > + </p> > + </section> > + <menu> > + <button class="full" data-l10n-id="continue">Continue</button> After confirmed with the UX designer, please use OK instead of continue. ::: apps/settings/locales/settings.en-US.properties @@ +278,5 @@ > # in headers, you can use a shorter string here. > networkOperator-header={{networkOperator}} > availableOperators=Network operators in the area > +preferredNetworkTypeAlertTitle = Preferred network type > +preferredNetworkTypeAlertErrorMessage = Network type could not be set Please add a period in the end of the sentence.

Comment on attachment 8349005 [details] [diff] [review] v3 Review of attachment 8349005 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a native English speaker, so I shouldn't really be reviewing strings ;-) Having said that, patch looks good to me.

Gaia tree is closed. Travis went green in the PR, I'll merge it once the tree is open again. In the meantime I'd like to ask for 1.3 approval. 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 mostly living in the setting app. > - has this code caused churn or pain before? Nope IIRC. > - 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.

Tree open, Travis went green. Landed on Gaia master branch. https://github.com/mozilla-b2g/gaia/commit/5d53d86675a91ced588e10239dbe136b78d528cc

José Antonio, feel free to uplift to 1.3

(In reply to José Antonio Olivera Ortega [:jaoo](PTO 12/20 ~01/07)) from comment #35) > > - do we have strong partner expectations for the feature? > > Wesley to provide an answer here. Requesting ni? then. Yes, this is blocking partner testing.

(In reply to Arthur Chen [:arthurcc] (PTO 12/25 ~ 1/5) from comment #32) > Jessica, I tested this on unagi and found that if we set preferred network > type to CDMA related types it still returns success. Just FYI that the > problem seems not only limited to fugu. Oh, modem return is returning error in this case, it's gecko's bug, I have filed Bug 951958 for this and will provide the fix by today. Thanks.

quick test on Buri master build. No major bug found. As for bug 951958 since the direct error user will see is that after choose CDMA type, exit the setting and enter it again, it will restore back to previous one other than "CDMA", this is not a real case user will do because CDMA related items should be removed from the list already. So the impact is low.

What is the ETA of getting this uplifted to 1.3?

v1.3: 3508d5c030f3c3474a6566140aa663d07801d333