Closed Bug 938015 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)

People

(Reporter: pzhang, Assigned: pzhang)

References

Details

(Whiteboard: [FT:System-Platform])

Attachments

(1 file, 1 obsolete file)

+++ 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?
Flags: needinfo?(timdream)
Priority: P1 → --
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?
Blocks: 908549
Flags: needinfo?(timdream) → needinfo?(htsai)
Whiteboard: [FT:System-Platform]
(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.
(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
(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?
(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
(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?
Assignee: nobody → pzhang
Thanks for Evelyn's response.
Flags: needinfo?(htsai)
(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.
(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.
(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?
(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.
(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?
Attached patch bug-938015-fix (obsolete) — Splinter Review
Attachment #8335187 - Flags: review?(khuey)
(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 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
Attachment #8335187 - Flags: review?(khuey) → review+
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!
Attachment #8335187 - Attachment is obsolete: true
Attachment #8357548 - Flags: review?(khuey)
Hi, Kyle, thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e60f0de566ec
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
You need to log in before you can comment on or make changes to this bug.