Closed Bug 1095362 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

defect
Not set

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: nobody → bhsu
As bug 1077075, this is also a request from partners. Please get aware of this, Anshul :)
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+
Attachment #8545888 - Flags: review?(szchen)
Attachment #8545889 - Flags: review?(szchen)
Attachment #8545890 - Flags: review?(szchen)
Attachment #8545891 - Flags: review?(szchen)
Attachment #8545892 - Flags: review?(szchen)
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-
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+
Attachment #8545892 - Attachment is obsolete: true
Attachment #8546506 - Flags: review+
Attachment #8545888 - Attachment is obsolete: true
Attachment #8546540 - Flags: review?(szchen)
Attachment #8546540 - Attachment description: 2.patch → Part 2: Modify related dom implementaion (DOM) (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)
Attachment #8545890 - Attachment is obsolete: true
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)
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+
Attachment #8546543 - Attachment is obsolete: true
Attachment #8547163 - Flags: review?(szchen)
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+
(In reply to Ben Hsu [:benhsu] from comment #26)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd69795b6508

Thank you, Ben and Aknow!
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.
(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!
Blocks: 1124992
You need to log in before you can comment on or make changes to this bug.