Relax the checks for CDMA call-waiting to ensure stable operation while switching calls

RESOLVED WONTFIX

Status

Firefox OS
Gaia::Dialer
--
major
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: gsvelto, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1087166 +++

Handling CDMA call-waiting mode relies on an overly strict check of the call conditions which is unnecessary. We should relax it to ensure that call switching in that mode works regardless of all those conditions being met.
(Reporter)

Comment 1

4 years ago
Created attachment 8519854 [details] [diff] [review]
[PATCH] Recognize the CDMA call-waiting scenario independently of the call state

I've addressed your comments as well as the issues that were highlighted in testing. I've also exposed the cdmaCallWaiting() function to test it directly and modified the mock to reflect the relaxed CDMA call-waiting condition.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8519854 - Flags: review?(drs.bugzilla)
(Reporter)

Comment 2

4 years ago
Created attachment 8519856 [details] [review]
[PULL REQUEST] Recognize the CDMA call-waiting scenario independently of the call state
Comment on attachment 8519854 [details] [diff] [review]
[PATCH] Recognize the CDMA call-waiting scenario independently of the call state

Review of attachment 8519854 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/callscreen/js/calls_handler.js
@@ +456,5 @@
>  
> +    if (cdmaCallWaiting()) {
> +      // Connected or held call in CDMA mode, hold to answer to the second call
> +      handledCalls[0].call.hold();
> +      btHelper.answerWaitingCall();

We should explain why there's no `telephony.active` call since this is a very specific edge case that only happens with a certain setup, and would be confusing to onlookers without the history here.

::: apps/callscreen/test/unit/calls_handler_test.js
@@ +621,5 @@
>        });
>  
> +      test('should correctly detect the condition', function() {
> +        assert.isTrue(CallsHandler.cdmaCallWaiting());
> +      });

I was actually thinking that we would test `CallsHandler.cdmaCallWaiting()` directly as a suite, checking that it behaves correctly with every permutation of inputs. This should only require 4 tests anyways since there are only 2 inputs. I think that this is more in the spirit of unit testing.
Attachment #8519854 - Flags: review?(drs.bugzilla) → review-
(Reporter)

Comment 4

4 years ago
Created attachment 8520669 [details] [diff] [review]
[PATCH v2] Recognize the CDMA call-waiting scenario independently of the call state

Review comments addressed + added some comments to explain what's going on.
Attachment #8519854 - Attachment is obsolete: true
Attachment #8520669 - Flags: review?(drs.bugzilla)
Comment on attachment 8520669 [details] [diff] [review]
[PATCH v2] Recognize the CDMA call-waiting scenario independently of the call state

Review of attachment 8520669 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks.

::: apps/callscreen/js/calls_handler.js
@@ +454,5 @@
>        return;
>      }
>  
> +    if (cdmaCallWaiting()) {
> +      /* Connected or held call in CDMA mode. In this mode only a single call

Please add the bug number here as well.
Attachment #8520669 - Flags: review?(drs.bugzilla) → review+
Comment on attachment 8520669 [details] [diff] [review]
[PATCH v2] Recognize the CDMA call-waiting scenario independently of the call state

Review of attachment 8520669 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/callscreen/js/calls_handler.js
@@ +762,5 @@
>     *
>     * @return {Boolean} Returns true if we're in CDMA call waiting mode.
>     */
>    function cdmaCallWaiting() {
> +    return !!((telephony.calls.length === 1) && telephony.calls[0].secondId);

Why are we checking the call length?  If there is a secondId, there is call waiting.
Thinking about it more.  We need to know from Hsin-Yi what the API behaviour will be in the conference call to call-waiting case before we can move forward.  More specifically, can we count on the secondId being attached to telephony.calls[0]?
Flags: needinfo?(htsai)
(In reply to Michael Schwartz [:m4] from comment #6)
> Why are we checking the call length?  If there is a secondId, there is call
> waiting.

Without this check, we will be checking |telephony.calls[0].secondId| without a null check on |telephony.calls[0]|, so we could get a runtime error.
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #8)
> 0 perhaps?  Let's hear from Hsin-Yi to see if I'm just being paranoid.
(In reply to Michael Schwartz [:m4] from comment #9)
> (In reply to Doug Sherk (:drs) (use needinfo?) from comment #8)
> > 0 perhaps?  Let's hear from Hsin-Yi to see if I'm just being paranoid.

Michael, thanks for being cautious. Checking the call length fits the design very well even considering waiting call comes after a cdma 3way call. I verified the scenario before.
Flags: needinfo?(htsai)
Comment on attachment 8520669 [details] [diff] [review]
[PATCH v2] Recognize the CDMA call-waiting scenario independently of the call state

Review of attachment 8520669 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/callscreen/js/calls_handler.js
@@ +567,5 @@
> +     * it might be in the 'held' state thus telephony.active is null and we
> +     * use the first available call instead. */
> +    var call = telephony.active || telephony.calls[0];
> +
> +    call.hold();

If this isn't an active call, we should "call.resume()." Sorry for not mentioning this clearly enough before.
(Reporter)

Comment 12

4 years ago
Hsin-Yi, I'm not 100% sure I understand your last comment. Should I use |resume()| instead of |hold()| to switch the calls when the call's status is held?
Flags: needinfo?(htsai)
(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> Hsin-Yi, I'm not 100% sure I understand your last comment. Should I use
> |resume()| instead of |hold()| to switch the calls when the call's status is
> held?

Yes, use |resume()| instead of |hold()| to switch the calls when the call's status is held, otherwise code simply returns [1].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyCall.cpp?from=TelephonyCall.cpp&case=true#271
Flags: needinfo?(htsai)
(Reporter)

Comment 14

4 years ago
OK, next iteration of this patch coming soon.
(Reporter)

Comment 15

4 years ago
Created attachment 8523863 [details] [diff] [review]
[PATCH v3] Recognize the CDMA call-waiting scenario independently of the call state

Third iteration of the patch taking into account that if the sole call is in the 'held' state then |telephony.active| will be null and we should call resume() instead of hold() to switch the calls. I've also added more unit-tests to cover this scenario.

Hopefully we'll clean up this API soon and remove all this cruft.
Attachment #8520669 - Attachment is obsolete: true
Attachment #8523863 - Flags: review?(drs.bugzilla)
Comment on attachment 8523863 [details] [diff] [review]
[PATCH v3] Recognize the CDMA call-waiting scenario independently of the call state

Review of attachment 8523863 [details] [diff] [review]:
-----------------------------------------------------------------

r- due to my final comment, but this isn't actually a big deal.

::: apps/callscreen/js/calls_handler.js
@@ +461,5 @@
>        return;
>      }
>  
> +    if (cdmaCallWaiting()) {
> +      /* Connected or held call in CDMA mode. In this mode only a single call

Please include the bug number here as this is really confusing on the surface and having some background would be nice.

@@ +464,5 @@
> +    if (cdmaCallWaiting()) {
> +      /* Connected or held call in CDMA mode. In this mode only a single call
> +       * is present and it might be in the 'held' state on certain RILs thus we
> +       * can't access it via the telephony.active field which would be null. */
> +      if (handledCalls[0].call.state === 'held') {

`var call = handledCalls[0];`

@@ +576,5 @@
>      }
>  
> +    /* In CDMA call waiting mode only one call is present and, on certain RILs,
> +     * it might be in the 'held' state thus telephony.active is null and we
> +     * use the first available call instead. */

I don't think we can get to this spot in CDMA call waiting mode. If we get through line 486, we will return at line 575.
Attachment #8523863 - Flags: review?(drs.bugzilla) → review-
(Reporter)

Comment 17

4 years ago
Created attachment 8524108 [details] [diff] [review]
[PATCH v4] Recognize the CDMA call-waiting scenario independently of the call state

Nits addressed, if this one's good let's proceed with testing.
Attachment #8523863 - Attachment is obsolete: true
Attachment #8524108 - Flags: review?(drs.bugzilla)
Comment on attachment 8524108 [details] [diff] [review]
[PATCH v4] Recognize the CDMA call-waiting scenario independently of the call state

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #16)
> I don't think we can get to this spot in CDMA call waiting mode. If we get
> through line 486, we will return at line 575.

Sorry, this was wrong. I was looking at the context in the review tool incorrectly.

Looks good! Let's test this now.

Michael, could you test this to make sure it fixes the issue for you?

Eric, could you test this to make sure that it doesn't break our standard CDMA setup?
Flags: needinfo?(mschwart)
Flags: needinfo?(echang)
Attachment #8524108 - Flags: review?(drs.bugzilla) → review+
Flags: needinfo?(echang)
Attachment #8524108 - Flags: qa-approval?(echang)
(Reporter)

Comment 19

4 years ago
The green try run is here: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=cdc0cc47a9da

Waiting for testing on the actual devices before merging though.
Hi,

Unfortunately, this change took too long for the customer so we had to make the change in our code to be inline with the reference RIL behaviour.  Although most of this change is no longer needed, please still merge the cdmaCallWaiting() portion as that is right thing to do.

Specifically only,

   function cdmaCallWaiting() {
-    return ((telephony.calls.length == 1) &&
-            (telephony.calls[0].state == 'connected') &&
-            (telephony.calls[0].secondId));
+    return !!((telephony.calls.length === 1) && telephony.calls[0].secondId);
   }
Flags: needinfo?(mschwart)
(Reporter)

Comment 22

3 years ago
(In reply to Michael Schwartz [:m4] from comment #21)
> Unfortunately, this change took too long for the customer so we had to make
> the change in our code to be inline with the reference RIL behaviour. 
> Although most of this change is no longer needed, please still merge the
> cdmaCallWaiting() portion as that is right thing to do.

OK, so now we can assume that the single CDMA call is always in the 'connected' state when in call-waiting mode, is this correct? Sorry for asking again but I want to be 100% sure even before landing the small change in your comment.
(Reporter)

Comment 23

3 years ago
See comment 22, I forgot the NI?
Flags: needinfo?(mschwart)
(In reply to Gabriele Svelto [:gsvelto] from comment #22)
> OK, so now we can assume that the single CDMA call is always in the
> 'connected' state when in call-waiting mode, is this correct? Sorry for
> asking again but I want to be 100% sure even before landing the small change
> in your comment.

No worries - clarity is important ;)  Yes, the CDMA call is always in 'connected' state when in call-waiting.

But again, just to be clear, the check should still be removed as that is not a detail the UI should worry about... but that's absolutely not a priority change.  I leave it to you.
Flags: needinfo?(mschwart)
(Reporter)

Updated

2 years ago
Assignee: gsvelto → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.