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)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(2 files, 1 obsolete file)

Return success/failure in hal::EnableRDS
3.37 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
Update rdsEnabled based on the return value of hal::EnableRDS, v2
3.06 KB, patch
pzhang
: review+
Details | Diff | Splinter Review
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.

Attachment

General

Creator:
Created:
Updated:
Size: