Closed Bug 1075727 Opened 10 years ago Closed 10 years ago

Don't report RDS as enabled when not supported

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(2 files, 1 obsolete file)

Right now, we report RDS as enabled if the user enables it. A number of things can prevent RDS from working though, and we shouldn't report RDS as enabled in those cases.
This allows the HAL side to report whether it was able to enable RDS.
Attachment #8498336 - Flags: review?(dhylands)
This ensures rdsEnabled is updated according to the return value of hal::EnableRDS().
Attachment #8498344 - Flags: review?(pzhang)
Comment on attachment 8498336 [details] [diff] [review]
Return success/failure in hal::EnableRDS

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

Looks good to me.
Attachment #8498336 - Flags: review?(dhylands) → review+
Comment on attachment 8498344 [details] [diff] [review]
Update rdsEnabled based on the return value of hal::EnableRDS

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

Should we handle this when setting group mask[1]?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp?from=FMRadioService.cpp#778
Oh, that logic is actually wrong. It needs to make sure RDS is already enabled before calling EnableRDS again. It's calling EnableRDS again to update the mask, but it shouldn't call EnableRDS if RDS isn't currently enabled.
Comment on attachment 8498344 [details] [diff] [review]
Update rdsEnabled based on the return value of hal::EnableRDS

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

(In reply to Michael Wu [:mwu] from comment #5)
> Oh, that logic is actually wrong. It needs to make sure RDS is already
> enabled before calling EnableRDS again. It's calling EnableRDS again to
> update the mask, but it shouldn't call EnableRDS if RDS isn't currently
> enabled.

Then let me clear the r? flag here.
Attachment #8498344 - Flags: review?(pzhang)
SetRDSGroupMask fixed.
Attachment #8498344 - Attachment is obsolete: true
Attachment #8504236 - Flags: review?(pzhang)
Comment on attachment 8504236 [details] [diff] [review]
Update rdsEnabled based on the return value of hal::EnableRDS, v2

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

r=me
Attachment #8504236 - Flags: review?(pzhang) → review+
https://hg.mozilla.org/mozilla-central/rev/5f053be160fb
https://hg.mozilla.org/mozilla-central/rev/28f02b49b5e6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.