Closed
Bug 997064
Opened 10 years ago
Closed 10 years ago
Do not call navigator.mozFMRadio from system app.
Categories
(Firefox OS Graveyard :: Gaia::FMRadio, defect)
Firefox OS Graveyard
Gaia::FMRadio
Tracking
(blocking-b2g:1.3T+, 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
|
praghunath
:
approval-mozilla-b2g30+
|
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.
Comment 1•10 years ago
|
||
(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?
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Comment 4•10 years ago
|
||
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?
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
(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
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Remove fm radio disable from system app, and change setting name in fm radio app.
Attachment #8408785 -
Flags: review?(arthur.chen)
Assignee | ||
Updated•10 years ago
|
Attachment #8408785 -
Attachment description: gaia patch → pr 18444
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8408785 -
Flags: review?(alive) → review+
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
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)
Comment 19•10 years ago
|
||
Pin, We need land this patch on v1.3T,not just master.
Flags: needinfo?(pzhang)
Comment 20•10 years ago
|
||
Thomas, we need land this patch on v1.3T.
blocking-b2g: --- → 1.3T?
Flags: needinfo?(ttsai)
Assignee | ||
Comment 21•10 years ago
|
||
(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.
Comment 23•10 years ago
|
||
(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.
Comment 24•10 years ago
|
||
hi Pin, seem like you are working on it, do you mind taking? Thanks
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pzhang
Flags: needinfo?(pzhang)
Updated•10 years ago
|
Whiteboard: [sprd296957] [partner-blocker]
Comment 25•10 years ago
|
||
Please review+ and land it.
Assignee | ||
Comment 26•10 years ago
|
||
Hi :baku, can you take a look?
Attachment #8410744 -
Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8410745 -
Flags: review?(arthur.chen)
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
(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 ...
Assignee | ||
Comment 30•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8410745 -
Flags: review?(arthur.chen) → review+
Updated•10 years ago
|
Attachment #8410744 -
Flags: review?(amarchesini) → review+
Comment 31•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 32•10 years ago
|
||
Addressed nits in comment 31, will provide all-in-one patch after r+.
Attachment #8411086 -
Flags: review?(amarchesini)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8411087 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(amarchesini)
Updated•10 years ago
|
Attachment #8411086 -
Flags: review?(amarchesini) → review+
Updated•10 years ago
|
Attachment #8411087 -
Flags: review?(amarchesini) → review+
Updated•10 years ago
|
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 34•10 years ago
|
||
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.
Assignee | ||
Comment 35•10 years ago
|
||
Assignee | ||
Comment 36•10 years ago
|
||
Assignee | ||
Comment 37•10 years ago
|
||
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
Assignee | ||
Comment 38•10 years ago
|
||
@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)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 39•10 years ago
|
||
(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.
Comment 40•10 years ago
|
||
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?
Keywords: checkin-needed
Comment 41•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/300930f28f64
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → ?
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → ?
status-firefox31:
--- → fixed
Flags: needinfo?(pzhang)
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Comment 42•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(fabrice)
Comment 43•10 years ago
|
||
Fabrice and Ping, can you help to land this patch on v1.3t? I have no gecko permission.
Updated•10 years ago
|
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Flags: needinfo?(jesse.ji)
Assignee | ||
Comment 44•10 years ago
|
||
(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.
Comment 45•10 years ago
|
||
1.3t: https://github.com/mozilla-b2g/gaia/commit/8895b180ed636069473703d0e7b73086989601ce https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/88fd83a288e1
Flags: needinfo?(fabrice)
Comment 46•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(jcheng)
Assignee | ||
Comment 48•10 years ago
|
||
(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)
Comment 49•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(ttsai)
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8415083 -
Flags: review?(arthur.chen)
Attachment #8415083 -
Flags: approval-gaia-v1.4?(fabrice)
Assignee | ||
Comment 51•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8415083 -
Flags: approval-gaia-v1.4?(fabrice) → approval-gaia-v1.4?(release-mgmt)
Updated•10 years ago
|
Attachment #8415083 -
Flags: review?(arthur.chen) → review+
Comment 52•10 years ago
|
||
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)
Assignee | ||
Comment 53•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(styang)
Updated•10 years ago
|
Attachment #8413078 -
Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Updated•10 years ago
|
Attachment #8415083 -
Flags: approval-gaia-v1.4?(release-mgmt)
Comment 54•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b0f287889ae9 v1.4: https://github.com/mozilla-b2g/gaia/commit/f19735d288d9bf1c6ee0c0ecc7941421365037c7
Assignee | ||
Comment 55•10 years ago
|
||
(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.
Description
•