Closed
Bug 746496
Opened 13 years ago
Closed 13 years ago
B2G telephony: update the audio system for incoming calls and holding calls
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: hsinyi, Assigned: hsinyi)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
We need to do some work for updating the audio system for holding calls. Also, some modification is needed to update the audio correctly when there's an incoming call.
Assignee | ||
Comment 1•13 years ago
|
||
I just realized that it fails in assigning value to |gAudioManager.phoneState| in updateCallAudioState(), RadioInterfaceLayer.js. It also seems that gAudioManager.setForceForUse() doesn't work because nothing changes even after I marked this.
Assignee | ||
Comment 2•13 years ago
|
||
The audio state should be updated with the state of *activeCall*. This patch updates *activeCall* and the audio state for holding calls and multiple calls. At any given time, there is only one *activeCall* even we support multiple. Also, as I mentioned in Comment 1, I found it fails in assigning a value to |gAudioManager.phoneState|. I.e. the value of |gAudioManager.phoneState| is the same no matter what we assign. However, when I marked and unmarked the line, gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_IN_CALL; in RadioInterfaceLayer.js, the audio system acted differently, even the assignment failed. I have no idea why. And I am also wondering if this is out of the scope of this bug? Would it be better to file a new bug for deeper discussion, if this is really a bug? Thanks!
Attachment #617452 -
Flags: review?(philipp)
Assignee | ||
Updated•13 years ago
|
Attachment #617452 -
Attachment is obsolete: true
Attachment #617452 -
Attachment is patch: false
Attachment #617452 -
Flags: review?(philipp)
Assignee | ||
Comment 3•13 years ago
|
||
Update the audio system as call changes The audio state should be updated with the state of *activeCall*. This patch updates *activeCall* and the audio state for holding calls and multiple calls. At any given time, there is only one *activeCall* even we support multiple. Also, as I mentioned in Comment 1, I found it fails in assigning a value to |gAudioManager.phoneState|. I.e. the value of |gAudioManager.phoneState| is the same no matter what we assign. However, when I marked and unmarked the line, gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_IN_CALL; in RadioInterfaceLayer.js, the audio system acted differently, even the assignment failed. I have no idea why. And I am also wondering if this is out of the scope of this bug? Would it be better to file a new bug for deeper discussion, if this is really a bug? Thanks!
Attachment #617453 -
Flags: review?(philipp)
Assignee | ||
Comment 4•13 years ago
|
||
Comment on attachment 617453 [details] [diff] [review] Update the audio system as call changes (v2) Sorry for canceling the request for review. I just thought of a better implementation and would like to submit a update later. Thanks :)
Attachment #617453 -
Attachment is obsolete: true
Attachment #617453 -
Flags: review?(philipp)
Assignee | ||
Comment 5•13 years ago
|
||
The audio state should be updated with the state of *activeCall*. This patch updates *activeCall* and the audio state for holding calls and multiple calls. At any given time, there is only one *activeCall* even we support multiple. This patch also fixes the broken setter of |gAudioManager.phoneState|. So the incorrect assignment I mentioned in Comment 3 doesn't exist.
Attachment #618573 -
Flags: review?(philipp)
Comment 6•13 years ago
|
||
Comment on attachment 618573 [details] [diff] [review] Patch (v3) Review of attachment 618573 [details] [diff] [review]: ----------------------------------------------------------------- I love that we're centralizing the active call computation in one place now. However, it's not quite consistent with how `Telephony.cpp` computes the active call [1]. We might want to update `Telephony.cpp` to use the same logic that you implemented in `ril_worker.js` now. Also, we might as well update `RadioInterfacdeLayer.handleEnumerateCalls()` and the corresponding code in `RILContentHelper` to simply use `call.isActive` rather than comparing indexes. [1] https://mxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#447 ::: dom/system/gonk/AudioManager.cpp @@ +166,5 @@ > if (AudioSystem::setPhoneState(aState)) { > return NS_ERROR_FAILURE; > } > + > + mPhoneState = aState; Good catch! ::: dom/system/gonk/ril_worker.js @@ +1707,4 @@ > currentCall.state = newCall.state; > + this.currentCallsLength = newCalls.length; > + currentCall.isActive = > + this._isActiveCall(currentCall.state, this.currentCallsLength); If all you're doing is passing `newCalls.length` along to _isActiveCall, why even store it in `this.currentCallsLength`? Also, you're doing the storing in a loop... Could just do it once at the beginning, no? (With the right guards against `newCalls` of course.)
Attachment #618573 -
Flags: review?(philipp)
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #6) > Comment on attachment 618573 [details] [diff] [review] > Patch (v3) > > Review of attachment 618573 [details] [diff] [review]: > ----------------------------------------------------------------- > > I love that we're centralizing the active call computation in one place now. > However, it's not quite consistent with how `Telephony.cpp` computes the > active call [1]. We might want to update `Telephony.cpp` to use the same > logic that you implemented in `ril_worker.js` now. Also, we might as well > update `RadioInterfacdeLayer.handleEnumerateCalls()` and the corresponding > code in `RILContentHelper` to simply use `call.isActive` rather than > comparing indexes. > Yap, you are right. Thanks for pointing this out. I shall revise those as well. > > ::: dom/system/gonk/ril_worker.js > @@ +1707,4 @@ > > currentCall.state = newCall.state; > > + this.currentCallsLength = newCalls.length; > > + currentCall.isActive = > > + this._isActiveCall(currentCall.state, this.currentCallsLength); > > If all you're doing is passing `newCalls.length` along to _isActiveCall, why > even store it in `this.currentCallsLength`? Also, you're doing the storing > in a loop... Could just do it once at the beginning, no? (With the right > guards against `newCalls` of course.) Agree, there could be a better way to implementing this block. Will re-implement this. Thanks~
Assignee | ||
Comment 8•13 years ago
|
||
The updated patch addresses the comments above. Thanks!
Attachment #618573 -
Attachment is obsolete: true
Attachment #620596 -
Flags: review?(philipp)
Comment 9•13 years ago
|
||
Comment on attachment 620596 [details] [diff] [review] Patch (v4) Review of attachment 620596 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! ::: dom/system/gonk/RadioInterfaceLayer.js @@ +503,3 @@ > for (let i in calls) { > calls[i].state = convertRILCallState(calls[i].state); > } You know, we could even do this in ril_worker.js, then the mainthread doesn't have to do anything special anymore... Not super necessary, just a thought. ::: dom/system/gonk/ril_worker.js @@ +1830,5 @@ > + case CALL_STATE_INCOMING: > + case CALL_STATE_DIALING: > + case CALL_STATE_ALERTING: > + case CALL_STATE_ACTIVE: > + return true; I'm excited about this because I think it will make the API saner (I discovered the inconsistencies you're fixing here while writing tests...)
Attachment #620596 -
Flags: review?(philipp) → review+
Comment 10•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e002bf20e8b3
Target Milestone: --- → mozilla15
Comment 11•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e002bf20e8b3
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•