938015 - FM is turned off when enabling on airplane mode both in Gecko and Gaia.

Status: RESOLVED FIXED

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

Description

[Pin Zhang [:pzhang] (inactive)]

+++ This bug was initially created as a clone of Bug #814918 +++

Here is what we did in Gaia (Bug #814918):
  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/airplane_mode.js#L174
and this is what we did in Gecko (Bug #870676):
  http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#701

we should only keep one of them.

:Tim, what do you think?

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am not sure "ril.radio.disabled" is going away or not. If so it's probably better to do everything in Gaia.

If that is not going away, it would make more sense to remove the Gaia part for now.

BTW, I don't see any code that monitor the change of the settings value in Gecko, will FM radio actually be turned off (by Gecko along) when the user toggle the setting?

Comment 2

[Evelyn Hung]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #1)
> I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> better to do everything in Gaia.
> 
we don't trigger airplane mode via mozSettings anymore, but use MobileConnection.setRadioEnabled() instead. (see bug 928851)
So it's necessary to remove the logic of listening "ril.radio.disabled" in Gecko.

Comment 3

[Pin Zhang [:pzhang] (inactive)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #1)
> I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> better to do everything in Gaia.
> 
> If that is not going away, it would make more sense to remove the Gaia part
> for now.
> 
> BTW, I don't see any code that monitor the change of the settings value in
> Gecko, will FM radio actually be turned off (by Gecko along) when the user
> toggle the setting?

The code to monitor settings' change is:
  http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#96

Comment 4

[Pin Zhang [:pzhang] (inactive)]

(In reply to Evelyn Hung [:evelyn] from comment #2)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #1)
> > I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> > better to do everything in Gaia.
> > 
> we don't trigger airplane mode via mozSettings anymore, but use
> MobileConnection.setRadioEnabled() instead. (see bug 928851)
> So it's necessary to remove the logic of listening "ril.radio.disabled" in
> Gecko.

I see, I will remove it from Gecko.

I searched 'ril.radio.disabled' in Gaia, there are several apps that monitored this setting, like costcontrol and fm (bug 876597), how about them?

Comment 5

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #3)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #1)
> > I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> > better to do everything in Gaia.
> > 
> > If that is not going away, it would make more sense to remove the Gaia part
> > for now.
> > 
> > BTW, I don't see any code that monitor the change of the settings value in
> > Gecko, will FM radio actually be turned off (by Gecko along) when the user
> > toggle the setting?
> 
> The code to monitor settings' change is:
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.
> cpp#96

And this is the test:
 http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/test_bug876597.js

Comment 6

[Pin Zhang [:pzhang] (inactive)]

(In reply to Evelyn Hung [:evelyn] from comment #2)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #1)
> > I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> > better to do everything in Gaia.
> > 
> we don't trigger airplane mode via mozSettings anymore, but use
> MobileConnection.setRadioEnabled() instead. (see bug 928851)
> So it's necessary to remove the logic of listening "ril.radio.disabled" in
> Gecko.

Hi Evelyn, how can I know the airplane node is disabled or not now?

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks for Evelyn's response.

Comment 8

[Marco Chen [:mchen]]

(In reply to Pin Zhang [:pzhang] from comment #6)
> Hi Evelyn, how can I know the airplane node is disabled or not now?

I think FMRadio in Gecko didn't need to handle airplan mode and that should be implemented from gaia side to control normal FMRadio Web APIs.

Comment 9

[Marco Chen [:mchen]]

(In reply to Evelyn Hung [:evelyn] from comment #2)
> we don't trigger airplane mode via mozSettings anymore, but use
> MobileConnection.setRadioEnabled() instead. (see bug 928851)
> So it's necessary to remove the logic of listening "ril.radio.disabled" in
> Gecko.

Hi Evelyn,

Please help to make sure that the mechanism for airplan mode can be workable for devices without RIL.
Thanks.

Comment 10

[Pin Zhang [:pzhang] (inactive)]

(In reply to Marco Chen [:mchen] from comment #8)
> (In reply to Pin Zhang [:pzhang] from comment #6)
> > Hi Evelyn, how can I know the airplane node is disabled or not now?
> 
> I think FMRadio in Gecko didn't need to handle airplan mode and that should
> be implemented from gaia side to control normal FMRadio Web APIs.

I'm kinda confused, if user enabled the airplane mode in settings app, and then opened an installed-not-built-in FM radio app, how the Gaia handle the scenario if Gecko don't handle it?

Comment 11

[Marco Chen [:mchen]]

(In reply to Pin Zhang [:pzhang] from comment #10)
> I'm kinda confused, if user enabled the airplane mode in settings app, and
> then opened an installed-not-built-in FM radio app, how the Gaia handle the
> scenario if Gecko don't handle it?

From my point of view, Gaia just follow the description of Web API to control the FMRadio instance. It should not take care such kind of things.

The Gecko should handle/protect or allow stuff of multiple clients using single HW source, and currently it seems we allowed multiple clients to control single HW simultaneously.

Comment 12

[Pin Zhang [:pzhang] (inactive)]

(In reply to Marco Chen [:mchen] from comment #11)
> (In reply to Pin Zhang [:pzhang] from comment #10)
> > I'm kinda confused, if user enabled the airplane mode in settings app, and
> > then opened an installed-not-built-in FM radio app, how the Gaia handle the
> > scenario if Gecko don't handle it?
> 
> From my point of view, Gaia just follow the description of Web API to
> control the FMRadio instance. It should not take care such kind of things.
> 
> The Gecko should handle/protect or allow stuff of multiple clients using
> single HW source, and currently it seems we allowed multiple clients to
> control single HW simultaneously.

Even if we only allow one client one time, I think we still have the problem. What i still don't understand is, if we support airplane mode, and don't allow FM radio to be enabled by design, then how can we block the request of enabling the FM radio from a third party FM radio APP in Gaia?

Comment 13

[Pin Zhang [:pzhang] (inactive)]

Attachment: bug-938015-fix

Comment 14

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #13)
> Created attachment 8335187 [details] [diff] [review]
> bug-938015-fix

According to the patch for bug 928851, we still use the setting key 'ril.airplane.disabled' to remember the status of airplane mode, so I kept the airplane mode checking codes in the FM radio enabling logic.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8335187 [details] [diff] [review]
bug-938015-fix

Review of attachment 8335187 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here, I've been swamped since Thanksgiving.

r=me

Comment 16

[Pin Zhang [:pzhang] (inactive)]

Attachment: 0001-Bug-938015-do-not-turn-off-FM-radio-when-enabling-on.patch

Hi Kyle, the old patch can't be applied on the latest code, I rebased it and removed useless nullptr checking in FMRadioService::Disable, please help review it again, thanks!

Comment 17

[Pin Zhang [:pzhang] (inactive)]

Hi, Kyle, thanks!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e60f0de566ec

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e60f0de566ec