Closed Bug 1029721 Opened 5 years ago Closed 5 years ago

CDMA call waiting call is not logged in call history

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: anshulj, Assigned: thills)

References

()

Details

(Whiteboard: [planned-sprint])

Attachments

(2 files, 6 obsolete files)

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.
Wilfred - Are there any production devices shipping in 2.0 with CDMA support?
Flags: needinfo?(wmathanaraj)
(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.
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.
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(wmathanaraj)
I can help test the patch.
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.
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
(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?
Flags: needinfo?(htsai)
See comment 6 and comment 7.
blocking-b2g: 2.0+ → 2.0?
(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?
Flags: needinfo?(wmathanaraj)
(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.
Flags: needinfo?(htsai)
This is a clear cert blocker in the 2.0 timeframe for CDMA, so this needs to stay as a blocker for 2.0.
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(wmathanaraj)
(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 :)
Moving to RIL. Is this something that Tamara could work on with mentorship?
Component: Gaia::Dialer → RIL
Whiteboard: [planned-sprint]
(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 :)
(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. :)
Flags: needinfo?(thills)
We need 2.0+ to be cleared before end of sprint6.
Target Milestone: --- → 2.0 S6 (18july)
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!
Assignee: nobody → thills
Flags: needinfo?(htsai)
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(htsai)
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.
Flags: needinfo?(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
Flags: needinfo?(htsai)
Attached patch 1029721 (obsolete) — Splinter Review
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 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
(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
Flags: needinfo?(htsai)
Hi Hsin-Yi,
Yes, that was the approach I was looking at.
Flags: needinfo?(htsai)
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().
Flags: needinfo?(htsai)
Attached patch WIP - Gecko only (obsolete) — Splinter Review
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
Attachment #8454265 - Flags: feedback?(htsai)
Flags: needinfo?(htsai)
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!
Attachment #8454265 - Flags: feedback?(htsai) → feedback+
(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
Flags: needinfo?(htsai)
Attached file Gaia portion Pull request (obsolete) —
Attachment #8453494 - Attachment is obsolete: true
Attachment #8454265 - Attachment is obsolete: true
Attached patch Gecko Portion of Patch (obsolete) — Splinter Review
Attached file Gaia Portion of Pull Request (obsolete) —
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).
Attachment #8454633 - Attachment is obsolete: true
Attachment #8455338 - Flags: review?(anthony)
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
Flags: needinfo?(anshulj)
Tamara, we don't have the corresponding gecko changes done yet so won't be able to verify the patch just yet.
Flags: needinfo?(anshulj)
(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  ?
Flags: needinfo?(anshulj)
Bhavana, I was talking about the proprietary changes that need to happen on our side.
Flags: needinfo?(anshulj)
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....:-(.
I could help verify the patches with wasabi.
(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.
(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 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.
Attached patch Updated Gecko Patch (obsolete) — Splinter Review
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
Attachment #8454634 - Attachment is obsolete: true
Attachment #8456276 - Flags: feedback?(htsai)
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.
Attachment #8456276 - Flags: feedback?(htsai) → feedback+
Hi Hsin-Yi,
Good catch.  Thanks. Do you mind retesting?

Thanks,
-tamara
Attachment #8456276 - Attachment is obsolete: true
Attachment #8456687 - Flags: feedback?(htsai)
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
Attachment #8456687 - Flags: feedback?(htsai) → review+
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
Attachment #8455338 - Attachment is obsolete: true
Attachment #8455338 - Flags: review?(anthony)
Attachment #8456752 - Flags: feedback?(anthony)
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.
Attachment #8456752 - Flags: feedback?(anthony) → feedback?(gsvelto)
Oops, you did add unit tests, I just didn't refresh the github page.
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
Attachment #8456752 - Flags: feedback?(gsvelto) → feedback-
Need checkin in the Gecko portion only.  r+ for the Gecko portion is in the Comment 44.
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/88ad2e93bce6 Great job, Tamara!

leave-open for the gaia part.
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
Attachment #8456752 - Flags: review?(gsvelto)
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.
Attachment #8456752 - Flags: review?(gsvelto) → review+
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
Flags: needinfo?(htsai)
(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.
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.
Component: RIL → Gaia::Dialer
(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!
Flags: needinfo?(htsai)
Tamara is on a plane so I merged it for her.

https://github.com/mozilla-b2g/gaia/commit/858cb715524502bcaa15b5e5060c6ad67eec890e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This will need a new test case to be covered.
Flags: needinfo?(ktucker)
Flags: in-moztrap?(srapanan)
Test case has been added to moztrap:

https://moztrap.mozilla.org/manage/case/14386/
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(srapanan)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.