1029721 - CDMA call waiting call is not logged in call history

Status: RESOLVED FIXED

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

Description

[Anshul]

STR:
1. On a CDMA device, make an outgoing call
2. When the call in step 1 is connected place a call to this device from another device (incoming call)
3. Accept the cdma call waiting call.
4. End both the calls.
5. Go to the Call log

Expected: Both the outgoing and the incoming call should be shown
Observed: Only the outgoing call is available in the call log screen.

Comment 1

[Jason Smith [:jsmith]]

Wilfred - Are there any production devices shipping in 2.0 with CDMA support?

Comment 2

[Anshul]

(In reply to Jason Smith [:jsmith] from comment #1)
> Wilfred - Are there any production devices shipping in 2.0 with CDMA support?
This issue is already reproducible on 1.4. The version 1.4 is FFOS's first official CDMA release but no one seems to have noticed this bug.

Comment 3

[Wilfred Mathanaraj [:WDM]]

I dont think we have any 1.4 product with CDMA out there - but 2.0 may be the first one with Madai. 
We can not do any CDMA testing in Europe - given there is no CDMA network.

Sounds like a bug we should fix.

Comment 4

[Anshul]

I can help test the patch.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

I'm dealing with a few 1.4/1.3T+ blockers but once I'm done with them I'll try to fix this bug if no one else is available. Realistically that's going to happen sometimes in the middle of next week.

Comment 6

[Etienne Segonzac (:etienne)]

The call log is populated by the telephony-call-ended system message.
And in the case of CDMA call waiting, we never actually see 2 call objects, we have only 1 call with an optional secondNumber property, which isn't even bubbled through the system message [1] (note that that probably would not help since by the time the call ends the secondNumber property should be empty).

This looks like a feature request requiring gecko and gaia work, we might want to reconsider the blockiness.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js#730

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Etienne Segonzac (:etienne) from comment #6)
> The call log is populated by the telephony-call-ended system message.

Now that you make me think about it this bug might be unfixable because of how CDMA message works: AFAIK there is no message from the network about the call ending - ever. You get a message indicating that there's an incoming call and you can accept it or not but irrespective of if you've replied the second call or not you don't get anything back from the network. Hsin-Yi can you confirm this and maybe give us some more details? Would there be a way to introduce a telephony-call-ended message for CDMA's second call? Maybe using a timeout or something?

Comment 8

[Doug Sherk (:drs) (inactive)]

See comment 6 and comment 7.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Doug Sherk (:drs) from comment #8)
> See comment 6 and comment 7.

Wilfred - Can you triage this based on the new info provided in comment 6 & 7?

Comment 10

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

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> (In reply to Etienne Segonzac (:etienne) from comment #6)
> > The call log is populated by the telephony-call-ended system message.
> 
> Now that you make me think about it this bug might be unfixable because of
> how CDMA message works: AFAIK there is no message from the network about the
> call ending - ever. 

Correct. It's hardly possible in reference phones. In customized phones, gecko can get particular messages from operator to know the exact state of the incoming call, but not in reference design.

> You get a message indicating that there's an incoming
> call and you can accept it or not but irrespective of if you've replied the
> second call or not you don't get anything back from the network. Hsin-Yi can
> you confirm this and maybe give us some more details? Would there be a way
> to introduce a telephony-call-ended message for CDMA's second call? Maybe
> using a timeout or something?

If it's really needed in 2.0+, then for short-term and for only this issue, I think we can introduce one more attribute like "secondNumber" in the telephony-call-ended message, which is sent when the *only connection* ends. 

Or, we can send one more "telephony-call-ended" message along with that for the 1st call. We have no idea when the 2nd call is actually ended. We only know for sure when the 1st call ends, so does the 2nd. 

Is it okay for gaia?

For long-term, Gabriele might have known that, I've been thinking about moving the current cdma call waiting API to what we have for gsm. Current API works well on answering and switching calls, but as it provides less information, we've seen some problems:
1) When user rejects the second call, the API and TelephonyService have no idea. That also leads other components monitoring TelephonyService, e.g. BT, to some trouble. If we change the API, then when user rejects the cdma incoming call, BT module as well as TelephonyService knows that. And it's TelephonyService's responsibility to make sure API's behaviour, like CDMA 3-way call.
2) The call log is another example. Gecko could do the best guess to provide the most precise information such as call duration if modem doesn't provide customized message.
3) Not being able to provide more call states. If partners want to know specific states of the second call as they require in cdma 3-way call (it's possible with customization), the current API isn't flexible enough.

I would be happier to see this bug being viewed as a new feature request, so that we could review and work on the long-term proposal properly.

Comment 11

[Jason Smith [:jsmith]]

This is a clear cert blocker in the 2.0 timeframe for CDMA, so this needs to stay as a blocker for 2.0.

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10) 
> If it's really needed in 2.0+, then for short-term and for only this issue,
> I think we can introduce one more attribute like "secondNumber" in the
> telephony-call-ended message, which is sent when the *only connection* ends. 
> 
> Or, we can send one more "telephony-call-ended" message along with that for
> the 1st call. We have no idea when the 2nd call is actually ended. We only
> know for sure when the 1st call ends, so does the 2nd. 
> 
> Is it okay for gaia?

Both are fine, the second one has the advantage of requiring no gaia work at all :)

Comment 13

[Doug Sherk (:drs) (inactive)]

Moving to RIL. Is this something that Tamara could work on with mentorship?

Comment 14

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

(In reply to Doug Sherk (:drs) from comment #13)
> Moving to RIL. Is this something that Tamara could work on with mentorship?

I think this is good for her. I could be the mentor, of course :)

Comment 15

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to Doug Sherk (:drs) from comment #13)
> > Moving to RIL. Is this something that Tamara could work on with mentorship?
> 
> I think this is good for her. I could be the mentor, of course :)

Hey Tamara,

Are you interested in taking this bug? I will be happy to be the mentor. :)

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

We need 2.0+ to be cleared before end of sprint6.

Comment 17

[Tamara Hills [:thills]]

Hi Hsin-Yi, I would like to work on this.  Will start taking look through comment #10.  In the meantime, would be good to know what kind of env I would need in order to repro this as I don't have a CDMA phone and I'm not sure about how to set it up for emulation.

Thanks!

Comment 18

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

(In reply to Tamara Hills [:thills] from comment #17)
> Hi Hsin-Yi, I would like to work on this.  Will start taking look through
> comment #10.  In the meantime, would be good to know what kind of env I
> would need in order to repro this as I don't have a CDMA phone and I'm not
> sure about how to set it up for emulation.
> 
> Thanks!

Hi Tamara,

The CDMA phone I use for Fxos development is wasabi. I am not sure how you could get that. We don't have many in fact. Sadly, emulator for cdma call waiting support isn't finished so you are not able to rely on the emulator. I think you could still study the code and provide your patch. I could test it for you.

Comment 19

[Tamara Hills [:thills]]

Thanks Hsin-Yi,

We are trying to get a few wasabi devices.  I will try and look through the code for this one to get started.

Comment 20

[Tamara Hills [:thills]]

Hi Hsin-Yi, 
I was reading through the code and it looks like there is this parent/child relationship for the CDMA call and the secondNumber is now a secondID in TelephonyCall now pointing to the index of the second incoming call.  

I'm trying to find a spot to grab that second number and put it into the System Message for the telephony-call-ended event.  I'm seeing some info in the IPCCdmaWaitingCallData structure, but not sure how I can access.  Can I access the second number from the secondId?

I'm thinking what would help (since I don't have a device yet) is either a class diagram or a debug trace of the steps to repro.  

I'll keep looking and maybe have some clearer questions for you. :)

Thanks,
-tamara

Comment 21

[Tamara Hills [:thills]]

Attachment: 1029721

Hi Hsin-Yi,

Would something like the attached work to get the second number up to Gaia on a CDMA call hangup?  Wondering if the data is still available at this point.

Comment 22

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

Comment on attachment 8453494 [details] [diff] [review]
1029721

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

Hi Tamara,

CDMA telephony API isn't firmed I would say. :( So, we have two different designs for Cdma call waiting and Cdma 3-way calling. The code you touched is the part of cdma 3way calling, not cdma call waiting. To resolve this issue, you will need to create a call object on TelephonyService in [1]. And then do what we need when receiving |notifyCallDisconnected| for the 1st call.

[1] dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#878

Comment 23

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
> Comment on attachment 8453494 [details] [diff] [review]
> 1029721
> 
> Review of attachment 8453494 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Tamara,
> 

Okay, maybe I should ask this first:
Which is the solution on comment 12 you are going to take? Are you going to send an extended system message including secondNumber or send a system message per call? Seems the former?  Just want to confirm! 

> CDMA telephony API isn't firmed I would say. :( So, we have two different
> designs for Cdma call waiting and Cdma 3-way calling. The code you touched
> is the part of cdma 3way calling, not cdma call waiting. To resolve this
> issue, you will need to create a call object on TelephonyService in [1]. And
> then do what we need when receiving |notifyCallDisconnected| for the 1st
> call.
> 
> [1]
> dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.
> js?from=TelephonyService.js#878

Comment 24

[Tamara Hills [:thills]]

Hi Hsin-Yi,
Yes, that was the approach I was looking at.

Comment 25

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

Comment on attachment 8453494 [details] [diff] [review]
1029721

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

Tamara,

Based on your goal comment 24, below is my comments in detail. Thank you!

::: dom/telephony/gonk/TelephonyService.js
@@ +730,5 @@
>        duration: duration,
>        direction: aCall.isOutgoing ? "outgoing" : "incoming"
>      };
> +
> +    let secondCall = this._currentCalls[aClientId][CDMA_SECOND_CALL_INDEX];

Let's not do this way. If doing so, then in cdma 3way calling scenario, gaia will receive two telephony-call-ended system messages both with 'secondNumber' attribute.

Due to the difference b/w the design of cdma 3way calling and cdma call waiting, I'd suggest in this bug we still keep the implementation for these two separate. We need a new variable '_cdmaWaitingCallNumber,'  assign a valid value in notifyCdmaCallWaiting(), and properly clear it in notifyCallDisconnected().

Comment 26

[Tamara Hills [:thills]]

Attachment: WIP - Gecko only

Hi Hsin-Yi,

Is the attached in the right direction?  I wasn't sure if you meant a global in TelephonyService.js is ok or not.

I had another question about the calling number presentation (the privacy bits).  It looks like Gecko notifies Gaia what it can present for an incoming call, but the telephony-call-ended event does not look like it includes those flags about what can be presented.  I couldn't find a place in Gaia where this is done (but I might be missing something).  

Thank you,

-tamara

Comment 27

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

Comment on attachment 8454265 [details] [diff] [review]
WIP - Gecko only

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

Yes, this is what I was talking about. It looks really good!

Comment 28

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

(In reply to Tamara Hills [:thills] from comment #26)
> Created attachment 8454265 [details] [diff] [review]
> WIP - Gecko only
> 
> Hi Hsin-Yi,
> 
> Is the attached in the right direction?  I wasn't sure if you meant a global
> in TelephonyService.js is ok or not.
> 

It looks great, thank you.

> I had another question about the calling number presentation (the privacy
> bits).  It looks like Gecko notifies Gaia what it can present for an
> incoming call, but the telephony-call-ended event does not look like it
> includes those flags about what can be presented.  I couldn't find a place
> in Gaia where this is done (but I might be missing something).  
> 

Oh, that's because the Gaia part isn't ready yet, see Bug 980598. IIRC, our partner is working on the gaia but.

Regarding telephony-call-ended system messages, I am not aware of a feature request about the call id presentation on call log. The information is only exposed to TelephonyCall now.

> Thank you,
> 
> -tamara

Comment 29

[Tamara Hills [:thills]]

Attachment: Gaia portion Pull request

Comment 30

[Tamara Hills [:thills]]

Attachment: Gecko Portion of Patch

Comment 31

[Tamara Hills [:thills]]

Attachment: Gaia Portion of Pull Request

Hi Anthony,

Do you think you can take a look at the callEnded event changes I made to handle this blocker?  I've tested some of the paths, but don't have a device to test the true CDMA (yet).

Comment 32

[Tamara Hills [:thills]]

Hi Anshul,

Do you think you'd be able to help test this patch?  It's a two-part patch (both gaia and gecko).  I don't have my device yet, but will be getting one soon.

Thanks,

-tamara

Comment 33

[Anshul]

Tamara, we don't have the corresponding gecko changes done yet so won't be able to verify the patch just yet.

Comment 34

[bhavana bajaj [:bajaj]]

(In reply to Anshul from comment #33)
> Tamara, we don't have the corresponding gecko changes done yet so won't be
> able to verify the patch just yet.

Anshul can you please help with the bug # where the gecko changes are happening  ?

Comment 35

[Anshul]

Bhavana, I was talking about the proprietary changes that need to happen on our side.

Comment 36

[Ken Chang[:kenkai]]

Tarama, do you need any help in Gecko patch? Since this week is the last week of sprint 6 of 2.0, we are supposed to fix all blockers for 2.0 before this week....:-(.

Comment 37

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

I could help verify the patches with wasabi.

Comment 38

[Tamara Hills [:thills]]

(In reply to Ken Chang[:ken] from comment #36)
> Tarama, do you need any help in Gecko patch? Since this week is the last
> week of sprint 6 of 2.0, we are supposed to fix all blockers for 2.0 before
> this week....:-(.

Hi Ken,

I'm not sure what Gecko patch is being referred to here.  The gecko patch for this bug is attached.  It's a two part patch.  One gecko and one gaia patch.

If there is something else required, then I'm not aware of that. We need to get it tested on Wasabi.  I'm getting a device, but won't get it in time to test this.  It sounds like Hsin-Yi can help test on the Wasabi.

Comment 39

[Tamara Hills [:thills]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #37)
> I could help verify the patches with wasabi.

Hi Hsin-Yi,

Yes, that would be great if you can help test.  Let me know if you have any question.

thanks

-tamara

Comment 40

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

Comment on attachment 8454634 [details] [diff] [review]
Gecko Portion of Patch

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

Tamara's patches (gecko + gaia) work really good. There's a call log for the cdma waiting call. :)

::: dom/telephony/gonk/TelephonyService.js
@@ +876,5 @@
>        this.notifyCallDisconnected(aClientId, call);
>      }
> +
> +    let presentNumber = aCall.numberPresentation != null ?
> +          aCall.numberPresentation : nsITelephonyService.CALL_PRESENTATION_ALLOWED;

I think we are fine to just have |this._cdmaCallWaitingNumber = aCall.number|. AFAIK, modem/rild would take care of the number string based on the value of number presentation.

Comment 41

[Tamara Hills [:thills]]

Attachment: Updated Gecko Patch

Hi Hsin-Yi,
thanks for testing that out.  As per your comment, I removed some of the checks for that CLID blocking (as it should be handled by RILD/modem).

Do you mind testing it again just to be sure?

Thanks,
-tamara

Comment 42

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

Comment on attachment 8456276 [details] [diff] [review]
Updated Gecko Patch

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

It works well, thank you!

::: dom/telephony/gonk/TelephonyService.js
@@ +108,5 @@
>    this._numClients = gRadioInterfaceLayer.numRadioInterfaces;
>    this._listeners = [];
>    this._currentCalls = {};
>  
> +  this._cdmaWaitingCallNumber = null;

You use _cdmaCallWaitingNumber elsewhere. I am fine with either name as long as we use it consistently.

Comment 43

[Tamara Hills [:thills]]

Attachment: Updated Gecko Patch -- _cdmaCallWaitingNumber

Hi Hsin-Yi,
Good catch.  Thanks. Do you mind retesting?

Thanks,
-tamara

Comment 44

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

Comment on attachment 8456687 [details] [diff] [review]
Updated Gecko Patch -- _cdmaCallWaitingNumber

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

Looks really nice so I gave r+ directly :P

Comment 45

[Tamara Hills [:thills]]

Attachment: Gaia Pull Request for feedback

Anthony,

This pull request has changes in dialer.js as well as new tests added in dialer_test.js.

Would like your feedback on this.

Thank you,

-tamara

Comment 46

[Anthony Ricaud (:rik)]

Comment on attachment 8456752 [details] [review]
Gaia Pull Request for feedback

Redirecting to Gabriele since he has more experience with CDMA.

Tamara: You'll need new unit tests for this.

Comment 47

[Anthony Ricaud (:rik)]

Oops, you did add unit tests, I just didn't refresh the github page.

Comment 48

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8456752 [details] [review]
Gaia Pull Request for feedback

f- because the code handling the high-priority wakelock is incorrect. The rest looks good to me (including the tests). Once you fix the high-priority wakelock issue it would be nice if you could add a test that ensures that:

- in case the secondNumber field is present the wake-lock is unlocked only once
- the wakelock is not unlocked after the first call is stored in the DB but only after both are

Comment 49

[Tamara Hills [:thills]]

Need checkin in the Gecko portion only.  r+ for the Gecko portion is in the Comment 44.

Comment 50

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

https://hg.mozilla.org/integration/b2g-inbound/rev/88ad2e93bce6 Great job, Tamara!

leave-open for the gaia part.

Comment 51

[Tamara Hills [:thills]]

Comment on attachment 8456752 [details] [review]
Gaia Pull Request for feedback

Hi Gabriele,

I spent bunch of time with Anthony on this today.  We were able to do it without the promises.

Would you be able to take look?  

Thanks,

-tamara

Comment 52

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8456752 [details] [review]
Gaia Pull Request for feedback

LGTM except for a change in where the high-priority wakelock is released. Feel free to land once that's been addressed and covered in the unit-tests as I described on GitHub.

Comment 53

[Tamara Hills [:thills]]

Hi Hsin-Yi,

The Gaia portion has been updated and we have the review done.

If you get a chance can you try out on the device?

https://github.com/mozilla-b2g/gaia/pull/21638/

Thank you!

-tamara

Comment 54

[Tamara Hills [:thills]]

(In reply to Gabriele Svelto [:gsvelto] from comment #52)
> Comment on attachment 8456752 [details] [review]
> Gaia Pull Request for feedback
> 
> LGTM except for a change in where the high-priority wakelock is released.
> Feel free to land once that's been addressed and covered in the unit-tests
> as I described on GitHub.

I addressed the wakelock call and we Updated tests to make sure that the lock is released in teh cdma case only after the second call has completed.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88ad2e93bce6

Comment 56

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Gecko patch was landed, I changed the component to Dialer since next step is to get gaia patch landed as well. Hsinyi will play with the Gaia patch and provide feedback.

Comment 57

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

(In reply to Tamara Hills [:thills] from comment #53)
> Hi Hsin-Yi,
> 
> The Gaia portion has been updated and we have the review done.
> 
> If you get a chance can you try out on the device?
> 
> https://github.com/mozilla-b2g/gaia/pull/21638/
> 
> Thank you!
> 
> -tamara

Work like a charm!

Comment 58

[Anthony Ricaud (:rik)]

Tamara is on a plane so I merged it for her.

https://github.com/mozilla-b2g/gaia/commit/858cb715524502bcaa15b5e5060c6ad67eec890e

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d1d5ae5d8a6
v2.0: https://github.com/mozilla-b2g/gaia/commit/bd2303de0be9bad0f5a3a1339c5a48c808c90e3a

Comment 60

[[:SalvadorR]]

This will need a new test case to be covered.

Comment 61

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case has been added to moztrap:

https://moztrap.mozilla.org/manage/case/14386/