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

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ 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.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Recognize the CDMA call-waiting scenario independently of the call state

Comment 3

[Doug Sherk (:drs) (inactive)]

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.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 5

[Doug Sherk (:drs) (inactive)]

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.

Comment 6

[Michael Schwartz [:m4]]

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.

Comment 7

[Michael Schwartz [:m4]]

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]?

Comment 8

[Doug Sherk (:drs) (inactive)]

(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.

Comment 9

[Michael Schwartz [:m4]]

(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.

Comment 10

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(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.

Comment 11

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

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.

Comment 12

[Gabriele Svelto [:gsvelto]]

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?

Comment 13

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(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

Comment 14

[Gabriele Svelto [:gsvelto]]

OK, next iteration of this patch coming soon.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 16

[Doug Sherk (:drs) (inactive)]

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.

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: [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.

Comment 18

[Doug Sherk (:drs) (inactive)]

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?

Comment 19

[Gabriele Svelto [:gsvelto]]

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.

Comment 20

[Eric Chang [:ericcc] [:echang]]

Comment on attachment 8524108 [details] [diff] [review]
[PATCH v4] Recognize the CDMA call-waiting scenario independently of the call state

Did some test cases & exploratory, no bugs found
https://moztrap.mozilla.org/manage/cases/?&pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-status=active&filter-priority=1&filter-priority=2&filter-priority=3&filter-name=cdma&filter-tag=2776&filter-productversion=217

Comment 21

[Michael Schwartz [:m4]]

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);
   }

Comment 22

[Gabriele Svelto [:gsvelto]]

(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.

Comment 23

[Gabriele Svelto [:gsvelto]]

See comment 22, I forgot the NI?

Comment 24

[Michael Schwartz [:m4]]

(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.