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

Status: RESOLVED FIXED

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

Description

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

It's a basic patch and it works well. Will try to test this on tablet later.

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 :)

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Because Tzh-Lin is going to fix airplane mode, just note here reminding me to rebase later.

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Looks good to me!

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 :)

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 9

[(inactive after 6/18) Alive Kuo [: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?

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

++

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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?

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Is outgoingCall a must-have setting?
If so I am fine to iterate all the mobile connections.

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 :)

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 :)

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Good job! r=me, thanks!

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all,  this patch just merged in Gaia/master : https://github.com/mozilla-b2g/gaia/commit/1e2997bb1625555919a52244330dac20ef6f4b99

Comment 19

[Carsten Book [:Tomcat]]

hi, sorry had to revert this changeset in https://github.com/mozilla-b2g/gaia/commit/780d8c8c61a30b8e108a4610445c2ed752da7420 for the suspicion that this broke the marionette tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=36592659&tree=B2g-Inbound

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: new patch on master

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)

Comment 21

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Comment 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Merged on Gaia/master : 13b00e279ebe5784772c763f730a277fd85f26e4

Thanks all ! :)

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

This commit make emulator always in airplane mode.  Please revert it.

Comment 24

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Backed out : https://github.com/mozilla-b2g/gaia/commit/a06077774a300065c8142e1d9fbd17c25e008ce9

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: new patch on master

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 !

Comment 27

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Merged at gaia/master : 7a9bef18aaa95be894ec6c5684b2a799e74c99bb

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

B2G emulator is again always booted in airplane mode.  Running git-bisect to find out whether it's caused by this bug.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

It's https://github.com/mozilla-b2g/gaia/commit/2a6aaa71e1dc39343dce115d005a6bd61a916591

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 31

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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 :)

Comment 32

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 33

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Comment 34

[bhavana bajaj [:bajaj]]

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

Comment 35

[Preeti Raghunath(:Preeti)]

Comment on attachment 8397588 [details] [review]
new patch on master

approving for 1.4

Comment 36

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Uplifted to v1.4

https://github.com/mozilla-b2g/gaia/pull/18307