Closed
Bug 793809
Opened 9 years ago
Closed 9 years ago
Take FM radio initialization off main thread
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: mwu, Assigned: mwu)
References
Details
Attachments
(1 file, 1 obsolete file)
6.51 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Attachment #664161 -
Flags: review?(justin.lebar+bug)
Comment 1•9 years ago
|
||
>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp > class RadioUpdate : public nsRunnable { > hal::FMRadioOperation mOp; >+ hal::FMRadioOperationStatus mStatus; > public: >- RadioUpdate(hal::FMRadioOperation op) >+ RadioUpdate(hal::FMRadioOperation op, >+ hal::FMRadioOperationStatus status = hal::FM_RADIO_OPERATION_STATUS_SUCCESS) I almost think this would be cleaner if we didn't have the default param here, but up to you. > : mOp(op) >+ , mStatus(status) > {} > > NS_IMETHOD Run() { > hal::FMRadioOperationInformation info; > info.operation() = mOp; >- info.status() = hal::FM_RADIO_OPERATION_STATUS_SUCCESS; >+ info.status() = mStatus; > info.frequency() = GetFMRadioFrequency(); > hal::NotifyFMRadioStatus(info); > return NS_OK; > } > }; > >+static void >+initTavaruaRadio(hal::FMRadioSettings &aInfo) > { >+ mozilla::ScopedClose fd(sRadioFD); > char command[64]; >- snprintf(command, sizeof(command), "/system/bin/fm_qsoc_patches %d 0", cap.version); >- rc = system(command); >+ snprintf(command, sizeof(command), "/system/bin/fm_qsoc_patches %d 0", sTavaruaVersion); >+ int rc = system(command); I'd feel a lot safer if this was int rc = system(nsPrintfCString("/system/bin/fm_qsoc_patches %d 0", sTavaruaVersion).get()); because we can't overflow that. But this is safe as-is, so you don't need to change it. >@@ -201,39 +143,116 @@ EnableFMRadio(const hal::FMRadioSettings& aInfo) >+static void * >+runTavaruaRadio(void *) Please indicate either with a comment or an assertion what thread this is running on. Same for initTavaruaRadio. >+void >+EnableFMRadio(const hal::FMRadioSettings& aInfo) >+{ ibid. >+ sRadioFD = fd; >+ sRadioSettings = aInfo; >+ fd.forget(); I think you can do |sRadioFD = fd.forget();|. r=me with nits picked.
Updated•9 years ago
|
Attachment #664161 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 2•9 years ago
|
||
Nits addressed except for the nsPrintfCString part. That line just looks weird to me..
Attachment #664161 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25086637d1f
Target Milestone: --- → mozilla18
Comment 4•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a25086637d1f
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•