Closed
Bug 1095362
Opened 10 years ago
Closed 9 years ago
Augment telephonyCallGroup API to notify client code of errors when attempting hold(), resume(), etc
Categories
(Firefox OS Graveyard :: RIL, defect)
Firefox OS Graveyard
RIL
Tracking
(Not tracked)
RESOLVED
FIXED
2.2 S4 (23jan)
People
(Reporter: hsinyi, Assigned: bhsu)
References
Details
Attachments
(7 files, 8 obsolete files)
1.49 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
7.78 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
3.19 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
10.23 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
8.36 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
6.69 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
This is very similar to Bug 1077075. File this separate bug so that we could land small pieces one by one.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bhsu
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8545179 -
Flags: review?(htsai)
Reporter | ||
Comment 2•9 years ago
|
||
As bug 1077075, this is also a request from partners. Please get aware of this, Anshul :)
Blocks: b2g-ril-interface
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8545179 [details] [diff] [review] Part 1: Change the return types to promises (WebIDL) Review of attachment 8545179 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you!
Attachment #8545179 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8545888 -
Flags: review?(szchen)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8545889 -
Flags: review?(szchen)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8545890 -
Flags: review?(szchen)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8545891 -
Flags: review?(szchen)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8545892 -
Flags: review?(szchen)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8545893 -
Flags: review?(szchen)
Comment 10•9 years ago
|
||
Comment on attachment 8545889 [details] [diff] [review] Part 3: Internal interface changes (IDL) Review of attachment 8545889 [details] [diff] [review]: ----------------------------------------------------------------- You need to update the uuid of nsIGonkTelephonyService Please use "./mach update-uuids nsITelephonyService" to update the uuids of all dependent interface. ::: dom/telephony/nsITelephonyService.idl @@ +5,5 @@ > #include "nsISupports.idl" > > interface nsIMobileCallForwardingOptions; > > +[scriptable, uuid(dc71b2d8-3985-4612-af26-f021c9825881)] I didn't see any changes in nsITelephonyListener. You don't have to update its uuid.
Attachment #8545889 -
Flags: review?(szchen) → review-
Assignee | ||
Comment 11•9 years ago
|
||
Thanks for the review and the correct usage of |./mach update update-uuids [interface]|. I took the [file_name] as the paremeter, which is not correct.
Attachment #8545889 -
Attachment is obsolete: true
Attachment #8546399 -
Flags: review?(szchen)
Updated•9 years ago
|
Attachment #8546399 -
Flags: review?(szchen) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8545888 [details] [diff] [review] Part 2: Modify related dom implementaion (DOM) Review of attachment 8545888 [details] [diff] [review]: ----------------------------------------------------------------- Let's have a utility function to create a promise [1] and use it in all the methods. The way could reduce some duplicated code among functions. [1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/USSDSession.cpp?from=USSDSession.cpp#49 ::: dom/telephony/TelephonyCallGroup.cpp @@ +245,5 @@ > + return nullptr; > + } > + > + nsRefPtr<Promise> promise = Promise::Create(global, aRv); > + NS_ENSURE_TRUE(!aRv.Failed(), nullptr); Let's not use NS_ENSURE_TRUE any more in new code. Remember to fix the same usage elsewhere. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Error_handling if (aRv.Failed()) { return nullptr; } @@ +264,5 @@ > TelephonyCallGroup::Add(TelephonyCall& aCall, > TelephonyCall& aSecondCall, > ErrorResult& aRv) > { > + MOZ_ASSERT(!mCalls.IsEmpty()); This is wrong. When we are first creating a conference, mCalls is empty. Only after the conference is ready, those two calls then be added into mCalls.
Attachment #8545888 -
Flags: review?(szchen) → review-
Comment 13•9 years ago
|
||
Comment on attachment 8545890 [details] [diff] [review] Part 4: Internal architecture changes (TelephonyService.js) Review of attachment 8545890 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/gonk/TelephonyService.js @@ +1021,1 @@ > this._notifyAllListeners("notifyConferenceError", [response.errorName, It looks like we could remove notifyConferenceError because the same purpose is replaced by promise. If you agree with me, please examine the path from here to telephony dom and remove the unnecessary code. @@ +1065,1 @@ > } I'm thinking about restructuring the function as below. function isCdmaSeparateCallSuccess() { let call = this._currentCalls[aClientId][aCallIndex]; if (!call || !call.isConference || !call.childId) { return false; } return true; } if (response.isCdma) { if (!isCdmaSeparateCallSuccess()) { aCallback.notifyError(RIL.GECKO_ERROR_GENERIC_FAILURE); } else { let call = this._currentCalls[aClientId][aCallIndex]; let childCall = this._currentCalls[aClientId][call.childId]; this.notifyCallDisconnected(aClientId, childCall); aCallback.notifySuccess(); } } else { aCallback.notifySuccess(); } Something like this. My idea is to put the aCallback access in the same level. Therefore the code is easier to understand.
Attachment #8545890 -
Flags: review?(szchen) → review-
Comment 14•9 years ago
|
||
Comment on attachment 8545891 [details] [diff] [review] Part 5: Internal architecture changes (ril_worker.js) Review of attachment 8545891 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1958,5 @@ > }, > > holdConference: function(options) { > if (this._isCdma) { > + // With a CDMA device, this branch shouln't be executed. I think that it's possible to be called from Telephony.js when we try to automatically hold the first call before dialing out the second one. In that case, returning a failure response might cause a problem. We probably need to fix telephonyService.js so that the above case won't happen. Then throw an exception here because we think it shouldn't be executed. @@ +1970,5 @@ > }, > > resumeConference: function(options) { > if (this._isCdma) { > + // With a CDMA device, this branch shouln't be executed. ditto, throw exception.
Attachment #8545891 -
Flags: review?(szchen) → review-
Comment 15•9 years ago
|
||
Comment on attachment 8545892 [details] [diff] [review] Part 6: Internal architecture changes (IPC) Review of attachment 8545892 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed ::: dom/telephony/ipc/TelephonyParent.cpp @@ +69,5 @@ > case IPCTelephonyRequest::TCancelUSSDRequest: { > const CancelUSSDRequest& request = aRequest.get_CancelUSSDRequest(); > service->CancelUSSD(request.clientId(), actor); > return true; > + } remove the trailing space
Attachment #8545892 -
Flags: review?(szchen) → review+
Updated•9 years ago
|
Attachment #8545893 -
Flags: review?(szchen) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8545892 -
Attachment is obsolete: true
Attachment #8546506 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8545888 -
Attachment is obsolete: true
Attachment #8546540 -
Flags: review?(szchen)
Assignee | ||
Updated•9 years ago
|
Attachment #8546540 -
Attachment description: 2.patch → Part 2: Modify related dom implementaion (DOM) (V2)
Assignee | ||
Comment 18•9 years ago
|
||
Hi aknow, I do agree with you, but I think we can deprecate |notifyConferenceError| in another bug, then we can inform gaia about this.
Attachment #8546543 -
Flags: review?(szchen)
Assignee | ||
Updated•9 years ago
|
Attachment #8545890 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Hi aknow, You are right. Those branches can be executed. However, it's hard to prevent that at Telephony.js, since Telephony.js doesn't aware whether the underlying hardware is a CDMA device or a GSM device. Thus, I think we can simply fail those requests without throwing exceptions here.
Attachment #8545891 -
Attachment is obsolete: true
Attachment #8546545 -
Flags: review?(szchen)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8546545 -
Attachment is obsolete: true
Attachment #8546545 -
Flags: review?(szchen)
Attachment #8546546 -
Flags: review?(szchen)
Updated•9 years ago
|
Attachment #8546540 -
Flags: review?(szchen) → review+
Comment 21•9 years ago
|
||
Comment on attachment 8546543 [details] [diff] [review] Part 4: Internal architecture changes (TelephonyService.js) (V2) Review of attachment 8546543 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/gonk/TelephonyService.js @@ +1088,5 @@ > > if (response.isCdma) { > + // See 3gpp2, S.R0006-522-A v1.0. Table 4, XID 6S. > + let call = this._currentCalls[aClientId][aCallIndex]; > + let childId = call.childId; It's wrong. You cannot directly access |call.childId| before you make sure that |call| is valid. @@ +1096,5 @@ > + } > + > + let childCall = this._currentCalls[aClientId][childId]; > + this.notifyCallDisconnected(aClientId, childCall); > + aCallback.notifySuccess(); You just call notifySuccess() twice, here and later.
Attachment #8546543 -
Flags: review?(szchen) → review-
Comment 22•9 years ago
|
||
Comment on attachment 8546546 [details] [diff] [review] Part 5: Internal architecture changes (ril_worker.js) (V2) Review of attachment 8546546 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/system/gonk/ril_worker.js @@ +1979,5 @@ > }, > > holdConference: function(options) { > if (this._isCdma) { > + // With a CDMA device, we cannot hold a conference call. We cannot hold a conference call on CDMA. @@ +1991,5 @@ > }, > > resumeConference: function(options) { > if (this._isCdma) { > + // With a CDMA device, since a conference call cannot be held, We cannot resume a conference call on CDMA.
Attachment #8546546 -
Flags: review?(szchen) → review+
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8546543 -
Attachment is obsolete: true
Attachment #8547163 -
Flags: review?(szchen)
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8546546 -
Attachment is obsolete: true
Attachment #8547164 -
Flags: review+
Comment 25•9 years ago
|
||
Comment on attachment 8547163 [details] [diff] [review] Part 4: Internal architecture changes (TelephonyService.js) (V3) Review of attachment 8547163 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.
Attachment #8547163 -
Flags: review?(szchen) → review+
Assignee | ||
Comment 26•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd69795b6508
Keywords: checkin-needed
Reporter | ||
Comment 27•9 years ago
|
||
(In reply to Ben Hsu [:benhsu] from comment #26) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd69795b6508 Thank you, Ben and Aknow!
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/a48c2bd869f4 https://hg.mozilla.org/integration/b2g-inbound/rev/1bbcc30b950b https://hg.mozilla.org/integration/b2g-inbound/rev/90515a836a64 https://hg.mozilla.org/integration/b2g-inbound/rev/a232d580f5e9 https://hg.mozilla.org/integration/b2g-inbound/rev/49140c36a7fd https://hg.mozilla.org/integration/b2g-inbound/rev/1357f398c622 https://hg.mozilla.org/integration/b2g-inbound/rev/87b3b48ccac0
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a48c2bd869f4 https://hg.mozilla.org/mozilla-central/rev/1bbcc30b950b https://hg.mozilla.org/mozilla-central/rev/90515a836a64 https://hg.mozilla.org/mozilla-central/rev/a232d580f5e9 https://hg.mozilla.org/mozilla-central/rev/49140c36a7fd https://hg.mozilla.org/mozilla-central/rev/1357f398c622 https://hg.mozilla.org/mozilla-central/rev/87b3b48ccac0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S4 (23jan)
Comment 30•9 years ago
|
||
Just as a note, any method annotated [NewObject] that returns a promise type is assumed by codegen to be able to throw as of when bug 1113827 landed, so they don't need explicit [Throws] annotations.
Reporter | ||
Comment 31•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #30) > Just as a note, any method annotated [NewObject] that returns a promise type > is assumed by codegen to be able to throw as of when bug 1113827 landed, so > they don't need explicit [Throws] annotations. Thank you for sharing!
You need to log in
before you can comment on or make changes to this bug.
Description
•