Closed Bug 997064 Opened 9 years ago Closed 8 years ago

Do not call navigator.mozFMRadio from system app.

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:1.3T+, firefox29 wontfix, firefox30 wontfix, firefox31 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: pzhang, Assigned: pzhang)

References

Details

(Whiteboard: [sprd296957] [partner-blocker])

Attachments

(5 files, 8 obsolete files)

46 bytes, text/x-github-pull-request
arthurcc
: review+
alive
: review+
Details | Review
52 bytes, text/plain
arthurcc
: review+
Details
16.37 KB, patch
Details | Diff | Splinter Review
11.32 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
+++ This bug was initially created as a clone of Bug #992478 +++

Currently, we need to disable FM radio from system app when airplane mode is enabled, the way we disable FM radio is calling |navigator.mozFMRadio.disable()|, but this causes lots of trouble when implementing audio strategy for WebFM, because we need to handle visibility status change of FM Radio app. The system app is not actual a FM app, we need to ignore the visibility status change of System app, more detailed discussion is here: https://bugzilla.mozilla.org/show_bug.cgi?id=992478#c28

My suggestion is do not call mozFMRadio from system app at all, use ContentEvent to talk to Gecko instead.
(In reply to Pin Zhang [:pzhang] from comment #0)
> +++ This bug was initially created as a clone of Bug #992478 +++
> 
> Currently, we need to disable FM radio from system app when airplane mode is
> enabled, the way we disable FM radio is calling
> |navigator.mozFMRadio.disable()|, but this causes lots of trouble when
> implementing audio strategy for WebFM, because we need to handle visibility
> status change of FM Radio app. The system app is not actual a FM app, we
> need to ignore the visibility status change of System app, more detailed
> discussion is here: https://bugzilla.mozilla.org/show_bug.cgi?id=992478#c28
> 
> My suggestion is do not call mozFMRadio from system app at all, use
> ContentEvent to talk to Gecko instead.

Don't we already know in gecko when we switch to airplane mode? If so, why do we need to send an event from gaia?
(In reply to Fabrice Desré [:fabrice] from comment #1)
> (In reply to Pin Zhang [:pzhang] from comment #0)
> > +++ This bug was initially created as a clone of Bug #992478 +++
> > 
> > Currently, we need to disable FM radio from system app when airplane mode is
> > enabled, the way we disable FM radio is calling
> > |navigator.mozFMRadio.disable()|, but this causes lots of trouble when
> > implementing audio strategy for WebFM, because we need to handle visibility
> > status change of FM Radio app. The system app is not actual a FM app, we
> > need to ignore the visibility status change of System app, more detailed
> > discussion is here: https://bugzilla.mozilla.org/show_bug.cgi?id=992478#c28
> > 
> > My suggestion is do not call mozFMRadio from system app at all, use
> > ContentEvent to talk to Gecko instead.
> 
> Don't we already know in gecko when we switch to airplane mode? If so, why
> do we need to send an event from gaia?

Hi Fabrice, we did have airplane mode listener in Gecko before, but we removed it in bug 938015, only keep airplane mode related logics in system app.
Could anyone quickly find a way to resolve this? If not, Pin and I will add airplane mode listener in Gecko again. We can't wait any longer.
Pin, doing that in gaia looks wrong to me (and it seems it looked wrong to you to in bug 938015).

What about using a gecko-side observer notification fired by the RIL when we disable/enable radio and have the FMRadioService listen to it?
(In reply to Fabrice Desré [:fabrice] from comment #4)
> Pin, doing that in gaia looks wrong to me (and it seems it looked wrong to
> you to in bug 938015).
> 
> What about using a gecko-side observer notification fired by the RIL when we
> disable/enable radio and have the FMRadioService listen to it?

+1, sounds good to me.

BTW, bug 938015 didn't land in 1.3T, which means we still have airplane mode setting listener in 1.3T.
One concern of (In reply to Fabrice Desré [:fabrice] from comment #4)
> What about using a gecko-side observer notification fired by the RIL when we
> disable/enable radio and have the FMRadioService listen to it?

Hi Fabrice and Pin,

One concern of this approach is that "Not every device will have RIL related services. ex: tablet". 
Therefore this approach will bring trouble on tablet project.

The second concern is that "What wireless functions should be taken care by airplane mode? ex: WiFi, FM, RIL, WiMax,". I think this logic should be maintained by system app not Gecko.

Just my 2 cents.
(In reply to Marco Chen [:mchen] from comment #6)
> One concern of (In reply to Fabrice Desré [:fabrice] from comment #4)
> > What about using a gecko-side observer notification fired by the RIL when we
> > disable/enable radio and have the FMRadioService listen to it?
> 
> Hi Fabrice and Pin,
> 
> One concern of this approach is that "Not every device will have RIL related
> services. ex: tablet". 
> Therefore this approach will bring trouble on tablet project.
> 

I checked our code, we now use setting "airplaneMode.enabled" instead, so we can listen the new setting instead of "ril.radio.disabled" in Gecko.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/airplane_mode_helper.js#L28
What is done in this patch include:
  - backout bug 938015
  - fixed compile error
  - use "airplaneMode.enabled" instead of "ril.radio.disabled"

Hi Marco, could you take a look?
Attachment #8408780 - Flags: review?(mchen)
Attached patch FYI, backout commit (obsolete) — Splinter Review
Attached file pr 18444
Remove fm radio disable from system app, and change setting name in fm radio app.
Attachment #8408785 - Flags: review?(arthur.chen)
Attachment #8408785 - Attachment description: gaia patch → pr 18444
Comment on attachment 8408785 [details] [review]
pr 18444

The system part looks good to me. However, in FM app we should use AirplaneModeHelper defined in the shared folder instead of accessing 'airplaneMode.enabled' directly.
Attachment #8408785 - Flags: review?(arthur.chen)
Comment on attachment 8408785 [details] [review]
pr 18444

(In reply to Arthur Chen [:arthurcc] from comment #12)
> Comment on attachment 8408785 [details] [review]
> pr 18444
> 
> The system part looks good to me. However, in FM app we should use
> AirplaneModeHelper defined in the shared folder instead of accessing
> 'airplaneMode.enabled' directly.

PR is updated, please take a look again, thanks.
Attachment #8408785 - Flags: review?(arthur.chen)
No longer depends on: 992478
Comment on attachment 8408785 [details] [review]
pr 18444

r=me for the part of fm app. Flagging alive for the part of system app.
Attachment #8408785 - Flags: review?(arthur.chen)
Attachment #8408785 - Flags: review?(alive)
Attachment #8408785 - Flags: review+
Comment on attachment 8408780 [details] [diff] [review]
0001-Bug-997064-Do-not-call-navigator.mozFMRadio-from-sys.patch

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

It looks good to me except the naming issues.
Could you fix these issues then ask the review from dom peer again?

Thanks.

::: dom/fmradio/FMRadioService.cpp
@@ +47,5 @@
>  
>  FMRadioService::FMRadioService()
>    : mPendingFrequencyInKHz(0)
>    , mState(Disabled)
>    , mHasReadRilSetting(false)

To change the name of mHasReadRilSetting corresponding to new airplan mode setting.

@@ +442,5 @@
>          NS_LITERAL_STRING("Can't create settings lock")), Disabled);
>        return;
>      }
>  
>      nsRefPtr<ReadRilSettingTask> callback =

To change the name of ReadRilSettingTask corresponding to airplan mode setting.

@@ +661,5 @@
> +    return NS_OK;
> +  }
> +
> +  // The string that we're interested in will be a JSON string looks like:
> +  //  {"key":"ril.radio.disabled","value":true}

Change to airplan mode setting?

@@ +701,5 @@
> +    if (mAirplaneModeEnabled) {
> +      Disable(nullptr);
> +    }
> +
> +    return NS_OK;

It seems that this is a redundant return here.

::: dom/fmradio/test/marionette/test_bug876597.js
@@ +16,5 @@
> +  ok(FMRadio);
> +  is(FMRadio.enabled, false);
> +  ok(mozSettings);
> +
> +  checkRilSettings();

Change ril related naming to airplan mode.
Attachment #8408780 - Flags: review?(mchen)
Attachment #8408785 - Flags: review?(alive) → review+
Attached patch Patch V2 (obsolete) — Splinter Review
Addressed naming issues mentioned in comment 15.

Hi, @Kyle, could you also take a look?
Attachment #8408780 - Attachment is obsolete: true
Attachment #8409561 - Flags: review?(khuey)
(BTW I believe Kyle is OOO for three weeks.)
Flags: needinfo?
Comment on attachment 8409561 [details] [diff] [review]
Patch V2

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

Hi :baku, could you take a look?
Attachment #8409561 - Flags: review?(khuey) → review?(amarchesini)
Pin, We need land this patch on v1.3T,not just master.
Flags: needinfo?(pzhang)
Blocks: 992478
Thomas, we need land this patch on v1.3T.
blocking-b2g: --- → 1.3T?
Flags: needinfo?(ttsai)
(In reply to Jesse from comment #19)
> Pin, We need land this patch on v1.3T,not just master.

There are lots of differences, we need new patch for v1.3T.
triage: blocks a 1.3T+, 1.3T+
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?
(In reply to Pin Zhang [:pzhang] from comment #21)
> (In reply to Jesse from comment #19)
> > Pin, We need land this patch on v1.3T,not just master.
> 
> There are lots of differences, we need new patch for v1.3T.

Make a new one if needed.
hi Pin, seem like you are working on it, do you mind taking? Thanks
Assignee: nobody → pzhang
Flags: needinfo?(pzhang)
Whiteboard: [sprd296957] [partner-blocker]
Please review+ and land it.
Attached patch Gecko patch for v1.3T (obsolete) — Splinter Review
Hi :baku, can you take a look?
Attachment #8410744 - Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8410745 - Flags: review?(arthur.chen)
Comment on attachment 8410745 [details]
Gaia patch for v1.3T, PR 18555

Pin, please check my comment in github.

Tim, could you help review the makefile part?
Attachment #8410745 - Flags: review?(arthur.chen) → review?(timdream)
(In reply to Arthur Chen [:arthurcc] from comment #28)
> Comment on attachment 8410745 [details]
> Gaia patch for v1.3T, PR 18555
> 
> Pin, please check my comment in github.
> 
> Tim, could you help review the makefile part?

oops, the change to makefile is not supposed to be part of the patch, it's only a hack for Tarako ...
Comment on attachment 8410745 [details]
Gaia patch for v1.3T, PR 18555

Hi Arthur, patch is updated, please take a look, thanks.
Attachment #8410745 - Flags: review?(timdream) → review?(arthur.chen)
Attachment #8410745 - Flags: review?(arthur.chen) → review+
Attachment #8410744 - Flags: review?(amarchesini) → review+
Comment on attachment 8409561 [details] [diff] [review]
Patch V2

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

::: dom/fmradio/FMRadioService.cpp
@@ +91,5 @@
>        break;
>    }
>  
>    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
>  

if (obs) { ...

@@ +666,5 @@
> +  AutoSafeJSContext cx;
> +  const nsDependentString dataStr(aData);
> +  JS::Rooted<JS::Value> val(cx);
> +  if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||
> +      !val.isObject()) {

I would add: NS_WARNING("... something

@@ +673,5 @@
> +
> +  JS::Rooted<JSObject*> obj(cx, &val.toObject());
> +  JS::Rooted<JS::Value> key(cx);
> +  if (!JS_GetProperty(cx, obj, "key", &key) ||
> +      !key.isString()) {

a warning here as well.

@@ +684,5 @@
> +    return NS_OK;
> +  }
> +
> +  JS::Rooted<JS::Value> value(cx);
> +  if (!JS_GetProperty(cx, obj, "value", &value)) {

a warning

@@ +688,5 @@
> +  if (!JS_GetProperty(cx, obj, "value", &value)) {
> +    return NS_OK;
> +  }
> +
> +  if (keyStr.EqualsLiteral(SETTING_KEY_AIRPLANEMODE_ENABLED)) {

Move the JS_GETProperty(cx, obj, "value".. here. You don't need the value outside this 'if' condition.
Attachment #8409561 - Flags: review?(amarchesini) → review+
Flags: needinfo?(amarchesini)
Addressed nits in comment 31, will provide all-in-one patch after r+.
Attachment #8411086 - Flags: review?(amarchesini)
Attached patch incremental patch for 1.3T.diff (obsolete) — Splinter Review
Attachment #8411087 - Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8411086 - Flags: review?(amarchesini) → review+
Attachment #8411087 - Flags: review?(amarchesini) → review+
Flags: needinfo?(amarchesini)
TBPL: https://tbpl.mozilla.org/?tree=Try&rev=332b1437d49e

2 fails on audio test and 1 fail on MacOS' installation test, I don't think they are caused by patch for this bug.
Attachment #8408781 - Attachment is obsolete: true
Attachment #8408782 - Attachment is obsolete: true
Attachment #8409561 - Attachment is obsolete: true
Attachment #8410744 - Attachment is obsolete: true
Attachment #8411086 - Attachment is obsolete: true
Attachment #8411087 - Attachment is obsolete: true
Attachment #8413080 - Attachment is obsolete: true
@Marco, do you know how to submit patch for v1.3T to TryServer?

I tried to clone http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/, and push to TryServer, but failed to build: 
  https://tbpl.mozilla.org/?tree=Try&rev=67c562a5d160
Flags: needinfo?(mchen)
Keywords: checkin-needed
Status: NEW → ASSIGNED
(In reply to Pin Zhang [:pzhang] from comment #38)
> I tried to clone http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/, and
> push to TryServer, but failed to build: 
>   https://tbpl.mozilla.org/?tree=Try&rev=67c562a5d160

Completely normal. Note that B2G Emulator ICS and Tarako device image builds are all that run on the 1.3t branch on TBPL because everything else is busted :). Just one of the caveats of pushing older branches to Try.
https://hg.mozilla.org/mozilla-central/rev/300930f28f64
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-b2g-v1.4: --- → ?
Flags: needinfo?(pzhang)
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> https://hg.mozilla.org/integration/b2g-inbound/rev/300930f28f64
> 
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> 265e1ac4ee71ad6190335c974bfce33f783edfce
> 
> Does this need to land on v1.4 too?

Should this be landed on 1.4?
Flags: needinfo?(pzhang) → needinfo?(jcheng)
Flags: needinfo?(fabrice)
Fabrice and Ping, can you help to land this patch on v1.3t? I have no gecko permission.
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Flags: needinfo?(jesse.ji)
(In reply to James Zhang from comment #43)
> Fabrice and Ping, can you help to land this patch on v1.3t? I have no gecko
> permission.

Hi James, I only have access level 1, sorry.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #39)
> Completely normal. Note that B2G Emulator ICS and Tarako device image builds
> are all that run on the 1.3t branch on TBPL because everything else is
> busted :). Just one of the caveats of pushing older branches to Try.

I don't know the answer and learned from here too. Thanks, Ryan.
Flags: needinfo?(mchen)
Thanks to Fabrice.

--
Keven
Flags: needinfo?(kkuo)
Flags: needinfo?(jcheng)
(In reply to Fabrice Desré [:fabrice] from comment #45)
> 1.3t:
> https://github.com/mozilla-b2g/gaia/commit/
> 8895b180ed636069473703d0e7b73086989601ce
> 
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/88fd83a288e1

Hi. Fabrice, does this need to be fixed on 1.4?
Flags: needinfo?(fabrice)
Flags: needinfo?(jesse.ji)
(In reply to Pin Zhang [:pzhang] from comment #48)
> (In reply to Fabrice Desré [:fabrice] from comment #45)
> > 1.3t:
> > https://github.com/mozilla-b2g/gaia/commit/
> > 8895b180ed636069473703d0e7b73086989601ce
> > 
> > https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/88fd83a288e1
> 
> Hi. Fabrice, does this need to be fixed on 1.4?

No idea. If you feel it needs to be, you have to request uplift approval on the patches.
Flags: needinfo?(fabrice)
Flags: needinfo?(ttsai)
Attachment #8415083 - Flags: review?(arthur.chen)
Attachment #8415083 - Flags: approval-gaia-v1.4?(fabrice)
Comment on attachment 8413078 [details] [diff] [review]
Gecko patch for check-in, master branch

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

The patch for master branch is also works for v1.4
Attachment #8413078 - Flags: approval-mozilla-b2g30?
Attachment #8415083 - Flags: approval-gaia-v1.4?(fabrice) → approval-gaia-v1.4?(release-mgmt)
Attachment #8415083 - Flags: review?(arthur.chen) → review+
pzhang, can you help answer why this needs to land on 1.4 ? Is this a 1.4 regression ? how is the user impacted ?
Flags: needinfo?(pzhang)
(In reply to bhavana bajaj [:bajaj] from comment #52)
> pzhang, can you help answer why this needs to land on 1.4 ? Is this a 1.4
> regression ? how is the user impacted ?

The patches move disable-fm-when-airplane-mode-enabled logic from Gaia back to Gecko, it's kinda bug 938015 backout, so user is not impacted at all. About why this needs to land on 1.4, i just think it's fixed on master and 1.3T, so it should be also be fixed on 1.4, :)
Flags: needinfo?(pzhang)
Flags: needinfo?(styang)
Attachment #8413078 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8415083 - Flags: approval-gaia-v1.4?(release-mgmt)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #54)
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b0f287889ae9
> 
> v1.4:
> https://github.com/mozilla-b2g/gaia/commit/
> f19735d288d9bf1c6ee0c0ecc7941421365037c7

Thanks, Rayn!
You need to log in before you can comment on or make changes to this bug.