Closed
Bug 974253
Opened 11 years ago
Closed 11 years ago
Preferred network type setting will not be saved if reboot DUT right after change the setting.
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: arthurcc, Assigned: arthurcc)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
jaoo
:
review+
|
Details |
(deleted),
text/html
|
fabrice
:
approval-gaia-v1.3+
|
Details |
Preferred network type setting cannot be saved after reboot. It will be restored back to the default value. This can only be reproduced on fugu as its gonk does not preserve the settings. However, gaia should always save the user preference in this case.
* Reproduce Steps
1. Set preferred network type to any item other than the default value.
2. Reboot the device.
* Expected Result
New preferred network type setting should be saved even reboot the device.
* Actual Result
New preferred network type setting is not saved.
* Occurrence rate
100%
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → arthur.chen
Assignee | ||
Comment 1•11 years ago
|
||
WIP, waiting for bug 946589 landing.
Assignee | ||
Updated•11 years ago
|
Target Milestone: --- → 1.4 S2 (28feb)
Assignee | ||
Updated•11 years ago
|
Target Milestone: 1.4 S2 (28feb) → ---
Assignee | ||
Comment 2•11 years ago
|
||
Nominate for 1.3T as it blocks a 1.3T blocker.
blocking-b2g: --- → 1.3T?
Assignee | ||
Comment 4•11 years ago
|
||
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477
Jose, Alive, could you help review the patch? Which is similar to other telephony settings. Thanks!
Attachment #8378903 -
Flags: review?(josea.olivera)
Attachment #8378903 -
Flags: review?(alive)
Updated•11 years ago
|
Attachment #8378903 -
Flags: review?(alive) → review+
Comment 5•11 years ago
|
||
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477
The patch LGTM but I want Edgar to provide the feedback requested in the PR. Once he provides that feedback I'll r+ the patch. Thanks for the work Arthur.
Attachment #8378903 -
Flags: review?(josea.olivera)
Comment 7•11 years ago
|
||
comment on github.(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> Comment on attachment 8378903 [details]
> link to https://github.com/mozilla-b2g/gaia/pull/16477
>
> The patch LGTM but I want Edgar to provide the feedback requested in the PR.
> Once he provides that feedback I'll r+ the patch. Thanks for the work Arthur.
It looks good to me, please see comments in github, thank you. :)
Flags: needinfo?(echen)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477
Jose, could you check if the function is okay based on Edgar's comment? Thanks.
Attachment #8378903 -
Flags: review?(josea.olivera)
Comment 9•11 years ago
|
||
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477
Thanks for the reply Edgar. I left a comment on the PR guys, would you mind to take a look a it please? Thanks!
Attachment #8378903 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 10•11 years ago
|
||
It seems we have a timing issue. Edgar, it seems we are not allowed to call "setPreferredNetworkType" immediately after starting up. Is there any way get the proper timing to call to the method?
Flags: needinfo?(echen)
Assignee | ||
Comment 11•11 years ago
|
||
I checked the issue with Jessica and she said that the error was due to the radio state off. If we call to setPreferredNetworkType when the radio is off, gecko returns an error at first, but will do the setting when radio is turned on. In general the patch works as expected.
Flags: needinfo?(josea.olivera)
Comment 12•11 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #11)
> I checked the issue with Jessica and she said that the error was due to the
> radio state off. If we call to setPreferredNetworkType when the radio is
> off, gecko returns an error at first, but will do the setting when radio is
> turned on. In general the patch works as expected.
Yes, gecko side will cache the value and set it again when radio is on.
I have another idea. How about calling setPreferredNetworkType() after radioState becomes to "enabled"?
Thank you
Flags: needinfo?(echen)
Comment 13•11 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #12)
> (In reply to Arthur Chen [:arthurcc] from comment #11)
> > I checked the issue with Jessica and she said that the error was due to the
> > radio state off. If we call to setPreferredNetworkType when the radio is
> > off, gecko returns an error at first, but will do the setting when radio is
> > turned on. In general the patch works as expected.
> Yes, gecko side will cache the value and set it again when radio is on.
> I have another idea. How about calling setPreferredNetworkType() after
> radioState becomes to "enabled"?
> Thank you
To be more clear, for the radio off case, currently gecko will cache the last value comes from setPreferredNetworkType() and set it again when radio is on. But it will still dispatch error event to gaia.
I think we should have more discussion about this api (already file a bug, bug 986395).
To me, I will suggest gaia calls setPreferredNetworkType() after radioState becomes to "enabled" given that it more consist with other handling in gaia.
Thank you.
Comment 14•11 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #11)
> I checked the issue with Jessica and she said that the error was due to the
> radio state off. If we call to setPreferredNetworkType when the radio is
> off, gecko returns an error at first, but will do the setting when radio is
> turned on. In general the patch works as expected.
Left my opinion on the PR. IMHO to wait until radio is on is the right thing to do. Thanks guys.
Flags: needinfo?(josea.olivera)
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477
Jose, I've updated the patch on the basis of Edgar's suggestion and added related tests. Could you help review it? Thanks!
Attachment #8378903 -
Flags: review?(josea.olivera)
Comment 16•11 years ago
|
||
Comment on attachment 8378903 [details]
link to https://github.com/mozilla-b2g/gaia/pull/16477
LGTM. r=me
Test pass, Travis went green and It seems everything works correctly. I tested the patch on peak and the timing issue is gone. IMHO we should take care of the timing issue for the rest of the mozMobileConnection API usages in this file. Could you please file a follow-up bug for it please? Thanks!
Rebase the work, squash the commits and land once travis goes green.
Thanks for the patch and sorry for the changes about the timing issue.
Attachment #8378903 -
Flags: review?(josea.olivera) → review+
Assignee | ||
Comment 17•11 years ago
|
||
Edgar, does this behavior also apply to both setRoamingPreference and setVoicePrivacy?
Flags: needinfo?(echen)
Assignee | ||
Comment 18•11 years ago
|
||
Jose, thanks for reviewing.
master: befb78cce80144ba6c0783aefcf6b98439d0a8e9
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Comment 19•11 years ago
|
||
backout. We should save the default value back to the settings db when there is no available value there.
80af23f8c74d9d2e9388d8ed3c204040b5c528ec
Assignee | ||
Comment 20•11 years ago
|
||
Jose, I think I missed two cases in which we should save the values back to the settings db when starting up. The last commit fixed the issue and added unit tests. Could you help review it? Thanks.
Attachment #8378903 -
Attachment is obsolete: true
Attachment #8396182 -
Flags: review?(josea.olivera)
Comment 21•11 years ago
|
||
Comment on attachment 8396182 [details]
link to https://github.com/mozilla-b2g/gaia/pull/17562
(In reply to Arthur Chen [:arthurcc] from comment #20)
> Created attachment 8396182 [details]
> link to https://github.com/mozilla-b2g/gaia/pull/17562
>
> Jose, I think I missed two cases in which we should save the values back to
> the settings db when starting up. The last commit fixed the issue and added
> unit tests. Could you help review it? Thanks.
LGTM, you're right. As we have commented on IRC, this is the right thing to do IMHO. Thanks for this good catch Arthur. r=me
Attachment #8396182 -
Flags: review?(josea.olivera) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Thanks Jose!
master: 0c9701c77aefccf5bbf11db3843f3a34e43ceb94
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•11 years ago
|
||
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 #): bug 946589, gecko no longer maintains 'ril.radio.preferredNetworkType' and gaia should handle it.
[User impact] if declined: The user preferred network type is not kept after reboot.
[Testing completed]: Yes, manual testing. Unit tests added in master. Not able to add unit tests in v1.3t given the code structure.
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: N/A
Attachment #8396930 -
Flags: approval-gaia-v1.3?(fabrice)
Updated•11 years ago
|
Attachment #8396930 -
Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
Updated•11 years ago
|
blocking-b2g: 1.3T+ → 1.3+
Comment 24•11 years ago
|
||
v1.4: https://github.com/mozilla-b2g/gaia/commit/c719cb2b2a0b1cf8e2382313558eafca1a1769c5
Arthur, can you please post an updated PR against v1.3 instead of v1.3t? :)
status-b2g-v1.3:
--- → affected
Flags: needinfo?(arthur.chen)
Keywords: branch-patch-needed
Target Milestone: --- → 1.4 S4 (28mar)
Assignee | ||
Comment 25•11 years ago
|
||
Ryan, here you are: https://github.com/mozilla-b2g/gaia/pull/17678 Thanks!
Flags: needinfo?(arthur.chen)
Comment 26•11 years ago
|
||
Updated•11 years ago
|
Keywords: branch-patch-needed
Updated•11 years ago
|
Comment 27•11 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #17)
> Edgar, does this behavior also apply to both setRoamingPreference and
> setVoicePrivacy?
Yes, I think so, thank you.
Flags: needinfo?(echen)
You need to log in
before you can comment on or make changes to this bug.
Description
•