Gecko http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d Gaia e2ef782119b7e79fc62c48d36f0c36909d982988 BuildID 20130712070210 Version 18.0 Steps to reproduce 1.Performed a factory reset; 2.Advance to get date & time; 3.Check the continent and city/country. Expected behavior: The handset should display the continent default: "America" and city: Los Angeles -0 at least in my case. Actual: The handset displays America and New York as default.

(In reply to John Hammink from comment #0) > The handset displays America and New York as default. Yep, Andreas asked to use America/New_York by default. :-) If a SIM card is detected and the MCC/MNC information can be read, we propose a default city for each country — so if you’re using an American SIM card, New York will be proposed by default. That’s a feature, and there’s not much we can do to improve it so far. However, users with non-American SIM cards can reach the Date/Time screen in the FTE app before the MCC/MNC information is read; in which case, America/New_York is selected by default, which is not optimal. Maybe we should wait for the MCC/MNC info to be available before displaying/enabling the timezone selector. Anyway, this is also sub-optimal when NITZ is available — that’s why I suggest to remove this timezone selection screen entirely in bug 826359 when NITZ is detected.

Taking the bug to keep it on my radar, but everybody is welcome to steal it.

(In reply to Fabien Cazenave [:kaze] from comment #1) > (In reply to John Hammink from comment #0) > > The handset displays America and New York as default. > > Yep, Andreas asked to use America/New_York by default. :-) > > If a SIM card is detected and the MCC/MNC information can be read, we > propose a default city for each country — so if you’re using an American SIM > card, New York will be proposed by default. That’s a feature, and there’s > not much we can do to improve it so far. > > However, users with non-American SIM cards can reach the Date/Time screen in > the FTE app before the MCC/MNC information is read; in which case, > America/New_York is selected by default, which is not optimal. Maybe we > should wait for the MCC/MNC info to be available before displaying/enabling > the timezone selector. Anyway, this is also sub-optimal when NITZ is > available — that’s why I suggest to remove this timezone selection screen > entirely in bug 826359 when NITZ is detected. We should have the information available before we reach the timezone selector for FTU. It should even be available for the default language screen as well.

In triage I think we actually meant to minus this.

So after few tests with different simcards (thanks Beatriz!), I discovered something: - When the SIM card is pin protected, as we are unable to retrieve mcc/mnc from the card, all the process (launched at FTU startup) ends up selecting the hardcoded default timezone (America/New York), which is acceptable. Problem is that when user inserts correct pin, this process is not called again, so we end up with new york even if now we have access to mcc/mnc data. - There's a setting called "time.timezone.user-selected" that is supposed to keep track of the user changes on the timezone, so if the user does change it, we keep using that one on next reboot, even if mcc/mnc indicates another timezone. Problem is, with our current code, we are storing a timezone in there even if the user didn't do anything, so it doesn't work the way it is supposed to. (bug 847342) So I have a few questions: - Should we solve the two problems in this bug, or split in two different ones?. - Is the flow we are using still the correct one or should we change it? 1. on FTU startup, check mobile connection. If we have, take mcc/mnc from it. If not, from SIM card. 1.1. [needs fix] if SIM is pin protected, wait till SIM ready, then repeat the process from step 1. 2. based on mcc/mnc, guess a timezone from the JSON file. 3. show the data&time screen with the guessed tz selected. If user changes it, keep the new one on next reboot [needs fix]. If not, repeat the process. There's other bugs related with this, that might affect this flow a little, e.g. Bug 1026098 - Skip Time&Date screen (if we can get the time automatically). Bug 820696 - Let the user choose between automatic or manual date on FTU (and sync that with settings).

IMHO we should fix the first issue explained by :fcampo in comment 5 section 1.1(or in first paragraph). This is the most common use case and reported during certification. Regarding the second fix... maybe file a new bug is the best option.

(In reply to Fernando Campo (:fcampo) from comment #5) > So after few tests with different simcards (thanks Beatriz!), I discovered > something: > - When the SIM card is pin protected, as we are unable to retrieve mcc/mnc > from the card, all the process (launched at FTU startup) ends up selecting > the hardcoded default timezone (America/New York), which is acceptable. > Problem is that when user inserts correct pin, this process is not called > again, so we end up with new york even if now we have access to mcc/mnc data. > > - There's a setting called "time.timezone.user-selected" that is supposed to > keep track of the user changes on the timezone, so if the user does change > it, we keep using that one on next reboot, even if mcc/mnc indicates another > timezone. Problem is, with our current code, we are storing a timezone in > there even if the user didn't do anything, so it doesn't work the way it is > supposed to. (bug 847342) I cannot see what caused this after I took anther look at shared/js/tz_select.js after not touching that code for a while, this "time.timezone.user-selected" should be updated when you change the time zone manually with changing the value selector, either by region or city.

(In reply to Beatriz Rodríguez [:brg] from comment #6) > IMHO we should fix the first issue explained by :fcampo in comment 5 section > 1.1(or in first paragraph). This is the most common use case and reported > during certification. > Regarding the second fix... maybe file a new bug is the best option. After playing with the code trying to get one fixed, I think they're so coupled that is not a good idea to split it in two bugs. I'm trying to provide a unique patch to solve all. (In reply to Rudy Lu [:rudyl] from comment #7) > I cannot see what caused this after I took anther look at > shared/js/tz_select.js after not touching that code for a while, this > "time.timezone.user-selected" should be updated when you change the time > zone manually with changing the value selector, either by region or city. I don't know if a change provoked this or it was working like that from the beginning (I need to make some tests with old versions for that), but the problem I see now is that the the update of the selector is triggered at least once on the beginning without user interaction. When we load the JSON file, we fill the regions, that fill the cities, and that sets a new timezone, setting the 'user-selected' variable at startup (and with the wrong timezone if the sim is pin locked). I'm trying to change this on my patch.

WIP - still working on the tests, which is taking a little longer than expected because there wasn't any for the timezone updating process, and everything is in shared, not in ftu. Asking for feedback to Rudy cause he was working on it before, and changes could affect settings side.

Tested using a Unagi master: Gecko: c0de5b Gaia eca491b I follow the steps to reproduce the bug, but I can see that its working fine. The handset display “Europe” and “Madrid”, the correct info for me.

Previous comment forgot to say that test was done included FCampo's patch. Sorry for the inconvenience!

(In reply to Fernando Campo (:fcampo) from comment #8) > (In reply to Beatriz Rodríguez [:brg] from comment #6) > > IMHO we should fix the first issue explained by :fcampo in comment 5 section > > 1.1(or in first paragraph). This is the most common use case and reported > > during certification. > > Regarding the second fix... maybe file a new bug is the best option. > After playing with the code trying to get one fixed, I think they're so > coupled that is not a good idea to split it in two bugs. I'm trying to > provide a unique patch to solve all. > > > (In reply to Rudy Lu [:rudyl] from comment #7) > > I cannot see what caused this after I took anther look at > > shared/js/tz_select.js after not touching that code for a while, this > > "time.timezone.user-selected" should be updated when you change the time > > zone manually with changing the value selector, either by region or city. > I don't know if a change provoked this or it was working like that from the > beginning (I need to make some tests with old versions for that), but the > problem I see now is that the the update of the selector is triggered at > least once on the beginning without user interaction. When we load the JSON > file, we fill the regions, that fill the cities, and that sets a new > timezone, setting the 'user-selected' variable at startup (and with the > wrong timezone if the sim is pin locked). I'm trying to change this on my > patch. Yes, just tested this with a SIM card available, the "user-selected" would be updated in this case, which is not as expected. And I think it might be caused by this line, https://github.com/mozilla-b2g/gaia/blob/94245d2f0b50339450fefda7ce56db47f9e36921/shared/js/tz_select.js#L163 -- If this is not an urgent request on v2.0, I would suggest we refactor this part of code to an instantiable object with public interface, so that we could add unit test to this module, and we might need to mock the RIL related dependencies out, also for testing. Thanks.

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 Just briefly took a look at the the code, and I think it should be working, but still suggest we refactor this as my previous comment. Thanks.

(In reply to Rudy Lu [:rudyl] from comment #13) > Comment on attachment 8451785 [details] > Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 > > Just briefly took a look at the the code, and I think it should be working, Thanks for the f+ Rudy, just one question: would you be able to review this fully, or do you know who I can assign for it? > but still suggest we refactor this as my previous comment. > Thanks. Problem is that this is a certification blocker at the moment, so it's quite urgent (Beatriz might be able to give more details). I'd rather to have this landed asap, and I'll open a followup for the refactor.

I don't think I am qualified to give r+ on this change since it lives in shared/js and should belong to the "Gaia" module, you might need to find owner/peer under Gaia section to review this, https://wiki.mozilla.org/Modules/FirefoxOS#Gaia Thanks.

(In reply to Fernando Campo (:fcampo) from comment #14) > (In reply to Rudy Lu [:rudyl] from comment #13) > > but still suggest we refactor this as my previous comment. > > Thanks. > Problem is that this is a certification blocker at the moment, so it's quite > urgent (Beatriz might be able to give more details). I'd rather to have this > landed asap, and I'll open a followup for the refactor. Yes. This was a cert waiver and if the patch has low risk we should ask for approval to 2.0 branch.

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 Updated the PR after Rudy's comments on github. As per Beatriz's comment 16, I opened bug 1036414 to refactor the current code so we are able to make some tests. At the moment is quite difficult to do anything, as everything happens inside shared/tz_select.js. I'm adding Francisco and Arthur as reviewers as the changes affects both FTU and Settings, but not sure if that's the proper path, so please delegate if needed.

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 Looks good to me, thanks!

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 Sorry, after checked the code again I realized that 'newTZObserver' should not be called twice. What to do when the sim card becomes available should only be updating the selectors instead of initializing them again.

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 Updated PR, now I only ask for the default timezone, but had to change the listener from 'cardstatechange' to 'iccinfochange' because of timing (even if card is 'ready', the info is not available for a few moments, and didn't want to use a timeout). Manual tests on device seemed to work both on FTU and Settings, but please review again.

This is awesome. Thanks!

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 r=me with the comment addressed.

Can I haz unit test?

Sorry, didn't add feedback. Code wise LGTM, testing on the device works, but will be happy if we add unit test to check when the change was triggered by the user or by the sim.

you haz your cheezeburger now, sir, code updated.

Comment on attachment 8451785 [details] Link to PR - https://github.com/mozilla-b2g/gaia/pull/21459 Great job Fernando!

waiting for travis to merge, but it looks we have a permared :(

green and merged. master - d4583df788d33119f1fc4c68dca40a986a9bf18b thanks all for help and patience :)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] (feature/regressing bug #): wrong auto selection of timezone [User impact] if declined: user with SIM won't get the timezone correctly [Testing completed]: unit test, device [Risk to taking this patch] (and alternatives if risky): medium-low risk, we are dealing with timezone selection files, affects FTE and Settings app [String changes made]: none

Tested using Flame - master- Gecko-c431020.Gaia-1c9eb3d, with a Movistar ES SIM card with PIN code activated: Europe/Madrid is properly set in the FTE. Thanks for the work to fix this issue. Bug marked as Verified. I would like to add that this feature is a cert waiver in 1.1, 1.3.... and due to the low risk, we would like to see it working in 2.0.

Comment on attachment 8459462 [details] 2.0 uplift request Given that it was past cert waivers providing approval for 2.0

v2.0: https://github.com/mozilla-b2g/gaia/commit/05ca48fe797e03c18f91e3779489ab9c38f6363f

This issue has been verified successfully on Flame v2.0 & v2.1 STR: 0. Insert a SIM card. 1. Performed a factory reset; 2. Next to get date & time; 3. Check the continent and city/country. **The city/country shows agrees with SIM card. See attachment: verify_video.MP4 Reproducing rate: 0/5 Flame 2.0 versions: Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3 Build-ID 20141127000203 Version 32.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141127.034818 FW-Date Thu Nov 27 03:48:29 EST 2014 Bootloader L1TC00011880 Flame 2.1 versions: Gaia-Rev 5372b675e018b6aac97d95ff5db8d4bd16addb9b Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b Build-ID 20141127001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141127.035005 FW-Date Thu Nov 27 03:50:16 EST 2014 Bootloader L1TC00011880