Closed Bug 882980 Opened 11 years ago Closed 11 years ago

[Gaia] UI support for CDMA call waiting

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+)

RESOLVED FIXED
blocking-b2g koi+

People

(Reporter: anshulj, Assigned: gsvelto)

References

Details

(Whiteboard: [UX ETA:8/9], [FT:RIL], [Sprint:3], [u=commsapps-user c=dialer p=3 s=v1.2-features-sprint-3])

Attachments

(6 files, 14 obsolete files)

358 bytes, text/html
Details
449.57 KB, text/plain
Details
155.46 KB, image/jpeg
Details
923.78 KB, application/pdf
Details
25.41 KB, patch
Details | Diff | Splinter Review
1.75 MB, application/zip
Details
Display a generic UI if the CDMA call waiting call is accepted. This is because user doesn’t really know which caller party he/she is talking to.
o   To switch calls, currently the user needs to click on any of the two calls. But if we show generic message there won’t be any calls displayed to switch. So need a button or any other way to trigger the call switch.
No longer blocks: b2g-ril-cdma
Blocks: 890807
Assignee: nobody → gsvelto
Summary: UI support for CDMA call waiting → [Gaia] UI support for CDMA call waiting
blocking-b2g: --- → koi+
QA Contact: echu
Whiteboard: [UX ETA:8/9]
I've got a partial implementation of this using the proposed interface from bug 882980 comment 34. Besides not being able to test this yet the reason why I left it incomplete is that it's not clear to me what should be shown on the switch screen. Do we have a more detailed description or even a mock up for me to see?
Wrong bug in my comment above, I meant bug 822210 comment 34.
OK, so after studying the current behavior a little bit better and considering the limitations we've highlighted in bug 822210 I've come up with the following design which leverages as much of the existing code and UI as possible while remaining consistent with the existing interface:

- When receiving an incoming call we display the same UI as with GSM with the second number showing up and the answer / dismiss buttons.

- If the user hits the dismiss button we'll revert to the single-call UI but we won't actually do anything in the background, that's because we can't dismiss the second call, just hide it.

- If the user hits the answer button the second call will be answered but we'll keep showing a single bar instead of two and the number will be replaced with some relevant text (such as "switch calls"). The point here is that we can't know which of the two numbers we're talking to (or even which one is connected) so whilst we can keep the existing interface we must reflect the limitations we have. The hang-up button hangs up both calls which is consistent with the fact that we won't show them both in separate lines.
Hi Hsin-Yi, I need a little clarification regarding the call state: when receiving a second call and invoking Telephony.oncallschanged is the |state| field of the |call| parameter still set to 'connected'? I would assume that should be the case since in bug 822210 we discussed about only adding |secondNumber| to reflect the waiting call.

There's another corner case I was curious about: if we've got a single call in CDMA mode is it possible to put it on hold? If yes what happens if I've got a single call on hold and I get a second call?
Flags: needinfo?(htsai)
This is a basic WIP patch that adjusts some of the code paths to accommodate for the presence of a call waiting in CDMA mode. It's still missing some important bits and pieces (such as updating the visible state of the call after answering the incoming one) but contains the basic scaffolding for it nevertheless.
(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> Hi Hsin-Yi, I need a little clarification regarding the call state: when
> receiving a second call and invoking Telephony.oncallschanged is the |state|
> field of the |call| parameter still set to 'connected'? 

Yup, the state is still connected, just as what the network reports.

> I would assume that
> should be the case since in bug 822210 we discussed about only adding
> |secondNumber| to reflect the waiting call.
> 
> There's another corner case I was curious about: if we've got a single call
> in CDMA mode is it possible to put it on hold? If yes what happens if I've
> got a single call on hold and I get a second call?

In spec, I don't see CDMA supports this case, I mean, one call on hold, then 2nd call coming. I also had a simple test with the local network provider. Once you have a call onhold, then the network will automatically rejects the 2nd call.

Hope it helps :)
Flags: needinfo?(htsai)
Whiteboard: [UX ETA:8/9] → [UX ETA:8/9], [FT:RIL], [Sprint:3]
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> Yup, the state is still connected, just as what the network reports.

Perfect! That means I can effectively treat it as a specialization of the single-call code plus some adjustments to the code used to handle GSM call waiting.

> In spec, I don't see CDMA supports this case, I mean, one call on hold, then
> 2nd call coming. I also had a simple test with the local network provider.
> Once you have a call onhold, then the network will automatically rejects the
> 2nd call.

Excellent, that makes it even simpler \o/

> Hope it helps :)

Yes, thank you very much!
This patch should contain all what's required to implement CDMA call waiting. I've tried to keep the existing code-paths as unchanged as possible. The call-waiting code-paths have been modified to take into account the situation where there's only one call but the |secondNumber| field shows up on it which effectively should be treated as a call waiting situation. Operations on multiple calls (such as hold-and-answer) have been adjusted accordingly. All other operations have been left untouched as CDMA call waiting behaves almost entirely as if we had a single call.

From this perspective the UI changes are absolutely minimal. When a second incoming call is detected it can be dismissed which is treated as with GSM calls. If the second call is answered instead of showing up two calls as in GSM mode we show a single one and we remove the number on it and replace it with the "Switch calls" text (for which I've added a string). Once this mode is entered the call behaves exactly as if it were a single call except that tapping it doesn't put it on hold but switches between the two calls instead.

Unfortunately lacking a device (and a network!) to test this on I cannot personally verify the functionality. Hsin-Yi, can you try it out for me using your platform patches? This should apply on top of a gaia/master clone, alternatively you can use the PR I'll post shortly.
Attachment #784419 - Attachment is obsolete: true
Attachment #784997 - Flags: feedback?(etienne)
Flags: needinfo?(htsai)
Gabriele,
I am happy to help out but unfortunately I am off office now, and I am going to head for Paris, so not able to do it for you until I am back from the Comms workweek XD 
Asking for another RIL developer, Aknow's help. :)

Aknow,
Would you mind doing me a favor for testing Gabriele's gaia patch with my patch for bug 822210?
Flags: needinfo?(htsai) → needinfo?(szchen)
Comment on attachment 784997 [details] [diff] [review]
[PATCH] Add support for CDMA call waiting

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

On a more general note: we're not going to get through the CDMA + conference call support without unit-test coverage for oncall.js :)

::: apps/communications/dialer/js/handled_call.js
@@ +101,4 @@
>  
>  HandledCall.prototype.updateCallNumber = function hc_updateCallNumber() {
>    var number = this.call.number;
> +  var secondNumber = this.call.secondNumber;

who will call |updateCallNumber| when a call is waiting?
Attachment #784997 - Flags: feedback?(etienne)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> Gabriele,
> I am happy to help out but unfortunately I am off office now, and I am going
> to head for Paris, so not able to do it for you until I am back from the
> Comms workweek XD 
> Asking for another RIL developer, Aknow's help. :)
> 
> Aknow,
> Would you mind doing me a favor for testing Gabriele's gaia patch with my
> patch for bug 822210?

Hsinyi,
Could you tell me the commit which your patch in bug 822210 is based on?
(1) hg or git
(2) hash, date/time and the commit message.

Therefore I could easily apply your patch without resolving the conflict.
Flags: needinfo?(szchen) → needinfo?(htsai)
I'll provide updated patches later today. Sorry for any inconvenience.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> I'll provide updated patches later today. Sorry for any inconvenience.

Gecko working branch: https://github.com/hsinyi/releases-mozilla-central/tree/bugzilla/822210/WIP
Just roughly test the patch on wasabi

1. When 2nd call is incoming. I can see the phone number
  *if screen is off, it doesn't turn on automatically
  There are 3 choice
  i) hangup: *click the button but no effect
  ii) hold: answer the 2nd call, ok. accept 2nd, hold 1st, voice is ok too.
  iii) skip: ok

2.  after 1(ii) hold. label shows 'switch calls', ok
3.  tap 'switch calls', voice switch. ok
4a. hangup the ongoing call from remote party
  *screen hang
4b. hangup the hold call from remote party. ok
  i) hangup the remain call from wasabi (FxOS).
    *screen hang
Attached file log.txt (obsolete) —
I got some error during the test. Please have a check. Thanks.
It's recorded by default debug setting.
(In reply to Szu-Yu Chen [:aknow] from comment #15)
> Created attachment 786801 [details]
> log.txt
> 
> I got some error during the test. Please have a check. Thanks.
> It's recorded by default debug setting.

(In reply to Szu-Yu Chen [:aknow] from comment #14)
> Just roughly test the patch on wasabi
> 
> 1. When 2nd call is incoming. I can see the phone number
>   *if screen is off, it doesn't turn on automatically
>   There are 3 choice
>   i) hangup: *click the button but no effect
>   ii) hold: answer the 2nd call, ok. accept 2nd, hold 1st, voice is ok too.
>   iii) skip: ok
> 
> 2.  after 1(ii) hold. label shows 'switch calls', ok
> 3.  tap 'switch calls', voice switch. ok
> 4a. hangup the ongoing call from remote party
>   *screen hang
> 4b. hangup the hold call from remote party. ok
>   i) hangup the remain call from wasabi (FxOS).
>     *screen hang

Thank you sooooo much, Aknow!  :)
Attached file log_ril.txt (obsolete) —
log with ril debug
Thank you very much for your testing Szu-Yu, it's really appreciated. I'll try to fix the remaining issues and then attach a revised patch.
Pointer to Github pull-request
Attachment #786917 - Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11401 → [PULL REQUEST] Add support for CDMA call waiting
(In reply to Szu-Yu Chen [:aknow] from comment #14)
> 1. When 2nd call is incoming. I can see the phone number
>   *if screen is off, it doesn't turn on automatically

Yes, this expected behavior. Can you hear a tone when the incoming call comes in? That's what we use to notify the user that there's a second call waiting.

>   There are 3 choice
>   i) hangup: *click the button but no effect

This is a bit surprising. What is the expected behavior? Should the hangup button drop both calls or just the incoming one? I have done some changes that might fix this, but more on this later.

>   ii) hold: answer the 2nd call, ok. accept 2nd, hold 1st, voice is ok too.
>   iii) skip: ok
> 
> 2.  after 1(ii) hold. label shows 'switch calls', ok
> 3.  tap 'switch calls', voice switch. ok
> 4a. hangup the ongoing call from remote party
>   *screen hang

I think I've identified the problem that's causing this. My previous patch did not detect exiting from the CDMA call waiting state correctly. This should be fixed in this patch.

>   i) hangup the remain call from wasabi (FxOS).
>     *screen hang

This should be the same issue you've seen in 4a and should also be fixed in this patch. Could you please re-test it for us? Thank you again for your feedback!
Attachment #784997 - Attachment is obsolete: true
Hi Gabriele,

(In reply to Gabriele Svelto [:gsvelto] from comment #20)
> Created attachment 786933 [details] [diff] [review]
> [PATCH v2] Add support for CDMA call waiting
> 
> (In reply to Szu-Yu Chen [:aknow] from comment #14)
> > 1. When 2nd call is incoming. I can see the phone number
> >   *if screen is off, it doesn't turn on automatically
> 
> Yes, this expected behavior. Can you hear a tone when the incoming call
> comes in? That's what we use to notify the user that there's a second call
> waiting.

I tried this on v1.1hd GSM phone, screen will be turned on automatically with ring tone. And then I tried it again with CDMA android phone, screen will be on as well when 2nd call is incoming. Could you check again if screen keeps off is expected behavior or not?
Flags: needinfo?(gsvelto)
correct my last comment, 2nd call will wake up the screen with audio sound heard by user from receiver, not ring tone.
Comment on attachment 786933 [details] [diff] [review]
[PATCH v2] Add support for CDMA call waiting

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

::: apps/communications/dialer/js/oncall.js
@@ +287,5 @@
>      });
>  
> +    // Handle CDMA call waiting
> +    if ((handledCalls.length == 1) &&
> +        (handledCalls[0].state == 'connected') &&

handledCalls[0].call.state

@@ +288,5 @@
>  
> +    // Handle CDMA call waiting
> +    if ((handledCalls.length == 1) &&
> +        (handledCalls[0].state == 'connected') &&
> +        (handledCalls[0].secondNumber && !cdmaCallWaiting)) {

handledCalls[0].call.secondNumber
(In reply to Gabriele Svelto [:gsvelto] from comment #20)
> Created attachment 786933 [details] [diff] [review]
> [PATCH v2] Add support for CDMA call waiting
> 
> (In reply to Szu-Yu Chen [:aknow] from comment #14)
> > 1. When 2nd call is incoming. I can see the phone number
> >   *if screen is off, it doesn't turn on automatically
> 
> Yes, this expected behavior. Can you hear a tone when the incoming call
> comes in? That's what we use to notify the user that there's a second call
> waiting.
> 
> >   There are 3 choice
> >   i) hangup: *click the button but no effect
> 
> This is a bit surprising. What is the expected behavior? Should the hangup
> button drop both calls or just the incoming one? I have done some changes
> that might fix this, but more on this later.

Same in this patch.

> >   ii) hold: answer the 2nd call, ok. accept 2nd, hold 1st, voice is ok too.
> >   iii) skip: ok
> > 
> > 2.  after 1(ii) hold. label shows 'switch calls', ok
> > 3.  tap 'switch calls', voice switch. ok
> > 4a. hangup the ongoing call from remote party
> >   *screen hang
> 
> I think I've identified the problem that's causing this. My previous patch
> did not detect exiting from the CDMA call waiting state correctly. This
> should be fixed in this patch.

fixed

> >   i) hangup the remain call from wasabi (FxOS).
> >     *screen hang
> 
> This should be the same issue you've seen in 4a and should also be fixed in
> this patch. Could you please re-test it for us? Thank you again for your
> feedback!

2nd call is incoming. hold and answer.
hang up one of remote party
dial to FxOS again => expect a call waiting pop again but no
Refreshed patch with fixes for the issues highlighted in comment 23.
Attachment #786933 - Attachment is obsolete: true
Hi Gabriele,

Here I summarize my test results of latest patch Aknow passed to me.
Passed items
1. Answer 2nd call 
2. Reject 2nd incoming call
3. Switch call 
Problems:
1. 2nd call cannot wake up device as comment 22
2. Unable to pick up 3rd call, no UI to answer it at all.(Same as comment 23?) Steps:
   a. Answered two calls already.
   b. One end line terminate the call, brings DUT and another line back on connection again.
   c. MT call to device with another phone.
3. An audio sound indicating there are call waiting still can be heard while there is only one call left. Steps:
   a. Answered two calls already.
   b. One end line terminate the call, brings DUT and another line back on connection again.
   c. Listen to DUT receiver, there is still call waiting "dodo" regularly sounds.

Thanks.
(In reply to echu from comment #26)
> Here I summarize my test results of latest patch Aknow passed to me.

First of all thank you for your help :)

> Problems:
> 1. 2nd call cannot wake up device as comment 22

I'll ask :etienne about this, apparently there was a bug on this topic where this behavior was decided so I want to dig that out.

> 2. Unable to pick up 3rd call, no UI to answer it at all.(Same as comment
> 23?) Steps:
>    a. Answered two calls already.
>    b. One end line terminate the call, brings DUT and another line back on
> connection again.
>    c. MT call to device with another phone.
> 3. An audio sound indicating there are call waiting still can be heard while
> there is only one call left. Steps:
>    a. Answered two calls already.
>    b. One end line terminate the call, brings DUT and another line back on
> connection again.
>    c. Listen to DUT receiver, there is still call waiting "dodo" regularly
> sounds.

I've just talked to Hsin-Yi about this topic and I didn't know that this situation was possible at all so I hadn't covered it. I'll try to come up with an updated patch ASAP.
Flags: needinfo?(gsvelto)
Fourth try, this patch is significantly different from the previous one:

- I've removed the |cdmaCallWaiting| field I was using to detect when we were entering CDMA call waiting mode and replaced it with a function; this way we won't have redundant state into the CallHandler object and we should be able to take multiple incoming calls making case 2 in comment 26 work.

- I've factorized the code used to play waiting tones and I've added code to clear it up manually when in CDMA call waiting mode, this should fix the issue for case 3 in comment 26 where the wait tone would keep playing non-stop.

- The hangup-and-answer sequence should work now fixing the issue :aknow had found in comment 14 case 1. i. I was calling telephony.answer() instead of telephony.active.answer() in that codepath which wouldn't work obviously.

I've also updated the pull-request in attachment 786917 [details]
Attachment #787420 - Attachment is obsolete: true
Rebased patch as the previous one didn't apply cleanly anymore; I've also updated the pull request. Enpei could you test this for me again? Hopefully this should be the last iteration of this patch.
Attachment #787506 - Attachment is obsolete: true
Flags: needinfo?(echu)
(In reply to Gabriele Svelto [:gsvelto] from comment #29)
> Created attachment 789577 [details] [diff] [review]
> [PATCH v5] Add support for CDMA call waiting
> 
> Rebased patch as the previous one didn't apply cleanly anymore; I've also
> updated the pull request. Enpei could you test this for me again? Hopefully
> this should be the last iteration of this patch.

Gabriele, the patch is still not apply cleanly. oncall.js has been removed. Could you please provide a rebased patch again? I checked the pull request as well but seems the last update of the PR is 13 days ago, so I am not sure if it's up-to-date.
Flags: needinfo?(echu)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #30)
> Gabriele, the patch is still not apply cleanly. oncall.js has been removed.

Yes, I just realized Etienne's refactoring has landed.

> Could you please provide a rebased patch again? I checked the pull request
> as well but seems the last update of the PR is 13 days ago, so I am not sure
> if it's up-to-date.

I've rebased the patch again and checked the PR again, it might be that I had forgot to update it. Fortunately my changes applied correctly at the first try as Etienne had only moved the code around but did not make any functional changes. I really hope it works this time :)
Attachment #789577 - Attachment is obsolete: true
BTW unit-tests were added to cover the regular GSM call waiting functionality so I'll try to add further tests to cover CDMA call waiting too before asking for review.
Hi Gabriele,

With patch v6 applied, I don't see any screen change when the 2nd call comes. I added log message in dialer.js and calls_handler.js to make sure that dialer did receive 'callschanged' event with secondNumber assigned. But no screen changes. No hold/reject buttons showing up. 

After two remote parties are released, the call screen is hung there.
No buttons popping up when 2nd call incoming. 
After the two remote parties are released, the call screen is hung there.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Created attachment 791218 [details]
> screenshot for comment 33
> 
> No buttons popping up when 2nd call incoming. 
> After the two remote parties are released, the call screen is hung there.

Even I pressed the red end-call button.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #33)
> With patch v6 applied, I don't see any screen change when the 2nd call
> comes. I added log message in dialer.js and calls_handler.js to make sure
> that dialer did receive 'callschanged' event with secondNumber assigned. But
> no screen changes. No hold/reject buttons showing up. 
> 
> After two remote parties are released, the call screen is hung there.

Thanks for the testing Hsin-Yi, I made a silly silly mistake and I'm sorry to have wasted your time; my previous patch contained this to detect CDMA call waiting:

function cdmaCallWaiting() {
  return ((telephony.length == 1) &&                   <-- silly mistake here
          (telephony.calls[0].state == 'connected') &&
          telephony.calls[0].secondNumber);
}

I've fixed it and uploaded a new patch. If this version doesn't work still I'll write some unit tests first before going forward.
Attachment #786801 - Attachment is obsolete: true
Attachment #786833 - Attachment is obsolete: true
Attachment #790107 - Attachment is obsolete: true
(In reply to Gabriele Svelto [:gsvelto] from comment #37)
> Created attachment 791247 [details] [diff] [review]
> [PATCH v7] Add support for CDMA call waiting
> 
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #33)
> > With patch v6 applied, I don't see any screen change when the 2nd call
> > comes. I added log message in dialer.js and calls_handler.js to make sure
> > that dialer did receive 'callschanged' event with secondNumber assigned. But
> > no screen changes. No hold/reject buttons showing up. 
> > 
> > After two remote parties are released, the call screen is hung there.
> 
> Thanks for the testing Hsin-Yi, I made a silly silly mistake and I'm sorry
> to have wasted your time; my previous patch contained this to detect CDMA
> call waiting:
> 
> function cdmaCallWaiting() {
>   return ((telephony.length == 1) &&                   <-- silly mistake here
>           (telephony.calls[0].state == 'connected') &&
>           telephony.calls[0].secondNumber);
> }
> 
> I've fixed it and uploaded a new patch. If this version doesn't work still
> I'll write some unit tests first before going forward.

Now the call screen is displayed correctly when there's 2nd call incoming. However, telephonyCall.hold() seems not called. There's no response when I pressed 'hold call' button. I didn't see the hold() function being called in gecko.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #38)
> Now the call screen is displayed correctly when there's 2nd call incoming.
> However, telephonyCall.hold() seems not called. There's no response when I
> pressed 'hold call' button. I didn't see the hold() function being called in
> gecko.

OK, let me write some unit tests to go with this first and then we can give it another try. I'm on PTO today but I should be able to do them tomorrow.
Blocks: 873006
Attached file UI spec
Updated the ETA for this bug. The functionality was in pretty good shape up until comment 26 (fourth iteration of the patch), then we started running into problems. I will be adapting the code to the new UX spec and will be introducing unit-tests alongside to be able to test this even without a CDMA network. Hopefully this should be done by the end of this week.
Status: NEW → ASSIGNED
Whiteboard: [UX ETA:8/9], [FT:RIL], [Sprint:3] → [UX ETA:8/9], [ETA:8/26], [FT:RIL], [Sprint:3]
Whiteboard: [UX ETA:8/9], [ETA:8/26], [FT:RIL], [Sprint:3] → [UX ETA:8/9], [ETA:8/26], [FT:RIL], [Sprint:3][u= c= p=0]
Re-based patch accomodating for the latest dialer changes. I have also fixed a couple of places where I was calling telephony.calls[0].answer() instead of telephony.calls[0].hold() when in CDMA mode which could have accounted for the problems we've seen in testing.

This patch doesn't have the latest UI spec changes and still lacks unit-tests so don't test just yet. I'll try to add the UI changes and unit-tests tomorrow.
Attachment #791247 - Attachment is obsolete: true
Whiteboard: [UX ETA:8/9], [ETA:8/26], [FT:RIL], [Sprint:3][u= c= p=0] → [UX ETA:8/9], [FT:RIL], [Sprint:3], [u=commsapps-user c=dialer p=3 s=v1.2-features-sprint-3]
Updated patch with all the new UI elements added (and hopefully proper switching that enables them when CDMA call waiting kicks in).

Hsin-Yi, if you have a minute could you test this? I haven't had time to write unit-tests yet so I'm still unsure if this works but I've fixed quite a few bugs I've spotted just by re-reading the code so maybe it's better than before. Even if it doesn't work completely I still would like to know if the new UI (as shown in attachment 792676 [details]) pops up correctly :-)
Attachment #794112 - Attachment is obsolete: true
Flags: needinfo?(htsai)
(In reply to Gabriele Svelto [:gsvelto] from comment #43)
> Created attachment 796778 [details] [diff] [review]
> [PATCH v9] Add support for CDMA call waiting
> 
> Updated patch with all the new UI elements added (and hopefully proper
> switching that enables them when CDMA call waiting kicks in).
> 
> Hsin-Yi, if you have a minute could you test this? I haven't had time to
> write unit-tests yet so I'm still unsure if this works but I've fixed quite
> a few bugs I've spotted just by re-reading the code so maybe it's better
> than before. Even if it doesn't work completely I still would like to know
> if the new UI (as shown in attachment 792676 [details]) pops up correctly :-)

Sorry that I am in Toronto for W3C meeting now so I won't be able to test this for you until next week. Let me ask for Enpei's help first. Hopefully the test won't be blocked.

Enpei: do you think you are able to help test this? Thank you!
Flags: needinfo?(htsai) → needinfo?(echu)
Hi Hsin Yi and Gabriele,

There are 3 user stories I am going to test this week, so I don't have extra time to try the patch right now. Can I verify it after it is landed on master? If you still need the patch to be verified first, may need you to check someone else first. Sorry for that.
Flags: needinfo?(echu)
(In reply to echu from comment #45)
> There are 3 user stories I am going to test this week, so I don't have extra
> time to try the patch right now. Can I verify it after it is landed on
> master? If you still need the patch to be verified first, may need you to
> check someone else first. Sorry for that.

It's no problem if you don't have spare cycles for this. I still have to write unit-tests so it can land safely next week; I was just curious to see if it worked or not before writing the unit-tests :)
Final refresh of this patch, now complete with unit tests which should cover all the added functionality minus the UI parts which I've visually verified. This is finally good for review :-) (and landing after Enpei verifies it works correctly on a real CDMA network)
Attachment #796778 - Attachment is obsolete: true
Attachment #797430 - Flags: review?(etienne)
Comment on attachment 797430 [details] [diff] [review]
[PATCH v10] Add support for CDMA call waiting

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

This is shaping up nicely :)

I'd like to have a last review round on a rebased patch with the comments below addressed but we're very close!

::: apps/communications/dialer/js/call_screen.js
@@ +34,5 @@
>    },
>  
> +  set cdmaCallWaiting(enabled) {
> +    this.calls.dataset.cdmaCallWaiting = enabled;
> +  },

we have tests for the CallScreen too now :)
Well we have one, but it's a very good example to test this method too.

::: apps/communications/dialer/js/calls_handler.js
@@ +108,4 @@
>        }
>      });
>  
> +    // Handle CDMA call waiting

nit: this comment might be superfluous

@@ +117,5 @@
>      if (handledCalls.length === 0) {
>        exitCallScreen(false);
> +
> +      // Always clear the CDMA call waiting state when hanging up the last call
> +      CallScreen.cdmaCallWaiting = false;

just curious, what led to this?
(since exitCallScreen is closing the window and blowing up everything anyway)
is it to make the transition look nicer?

::: apps/communications/dialer/js/handled_call.js
@@ +112,5 @@
> +      node.textContent = _('switch-calls');
> +      self._cachedInfo = _('switch-calls');
> +    });
> +    return;
> +  }

We should test this.
(lazyl10n is already mocked so should be pretty straightforward)

::: apps/communications/dialer/test/unit/calls_handler_test.js
@@ +71,5 @@
> +  /* Should be called in the context of a suite after one call has already been
> +   * added via telephonyAddCall(). */
> +  function telephonyAddCdmaCall(number, opt) {
> +    MockMozTelephony.calls[0].secondNumber = number;
> +    MockMozTelephony.calls[0].state = 'connected';

we could even pass the MockCall in parameter. It might be more readable. Your call (pun indented).

@@ +234,5 @@
> +        var holdSpy = this.sinon.spy(mockCall, 'hold');
> +        MockMozTelephony.mTriggerCallsChanged();
> +        assert.isTrue(showSpy.calledOnce);
> +        CallsHandler.holdAndAnswer();
> +        assert.isTrue(holdSpy.calledOnce);

should we spy/verify hideIncoming here too?
Attachment #797430 - Flags: review?(etienne)
Thanks for the review :)

(In reply to Etienne Segonzac (:etienne) from comment #48)
> ::: apps/communications/dialer/js/call_screen.js
> @@ +34,5 @@
> >    },
> >  
> > +  set cdmaCallWaiting(enabled) {
> > +    this.calls.dataset.cdmaCallWaiting = enabled;
> > +  },
> 
> we have tests for the CallScreen too now :)
> Well we have one, but it's a very good example to test this method too.

Alright, I'll add a test there.

> @@ +117,5 @@
> >      if (handledCalls.length === 0) {
> >        exitCallScreen(false);
> > +
> > +      // Always clear the CDMA call waiting state when hanging up the last call
> > +      CallScreen.cdmaCallWaiting = false;
> 
> just curious, what led to this?
> (since exitCallScreen is closing the window and blowing up everything anyway)
> is it to make the transition look nicer?

I didn't know the window was being closed, I thought it would be just hidden and thus I thought of reverting to the original state. It's superfluous then and I'll remove it.

> ::: apps/communications/dialer/js/handled_call.js
> @@ +112,5 @@
> > +      node.textContent = _('switch-calls');
> > +      self._cachedInfo = _('switch-calls');
> > +    });
> > +    return;
> > +  }
> 
> We should test this.
> (lazyl10n is already mocked so should be pretty straightforward)

OK, I'll check that out.

> ::: apps/communications/dialer/test/unit/calls_handler_test.js
> @@ +71,5 @@
> > +  /* Should be called in the context of a suite after one call has already been
> > +   * added via telephonyAddCall(). */
> > +  function telephonyAddCdmaCall(number, opt) {
> > +    MockMozTelephony.calls[0].secondNumber = number;
> > +    MockMozTelephony.calls[0].state = 'connected';
> 
> we could even pass the MockCall in parameter. It might be more readable.
> Your call (pun indented).

Well, the only call this applies to is the first call and it should be called only after having added it so I thought this would be a better interface.

> @@ +234,5 @@
> > +        var holdSpy = this.sinon.spy(mockCall, 'hold');
> > +        MockMozTelephony.mTriggerCallsChanged();
> > +        assert.isTrue(showSpy.calledOnce);
> > +        CallsHandler.holdAndAnswer();
> > +        assert.isTrue(holdSpy.calledOnce);
> 
> should we spy/verify hideIncoming here too?

Yep, I'll add it.
Nits addressed and new tests added :-)
Attachment #797430 - Attachment is obsolete: true
Attachment #797861 - Flags: review?(etienne)
Comment on attachment 797861 [details] [diff] [review]
[PATCH v11] Add support for CDMA call waiting

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

r=me with comments addressed, thanks!

::: apps/communications/dialer/js/calls_handler.js
@@ +572,5 @@
>    function ignore() {
> +    /* On CDMA there's no way to hangup an incoming call so we just skip this
> +     * step and hide the incoming call. */
> +    if (cdmaCallWaiting()) {
> +        stopWaitingTone();

nit: indentation

::: apps/communications/dialer/style/oncall.css
@@ +393,5 @@
> +  width: 12.8rem;
> +  border: 0;
> +  border-radius: .3rem;
> +  background: url('images/ActionIcon_40x40_switchlines.png'),
> +              url("images/btn_switchlines.png");

nit: single quotes everywhere

::: apps/communications/dialer/test/unit/call_screen_test.js
@@ +22,5 @@
> +    // Replace the existing elements
> +    if (CallScreen != null) {
> +      CallScreen.screen = screen;
> +      CallScreen.calls = calls;
> +    }

We need to move the requireApp in a suiteSetup then if we plan on needing this.

Don't like it very much but I don't have a better suggestion right now.
Attachment #797861 - Flags: review?(etienne) → review+
No longer blocks: 873006
Blocks: 912005
Un-bitrotted patch with all nits from comment 51 addressed. Enpei can you test this for me? If it works then we're good to go :)
Attachment #797861 - Attachment is obsolete: true
Flags: needinfo?(echu)
After discussing with Ken Chang & Enpei we decided to land this since it's r+'d and passes all the unit tests. If we encounter issues during QA we'll open follow ups for the individual issues.
Flags: needinfo?(echu)
Merged the patch in attachment 801404 [details] [diff] [review] with r=etienne as per comment 51.

master: 11ce9c03c9ae36acf2773365f8e5321c6d82d9fa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Neo provided updated Call waiting layout spec. please refer to the attachment.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: