Augment telephonyCallGroup API to notify client code of errors when attempting hold(), resume(), etc

RESOLVED FIXED in 2.2 S4 (23jan)

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hsinyi, Assigned: HoPang)

Tracking

(Blocks: 1 bug)

unspecified
2.2 S4 (23jan)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 8 obsolete attachments)

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
HoPang
: 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
HoPang
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
This is very similar to Bug 1077075. File this separate bug so that we could land small pieces one by one.
(Assignee)

Updated

3 years ago
Assignee: nobody → bhsu
(Assignee)

Comment 1

3 years ago
Created attachment 8545179 [details] [diff] [review]
Part 1: Change the return types to promises (WebIDL)
Attachment #8545179 - Flags: review?(htsai)
(Reporter)

Comment 2

3 years ago
As bug 1077075, this is also a request from partners. Please get aware of this, Anshul :)
Blocks: 959978
(Reporter)

Comment 3

3 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

3 years ago
Created attachment 8545888 [details] [diff] [review]
Part 2: Modify related dom implementaion (DOM)
Attachment #8545888 - Flags: review?(szchen)
(Assignee)

Comment 5

3 years ago
Created attachment 8545889 [details] [diff] [review]
Part 3: Internal interface changes (IDL)
Attachment #8545889 - Flags: review?(szchen)
(Assignee)

Comment 6

3 years ago
Created attachment 8545890 [details] [diff] [review]
Part 4: Internal architecture changes (TelephonyService.js)
Attachment #8545890 - Flags: review?(szchen)
(Assignee)

Comment 7

3 years ago
Created attachment 8545891 [details] [diff] [review]
Part 5: Internal architecture changes (ril_worker.js)
Attachment #8545891 - Flags: review?(szchen)
(Assignee)

Comment 8

3 years ago
Created attachment 8545892 [details] [diff] [review]
Part 6: Internal architecture changes (IPC)
Attachment #8545892 - Flags: review?(szchen)
(Assignee)

Comment 9

3 years ago
Created attachment 8545893 [details] [diff] [review]
Part 7: Update related testcases
Attachment #8545893 - Flags: review?(szchen)
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

3 years ago
Created attachment 8546399 [details] [diff] [review]
Part 3: Internal interface changes (IDL) (V2)

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)
Attachment #8546399 - Flags: review?(szchen) → review+
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 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 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 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+
Attachment #8545893 - Flags: review?(szchen) → review+
(Assignee)

Comment 16

3 years ago
Created attachment 8546506 [details] [diff] [review]
Part 6: Internal architecture changes (IPC) (V2)
Attachment #8545892 - Attachment is obsolete: true
Attachment #8546506 - Flags: review+
(Assignee)

Comment 17

3 years ago
Created attachment 8546540 [details] [diff] [review]
Part 2: Modify related dom implementaion (DOM) (V2)
Attachment #8545888 - Attachment is obsolete: true
Attachment #8546540 - Flags: review?(szchen)
(Assignee)

Updated

3 years ago
Attachment #8546540 - Attachment description: 2.patch → Part 2: Modify related dom implementaion (DOM) (V2)
(Assignee)

Comment 18

3 years ago
Created attachment 8546543 [details] [diff] [review]
Part 4: Internal architecture changes (TelephonyService.js) (V2)

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

3 years ago
Attachment #8545890 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8546545 [details] [diff] [review]
Part 5: Internal architecture changes (ril_worker.js) (V2)

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

3 years ago
Created attachment 8546546 [details] [diff] [review]
Part 5: Internal architecture changes (ril_worker.js) (V2)
Attachment #8546545 - Attachment is obsolete: true
Attachment #8546545 - Flags: review?(szchen)
Attachment #8546546 - Flags: review?(szchen)
Attachment #8546540 - Flags: review?(szchen) → review+
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 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

3 years ago
Created attachment 8547163 [details] [diff] [review]
Part 4: Internal architecture changes (TelephonyService.js) (V3)
Attachment #8546543 - Attachment is obsolete: true
Attachment #8547163 - Flags: review?(szchen)
(Assignee)

Comment 24

3 years ago
Created attachment 8547164 [details] [diff] [review]
Part 5: Internal architecture changes (ril_worker.js) (V3)
Attachment #8546546 - Attachment is obsolete: true
Attachment #8547164 - Flags: review+
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

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd69795b6508
Keywords: checkin-needed
(Reporter)

Comment 27

3 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!
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S4 (23jan)
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

3 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!
(Reporter)

Updated

3 years ago
Blocks: 1124992
You need to log in before you can comment on or make changes to this bug.