Closed Bug 960861 Opened 7 years ago Closed 7 years ago

[Gaia][System] mozMobileConnections should not be requisites for airplane mode

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S6 (25apr)
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: arthurcc, Assigned: eragonj)

References

Details

Attachments

(2 files, 1 obsolete file)

Currently airplane mode was developed based on the existence of mozMobileConnections and which does not make sense for devices without it. We should see mozMobileConnections as one of the services to be toggled in airplane mode.
Assignee: nobody → ejchen
Attached file patch on master
It's a basic patch and it works well. Will try to test this on tablet later.
Comment on attachment 8363392 [details] [review]
patch on master

Hi Arthur ! I just updated the code and tested it on tablet & mobile.

They all work well as what we expected. There is one more thing that we need to discuss about. `Telephony` might be to general and would be used (perhaps) one day in the future. Any good suggestions ? 

Thanks :)
Attachment #8363392 - Flags: feedback?(arthur.chen)
Because Tzh-Lin is going to fix airplane mode, just note here reminding me to rebase later.
Depends on: 938080
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #3)
> Because Tzh-Lin is going to fix airplane mode, just note here reminding me
> to rebase later.

"Tzu-Lin" typo.
Comment on attachment 8363392 [details] [review]
patch on master

Looks good to me!
Attachment #8363392 - Flags: feedback?(arthur.chen) → feedback+
(In reply to Arthur Chen [:arthurcc] from comment #5)
> Comment on attachment 8363392 [details] [review]
> patch on master
> 
> Looks good to me!

Thanks Arthur ! I'll start to update related unit tests for Telephony ! Thanks :)
Comment on attachment 8363392 [details] [review]
patch on master

Arthur,

I just updated the code to decouple AirplaneMode and Telephony service, it works well and got green from Travis.

Please help me review the code when you have spare time. 

Big thanks :)
Attachment #8363392 - Flags: review?(arthur.chen)
Comment on attachment 8363392 [details] [review]
patch on master

Thank for the work, EJ. Looks good to me. Please address my comments in github. Flagging Alive for reviewing.
Attachment #8363392 - Flags: review?(arthur.chen) → review?(alive)
Comment on attachment 8363392 [details] [review]
patch on master

Could we move all mozMobileConnection functions into Radio? Is there any reason some of them in airplaneMode some and in radio?
Attachment #8363392 - Flags: review?(alive)
Good idea, 

Alive, how about this implementation ? 

https://github.com/EragonJ/gaia/commit/3b5f4f03215f9aa86aaa2dc15d03789a4550af2a#diff-f6a75107258346b2a111bdf8e50c423dL382

I modified Radio to be a bridge between AirplaneMode and mozMobileConnectiions so that there is no direct relationship with each other.
Flags: needinfo?(alive)
++
Flags: needinfo?(alive)
Alive,

I forgot that Gecko would rely on the defaultId on `outgoingCall` to decide which mozMobileConneciton's radioState would be changed. (You can check bug 963467)

For example, if user change outgoingCall to card2, only that connection would be changed (can catch radioStateChange event)

In this way, we still have to iterate over mozMobileConnections to register event but only one connection would be triggered in this case ! In this way, we won't mess APM up with setting APM enabled twice !

what do you think :D?
Flags: needinfo?(alive)
Is outgoingCall a must-have setting?
If so I am fine to iterate all the mobile connections.
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #13)
> Is outgoingCall a must-have setting?

Yes ! 

> If so I am fine to iterate all the mobile connections.

OK !
Comment on attachment 8363392 [details] [review]
patch on master

Hi all, I didn't forget this patch !! 

Because the end of 1.4 is coming and I have just rebased the patch to make it pass Travis, I think this patch is ready to be reviewed. 

Please help me review this change when you have time ! Thanks Alive & Arthur :)
Attachment #8363392 - Flags: review?(alive)
Comment on attachment 8363392 [details] [review]
patch on master

Forgot to put Arthur into review queue XD

BTW, this feature would be put into v1.5 ! Thanks :)
Attachment #8363392 - Flags: review?(arthur.chen)
Comment on attachment 8363392 [details] [review]
patch on master

Good job! r=me, thanks!
Attachment #8363392 - Flags: review?(arthur.chen) → review+
Attachment #8363392 - Flags: review?(alive) → review+
Thanks all,  this patch just merged in Gaia/master : https://github.com/mozilla-b2g/gaia/commit/1e2997bb1625555919a52244330dac20ef6f4b99
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file new patch on master (obsolete) —
This is a new patch on master without any change, because the old one got backed out from master. (This one was just cherry-picked from the original commit)
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #20)
> Created attachment 8396207 [details] [review]
> new patch on master

This patch passed try server : https://hg.mozilla.org/try/rev/8778b98dd4ae
and also travis : https://travis-ci.org/mozilla-b2g/gaia/builds/21416285

In this way, I would merge this into master again and let's see whether there is any error happened or not.
Merged on Gaia/master : 13b00e279ebe5784772c763f730a277fd85f26e4

Thanks all ! :)
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
This commit make emulator always in airplane mode.  Please revert it.
Flags: needinfo?(timdream)
Flags: needinfo?(timdream) → needinfo?(ejchen)
Backed out : https://github.com/mozilla-b2g/gaia/commit/a06077774a300065c8142e1d9fbd17c25e008ce9
Status: RESOLVED → REOPENED
Flags: needinfo?(ejchen)
Resolution: FIXED → ---
Fixed a problem in airplaneMode that we didn't setRadioEnabled at the first time. 

Try Server report (Green) : https://tbpl.mozilla.org/?tree=Try&rev=625e2e4009f5
Travis report (Green) : https://travis-ci.org/mozilla-b2g/gaia/builds/21499502

And I tested this functionalities on Fugu/Inari/Flatfish and they all work well, hope this time it works well !
Merged at gaia/master : 7a9bef18aaa95be894ec6c5684b2a799e74c99bb
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
B2G emulator is again always booted in airplane mode.  Running git-bisect to find out whether it's caused by this bug.
Comment on attachment 8397588 [details] [review]
new patch on master

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 #): Not sure about originating bug, but nomming this because it fixes bug 991467
[User impact] if declined: Airplane mode incorrectly enabled (Bug 991467)
[Testing completed]: Yes, see comments of bug 991467
[Risk to taking this patch] (and alternatives if risky): None
[String changes made]: None

Note that there will be a conflict on the v1.3 merge. Just delete the check for mozSettings existence, that came in as part of an OOP unit test fix that is irrelevant to v1.3.
Attachment #8397588 - Flags: approval-gaia-v1.4?(release-mgmt)
Attachment #8397588 - Flags: approval-gaia-v1.3?(release-mgmt)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #30)

Hi Kyle,

If this did fix the problem in Bug 991467 and got approval flag, please help to uplift them to the needed branch.

Thanks :)
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #31)
> If this did fix the problem in Bug 991467 and got approval flag, please help
> to uplift them to the needed branch.

Yeah, no prob. I have a feeling that may just magically happen after approval (always seems to for me, but I'm usually not doing it on other people's already resolved bugs :) ), if not I can help out.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #32)
> (In reply to EJ Chen [:eragonj][:小龍哥] from comment #31)
> > If this did fix the problem in Bug 991467 and got approval flag, please help
> > to uplift them to the needed branch.
> 
> Yeah, no prob. I have a feeling that may just magically happen after
> approval

okkk !! Kyle ++
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #30)
> Comment on attachment 8397588 [details] [review]
> new patch on master
> 
> 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 #): Not sure about originating bug,
> but nomming this because it fixes bug 991467
> [User impact] if declined: Airplane mode incorrectly enabled (Bug 991467)
> [Testing completed]: Yes, see comments of bug 991467
> [Risk to taking this patch] (and alternatives if risky): None
> [String changes made]: None
> 
> Note that there will be a conflict on the v1.3 merge. Just delete the check
> for mozSettings existence, that came in as part of an OOP unit test fix that
> is irrelevant to v1.3.

Given https://bugzilla.mozilla.org/show_bug.cgi?id=991467 is pushed to 1.4, does not look like we need this change on 1.3 so clearing the approval request here.
Attachment #8397588 - Flags: approval-gaia-v1.3?(release-mgmt)
Comment on attachment 8397588 [details] [review]
new patch on master

approving for 1.4
Attachment #8397588 - Flags: approval-gaia-v1.4?(release-mgmt) → approval-gaia-v1.4+
Target Milestone: --- → 1.4 S6 (25apr)
You need to log in before you can comment on or make changes to this bug.