970187 - [Dialer] Support CDMA 3-way (conference) calls

Status: RESOLVED FIXED

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

Comment 1

[Ken Chang[:kenkai]]

Carol, have you discussed with partner for the UX design of this bug?

Comment 2

[Carrie Wang [New account: [e-mail]]]

I think Juwei will take care CDMA part and sync with Joe. Thanks!

Comment 3

[Juwei Huang[:juwei]]

Got it! Spec will deliver later.

Comment 4

[Juwei Huang[:juwei]]

Attachment: CDMA_conference call_v1.0.pdf

Spec delivered.

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

triage:feature should not block, remove blocking flag

Wayne, can you work with partner to take this bug on Gaia? Thanks

Comment 6

[Wayne Chang [:wchang]]

Rin,

Can someone from your team work on Gaia for this? the UX spec has been provided here.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

The UX flow here seems to depend on the new incoming screen design so this depends on bug 928700.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

On second thought the UX flow shown on slide 7 of attachment 8377491 [details] is for call waiting, could we revert it to the flow we currently have? We wouldn't have to implement bug 928700 then and I don't see any benefit from implementing it if what we're focusing on is just 3-way calling.

Comment 9

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

(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> On second thought the UX flow shown on slide 7 of attachment 8377491 [details]
> [details] is for call waiting, could we revert it to the flow we currently
> have? We wouldn't have to implement bug 928700 then and I don't see any
> benefit from implementing it if what we're focusing on is just 3-way calling.

Agree with Gabriele's comment on call waiting flow.
ni? Juwei for UX confirmation.

Comment 10

[Juwei Huang[:juwei]]

Attachment: CDMA_conference call_v1.1.pdf

If we are not implement bug928700, I'm ok to modify flow back to the old one.
Spec updated.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Juwei Huang from comment #10)
> If we are not implement bug928700, I'm ok to modify flow back to the old one.
> Spec updated.

+1 Thanks! Unblocking from bug 928700, this will be less risky and then we have all the time we want to refactor the incoming call screen without release pressure.

Comment 12

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

Needinfo to Wayne and Anshul.
is this also expected before 2/28? If so, we need to get someone to work on it asap.

Comment 13

[Anshul]

Hey Wesley, yes we would like this bug to also land by 2/28.

Comment 14

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

Rin-san,
KDDI owns this one?

Comment 15

[Rin Nagaike]

(In reply to Wayne Chang [:wchang] from comment #6)
> Rin,
> 
> Can someone from your team work on Gaia for this? the UX spec has been
> provided here.

(In reply to Wesley Huang [:wesley_huang] from comment #14)
> Rin-san,
> KDDI owns this one?

Wayne, Wesley,
Our team will take its Gaia work. It's bit difficult land it by 2/28, though.

Comment 16

[Wayne Chang [:wchang]]

(In reply to Wesley Huang [:wesley_huang] from comment #12)
> Needinfo to Wayne and Anshul.
> is this also expected before 2/28? If so, we need to get someone to work on
> it asap.

Partner has this. 


> Wayne, Wesley,
> Our team will take its Gaia work. It's bit difficult land it by 2/28, though.

Rin, can your team try to have this ready for verification during our meetup next week?

Thanks.

Comment 17

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

Attachment: ugly gaia test patch to successfully make out a 2nd call

This is my personal test patch only for making 3-way calling work. This doesn't touch modification required by CDMA UX-specific flow.

Comment 18

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

Comment on attachment 8390348 [details] [diff] [review]
ugly gaia test patch to successfully make out a 2nd call

This is my personal test patch only for making 3-way calling work. This doesn't touch modification required by CDMA UX-specific flow.

Note that without this patch, it's mainly likely to fail in making out a second call.

Comment 19

[Yusuke Yamamoto]

Hi Hsin-Yi,
Thanks the test patch to make out a 2nd call.
I'm going to implement this bug.

Comment 21

[Michael Schwartz [:m4]]

Bringing the conversation from https://bugzilla.mozilla.org/show_bug.cgi?id=987374

1. Connect an outgoing call
2. While connected, connect a second call
3. Merge the calls
4. Disconnect either of the calls from the far party

Result: Both calls are displayed

Expected: Just starting up this discussion again since we haven't made any progress.  I propose we go with the solution where we do not show call information once we enter the multiparty call state.  Further, in Step 3, a new call cannot be initiated even though the network allows it.  The calls cannot be separated even though the network allows it.

Comment 22

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Michael Schwartz [:m4] from comment #21)
> Bringing the conversation from
> https://bugzilla.mozilla.org/show_bug.cgi?id=987374
> 
> 1. Connect an outgoing call
> 2. While connected, connect a second call
> 3. Merge the calls
> 4. Disconnect either of the calls from the far party
> 
> Result: Both calls are displayed
> 
> Expected: Just starting up this discussion again since we haven't made any
> progress.  I propose we go with the solution where we do not show call
> information once we enter the multiparty call state.

FYI we had a similar discussion in bug 882980 and ultimately decided to treat two calls as a single one w/o information about who's connected and who's not as that information is fundamentally not available. So I agree with not showing the separate calls but a single merged one instead as there's really no way to tell them apart.

> Further, in Step 3, a
> new call cannot be initiated even though the network allows it.  The calls
> cannot be separated even though the network allows it.

What do you mean by initiating a new call in step 3? Adding yet-another call to the conference? IIRC that should be possible only if one of the two original calls has hanged up (i.e. it works only as long as there's up to three participants in the conference). Unfortunately since there's no way to know if one of the participants has dropped out there's no reliable way to let the user add another one. As for separating the calls I didn't know that was possible at all on CDMA, what happens then? Does the state go into call waiting mode?

Comment 23

[Michael Schwartz [:m4]]

> (In reply to Gabriele Svelto [:gsvelto] from comment #22)
> What do you mean by initiating a new call in step 3?

Basically, the user needs the ability to send a FLASH or FLASH WITH INFO and the network may or may not honour it.

> As for separating the calls I didn't know that was possible at all on CDMA, what happens then?

As usual no feedback is provided.

Comment 24

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Michael Schwartz [:m4] from comment #23)
> Basically, the user needs the ability to send a FLASH or FLASH WITH INFO and
> the network may or may not honour it.
> [...]
> As usual no feedback is provided.

OK, then both features would be fairly confusing for an user as they might succeed sometimes but fail silently otherwise. I see why you'd want both of them disabled.

Comment 25

[Michael Schwartz [:m4]]

(In reply to Gabriele Svelto [:gsvelto] from comment #24)
> I see why you'd want both of them disabled.

I'm not suggesting they be disabled.  Just that no assumptions be made about their success or failure.

Comment 26

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

(In reply to Michael Schwartz [:m4] from comment #21)
> Bringing the conversation from
> https://bugzilla.mozilla.org/show_bug.cgi?id=987374
> 
> 1. Connect an outgoing call
> 2. While connected, connect a second call
> 3. Merge the calls
> 4. Disconnect either of the calls from the far party
> 
> Result: Both calls are displayed
> 
> Expected: Just starting up this discussion again since we haven't made any
> progress.  I propose we go with the solution where we do not show call
> information once we enter the multiparty call state.  

yes, this is what we agreed in Tokyo workshop. And this is also what UX suggests.

> Further, in Step 3, a
> new call cannot be initiated even though the network allows it.  The calls
> cannot be separated even though the network allows it.

(In reply to Gabriele Svelto [:gsvelto] from comment #24)
> (In reply to Michael Schwartz [:m4] from comment #23)
> > Basically, the user needs the ability to send a FLASH or FLASH WITH INFO and
> > the network may or may not honour it.
> > [...]
> > As usual no feedback is provided.
> 
> OK, then both features would be fairly confusing for an user as they might
> succeed sometimes but fail silently otherwise. I see why you'd want both of
> them disabled.

We discussed this in the workshop as well. And the UX suggestion is, to avoid confusing user and avoid triggering surprising action, disabling the function.

With CDMA burst message, we will be able to know if the remote party answers or leaves the 3way call. That enhances the whole feature.

Comment 27

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)
> > I propose we go with the solution where we do not show call
> > information once we enter the multiparty call state.  
> yes, this is what we agreed in Tokyo workshop. And this is also what UX
> suggests.

Where is the latest UX spec?  Is there a bug for the change?

> We discussed this in the workshop as well. And the UX suggestion is, to
> avoid confusing user and avoid triggering surprising action, disabling the
> function.

Will taking this functionality away be able to meet conformance?

> With CDMA burst message, we will be able to know if the remote party answers
> or leaves the 3way call. That enhances the whole feature.

That is a carrier specific message that the reference solution shouldn't rely on.  The OEMs can customize the solution as needed.

Comment 28

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

(In reply to Michael Schwartz [:m4] from comment #27)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)
> > > I propose we go with the solution where we do not show call
> > > information once we enter the multiparty call state.  
> > yes, this is what we agreed in Tokyo workshop. And this is also what UX
> > suggests.
> 
> Where is the latest UX spec?  Is there a bug for the change?
> 

This is the latest UX spec per my understanding: attachment 8379451 [details] CDMA_conference call_v1.1.pdf

> > We discussed this in the workshop as well. And the UX suggestion is, to
> > avoid confusing user and avoid triggering surprising action, disabling the
> > function.
> 
> Will taking this functionality away be able to meet conformance?
> 

Could you please point me where I could get the conformance document, then our team could provide a better answer, maybe?
However, IIRC according to the demo video of other market phone in the Tokyo workshop, the phone on the market indeed has the same behaviour as the UX spec suggests. Another CDMA android phone I have on hand shows the same design - add-call is disabled after 2nd call is dialed out.

> > With CDMA burst message, we will be able to know if the remote party answers
> > or leaves the 3way call. That enhances the whole feature.
> 
> That is a carrier specific message that the reference solution shouldn't
> rely on.  The OEMs can customize the solution as needed.

Comment 29

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

For your reference, this is the video from our partner. This shows the behaviour of a existing product.
http://sdrv.ms/1c2EWQf

Comment 30

[Kotaro Oki]

Attachment: 3way_call.patch

Hi Mozillian.

I made patch for CDMA to "add-call is disabled after 2nd call is dialed out." and add-call is not show after 3-way calls.
Please review.

Thanks.

Comment 31

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

Hi Kotaro-san,

Please have a git pull request to https://github.com/mozilla-b2g/gaia first, then attach the pull request link to this bug and ask for review. The proper reviewers would be :etienne or Gabriele (:gsvelto). Thank you!

Comment 32

[Kotaro Oki]

Attachment: patch delete

Hi Hsin-Yi.

Thank you for teach me and sorry for inconvenient.
I found mistake in the patch. So I will retry later.

Thanks.

Comment 33

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

(In reply to Kotaro Oki from comment #32)
> Created attachment 8409555 [details]
> patch delete
> 
> Hi Hsin-Yi.
> 
> Thank you for teach me and sorry for inconvenient.

No problem. The bugzilla review process is kinda complex for new comers. :)

> I found mistake in the patch. So I will retry later.
> 
> Thanks.

Comment 34

[Gabriele Svelto [:gsvelto] (PTO)]

Kotaro, I've given a brief glance at your patch and I have a couple of suggestions:

- You've put the logic that decides whether to add the add call button or not in the CallScreen, I'm not exactly fond of this as all the other logic dealing with call state is in the CallsHandler object (calls_handler.js) with CallScreen being only a presentation object. My suggestion is to move this logic in calls_handler.js and expose a way to adjust the call screen instead (see how this is done for CDMA call waiting in both call_screen.js and calls_handler.js)

- Similarly the code in conference_group_handler.js should query the CallsHandler to figure out if we're in CDMA mode or not instead of the CallScreen

- Last but not least do not use the navigator.mozMobileConnection, it's deprecated and we're getting rid of its last uses in bug 980387; use navigator.mozMobileConnections instead

Comment 35

[Yusuke Yamamoto]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)

Hi Hsin-Yi
We are implementing to make out a 2nd call to telephony_helper.js for 3-way calling on CDMA.
Let me ask you about your test patch ( attachment 8390348 [details] [diff] [review] ).

Your test patch comment outed some lines.
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L39-49
Actually, I'm not sure why did you comment out that lines.

I think, that lines are necessary to make a call.
# Maybe, in CDMA, is telephony.active always NULL...?

Comment 36

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

(In reply to Yusuke Yamamoto from comment #35)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> 
> Hi Hsin-Yi
> We are implementing to make out a 2nd call to telephony_helper.js for 3-way
> calling on CDMA.
> Let me ask you about your test patch ( attachment 8390348 [details] [diff] [review]
> [review] ).
> 
> Your test patch comment outed some lines.
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/telephony_helper.js#L39-49
> Actually, I'm not sure why did you comment out that lines.
> 
> I think, that lines are necessary to make a call.
> # Maybe, in CDMA, is telephony.active always NULL...?

According to 3gpp2 S.R0006-522-A_v1.0_070624.pdf, the steps to make a 3-way call is a) Table 1, XID 3S3: put the 1st call on hold (send out 1st FLASH command), b) Table 2, XID 4A: dial out the 2nd call (send out 2nd FLASH command), then c) Table 3, XID 5S2: merge the calls (send out 3rd FLASH command).

However, when I tried on the local and KDDI networks, it fails to create a 3-way call if I followed these steps. I referred to another Android phone which run well on the local network, and I learned that only two FLASH commands (case b and c) were sent out. That is, don't need to put the 1st call on hold. Simply go ahead with making a 2nd call. That's way I commented the lines in dialer.

When I visited Maida workweek, I realized that my patch is needed. Not sure if things change. Without the patch, is 3-way call still working?

Comment 37

[Yusuke Yamamoto]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #36)
> However, when I tried on the local and KDDI networks, it fails to create a
> 3-way call if I followed these steps. I referred to another Android phone
> which run well on the local network, and I learned that only two FLASH
> commands (case b and c) were sent out. That is, don't need to put the 1st
> call on hold. Simply go ahead with making a 2nd call. That's way I commented
> the lines in dialer.
thanks, got it about the background.
 
> When I visited Maida workweek, I realized that my patch is needed. Not sure
> if things change. Without the patch, is 3-way call still working?
It’s not working 3-way call without your test patch.
Then, we are implementing in telephony_helper.js based on your test patch.
Thats why my previous comment( comment #35 ).
We will continue to implement. thanks Hsin-Yi ;)

Comment 38

[Kotaro Oki]

Attachment: CDMA_3way_conferenceUI.patch

Hi, Gabriele.
Thank you for giving me advice.

I made a patch for 3way-talk UI like "CDMA_conference call_v1.1.pdf" include your advice.
This patch has only UI side. I mean, we need to implement a other patch to make
a call feature on telephony_helper.js. I'm just coding that feature.
Then, I'd like you to review my UI side patch.

I will send you a review request if I'll have finished to code that other patch.

Thanks.

Comment 39

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8423611 [details] [diff] [review]
CDMA_3way_conferenceUI.patch

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

This is better than the last time but you still have to decouple part of the presentation code from the logic. Also I'm not how your CSS changes are supposed to interact with the existing modes. See below for details.

::: apps/callscreen/js/call_screen.js
@@ +75,5 @@
> +    var isCDMA = CallsHandler.isCDMA;
> +    if(isCDMA) {
> +      var call_cnt = navigator.mozTelephony.calls.length;
> +      var group_cnt = navigator.mozTelephony.conferenceGroup.calls.length;
> +      var hc = CallsHandler.activeCall;

This logic really doesn't belong here, you should move it to CallsHandler.onCallsChanged() and ConferenceGroupHandler.onCallsChanged(). Basically the CallScreen class should only have presentation methods. The choice of when to display something should be in the event handlers in either the CallsHandler or the ConferenceGroupHandler objects.

In this case for example you could have a |cdmaConferenceCall| property that when set to true hides the add-call button and adjusts the appropriate CSS classes. Then when you detect the appropriate situation in ConferenceGroupHandler.onCallsChanged() you can act accordingly.

The same thing applies to the presence or absence of the add call button.

::: apps/callscreen/js/calls_handler.js
@@ +718,5 @@
> +   * Detects if we're in CDMA network
> +   *
> +   * @return {Boolean} Return true if we're in CDMA network.
> +   */
> +  function isCDMA() {

nit: isCdmaNetwork() sounds like a better name to me

@@ +721,5 @@
> +   */
> +  function isCDMA() {
> +    var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
> +    var conns = window.navigator.mozMobileConnections &&
> +                   window.navigator.mozMobileConnections[0];

You should not hard-code the use of the first SIM, instead you should figure out which SIM is currently in use and use that to pick the correct mozMobileConnection.

@@ +724,5 @@
> +    var conns = window.navigator.mozMobileConnections &&
> +                   window.navigator.mozMobileConnections[0];
> +    var voiceType = conns.voice ? conns.voice.type : null;
> +
> +    if (cdmaTypes.indexOf(voiceType) !== -1) {

nit: Use

return (cdmaTypes.indexOf(voiceType) !== -1);

No need for an if/else block.

@@ +768,5 @@
>  
>      get activeCall() {
>        return activeCall();
> +    },
> +    get isCDMA() {

No need for this getter, just use the method instead.

::: apps/callscreen/js/conference_group_handler.js
@@ +37,5 @@
>  
>      LazyL10n.get(function localized(_) {
> +      var isCDMA = CallsHandler.isCDMA;
> +      if (isCDMA) {
> +        groupDetailsHeader.textContent = groupLabel.textContent = 

nit: Remove the trailing space.

@@ +62,5 @@
>      groupLine.classList.remove('ended');
>      groupLine.classList.remove('held');
>      groupDurationChildNode.textContent = null;
> +    var isCDMA = CallsHandler.isCDMA;
> +    if(isCDMA) {

nit: There's no need to assign CallsHandler.isCDMA to a variable, use it directly, also always put a space between the 'if' and the following parentheses

if (CallsHandler.isCdmaNetwork()) { ...

@@ +63,5 @@
>      groupLine.classList.remove('held');
>      groupDurationChildNode.textContent = null;
> +    var isCDMA = CallsHandler.isCDMA;
> +    if(isCDMA) {
> +      groupShow.hidden = true;

Move this into the CallScreen logic, create a single property there to switch the CDMA conference call UI on and off.

@@ +79,5 @@
>      });
>      groupLine.classList.add('ended');
>      groupLine.classList.remove('held');
> +    if(groupShow.hidden) {
> +      groupShow.hidden = false;

Likewise.

::: apps/callscreen/style/oncall.css
@@ +421,4 @@
>      margin: 0 1.3rem 0 0;
>      padding: 0;
>      height: 4rem;
> +    width: 6.4rem;

I'm not sure if this will work, I think it will break the CDMA call waiting UI.

@@ +777,4 @@
>    }
>  
>    #group-call .hangup-button,
> +  #calls:not(.big-duration):not(.small-duration) #group-call > .duration.isTimer,

Can you explain what you are trying to achieve with the small-duration class here?

::: shared/test/unit/mocks/dialer/mock_calls_handler.js
@@ +37,4 @@
>    mTeardown: function() {
>      this.mActiveCall = null;
>      this.mUpdateKeypadEnabledCalled = true;
> +

nit: Remove this empty line.

Comment 40

[Kotaro Oki]

(In reply to Gabriele Svelto [:gsvelto] from comment #39)
> Comment on attachment 8423611 [details] [diff] [review]
> CDMA_3way_conferenceUI.patch
> 
> Review of attachment 8423611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is better than the last time but you still have to decouple part of the
> presentation code from the logic. Also I'm not how your CSS changes are
> supposed to interact with the existing modes. See below for details.
> 
> ::: apps/callscreen/js/call_screen.js
> @@ +75,5 @@
> > +    var isCDMA = CallsHandler.isCDMA;
> > +    if(isCDMA) {
> > +      var call_cnt = navigator.mozTelephony.calls.length;
> > +      var group_cnt = navigator.mozTelephony.conferenceGroup.calls.length;
> > +      var hc = CallsHandler.activeCall;
> 
> This logic really doesn't belong here, you should move it to
> CallsHandler.onCallsChanged() and ConferenceGroupHandler.onCallsChanged().
> Basically the CallScreen class should only have presentation methods. The
> choice of when to display something should be in the event handlers in
> either the CallsHandler or the ConferenceGroupHandler objects.
> 
> In this case for example you could have a |cdmaConferenceCall| property that
> when set to true hides the add-call button and adjusts the appropriate CSS
> classes. Then when you detect the appropriate situation in
> ConferenceGroupHandler.onCallsChanged() you can act accordingly.
> 
> The same thing applies to the presence or absence of the add call button.
> 
> ::: apps/callscreen/js/calls_handler.js
> @@ +718,5 @@
> > +   * Detects if we're in CDMA network
> > +   *
> > +   * @return {Boolean} Return true if we're in CDMA network.
> > +   */
> > +  function isCDMA() {
> 
> nit: isCdmaNetwork() sounds like a better name to me
> 
> @@ +721,5 @@
> > +   */
> > +  function isCDMA() {
> > +    var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
> > +    var conns = window.navigator.mozMobileConnections &&
> > +                   window.navigator.mozMobileConnections[0];
> 
> You should not hard-code the use of the first SIM, instead you should figure
> out which SIM is currently in use and use that to pick the correct
> mozMobileConnection.
> 
> @@ +724,5 @@
> > +    var conns = window.navigator.mozMobileConnections &&
> > +                   window.navigator.mozMobileConnections[0];
> > +    var voiceType = conns.voice ? conns.voice.type : null;
> > +
> > +    if (cdmaTypes.indexOf(voiceType) !== -1) {
> 
> nit: Use
> 
> return (cdmaTypes.indexOf(voiceType) !== -1);
> 
> No need for an if/else block.
> 
> @@ +768,5 @@
> >  
> >      get activeCall() {
> >        return activeCall();
> > +    },
> > +    get isCDMA() {
> 
> No need for this getter, just use the method instead.
> 
> ::: apps/callscreen/js/conference_group_handler.js
> @@ +37,5 @@
> >  
> >      LazyL10n.get(function localized(_) {
> > +      var isCDMA = CallsHandler.isCDMA;
> > +      if (isCDMA) {
> > +        groupDetailsHeader.textContent = groupLabel.textContent = 
> 
> nit: Remove the trailing space.
> 
> @@ +62,5 @@
> >      groupLine.classList.remove('ended');
> >      groupLine.classList.remove('held');
> >      groupDurationChildNode.textContent = null;
> > +    var isCDMA = CallsHandler.isCDMA;
> > +    if(isCDMA) {
> 
> nit: There's no need to assign CallsHandler.isCDMA to a variable, use it
> directly, also always put a space between the 'if' and the following
> parentheses
> 
> if (CallsHandler.isCdmaNetwork()) { ...
> 
> @@ +63,5 @@
> >      groupLine.classList.remove('held');
> >      groupDurationChildNode.textContent = null;
> > +    var isCDMA = CallsHandler.isCDMA;
> > +    if(isCDMA) {
> > +      groupShow.hidden = true;
> 
> Move this into the CallScreen logic, create a single property there to
> switch the CDMA conference call UI on and off.
> 
> @@ +79,5 @@
> >      });
> >      groupLine.classList.add('ended');
> >      groupLine.classList.remove('held');
> > +    if(groupShow.hidden) {
> > +      groupShow.hidden = false;
> 
> Likewise.
> 
> ::: apps/callscreen/style/oncall.css
> @@ +421,4 @@
> >      margin: 0 1.3rem 0 0;
> >      padding: 0;
> >      height: 4rem;
> > +    width: 6.4rem;
> 
> I'm not sure if this will work, I think it will break the CDMA call waiting
> UI.
> 
> @@ +777,4 @@
> >    }
> >  
> >    #group-call .hangup-button,
> > +  #calls:not(.big-duration):not(.small-duration) #group-call > .duration.isTimer,
> 
> Can you explain what you are trying to achieve with the small-duration class
> here?
> 
> ::: shared/test/unit/mocks/dialer/mock_calls_handler.js
> @@ +37,4 @@
> >    mTeardown: function() {
> >      this.mActiveCall = null;
> >      this.mUpdateKeypadEnabledCalled = true;
> > +
> 
> nit: Remove this empty line.

Hi, Gabriele.
Thank you for review.
I wrote answer to your question.
Please see below.

> ::: apps/callscreen/style/oncall.css
> @@ +421,4 @@
> >      margin: 0 1.3rem 0 0;
> >      padding: 0;
> >      height: 4rem;
> > +    width: 6.4rem;
> 
> I'm not sure if this will work, I think it will break the CDMA call waiting
> UI.

I do not think that it is breaking the "CDMA call waiting UI".
This is only re-size to the switch call button.
In the original code of firefoxos, the "switch call button" is too large compared with "CDMA_conferencecall_v1.1.pdf".
So, I modified this code.


> @@ +777,4 @@
> >    }
> >  
> >    #group-call .hangup-button,
> > +  #calls:not(.big-duration):not(.small-duration) #group-call > .duration.isTimer,
> 
> Can you explain what you are trying to achieve with the small-duration class
> here?

Total duration is displayed the upper right corner in the case of Conference call display
in CDMA_conference call_v1.1.pdf.
So, I modified this code and set total duration on the upper right corner.

Comment 41

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kotaro Oki from comment #40)
> I do not think that it is breaking the "CDMA call waiting UI".
> This is only re-size to the switch call button.
> In the original code of firefoxos, the "switch call button" is too large
> compared with "CDMA_conferencecall_v1.1.pdf".
> So, I modified this code.

OK, I see. The original code however was made to match the original UI spec (attachment 792676 [details] on bug 882980), do we have a new UI spec for this bug? "CDMA_conferencecall_v1.1.pdf" contains only the UX flows so it shouldn't be used as a reference for the dimensions and looks of the interface but only for the user interaction flow. Juwei do we have a new UI spec for this functionality?

> Total duration is displayed the upper right corner in the case of Conference
> call display
> in CDMA_conference call_v1.1.pdf.
> So, I modified this code and set total duration on the upper right corner.

OK, makes sense.

Comment 42

[Juwei Huang[:juwei]]

I've check the patch (thanks for Hsinyi), I think we have no plan for changing the UI right now. "CDMA_conferencecall_v1.1.pdf" is only the user experience flow for the feature, so all the precisely UI measurement should follow what UI spec provides: https://bug882980.bugzilla.mozilla.org/attachment.cgi?id=792676

Comment 43

[Kotaro Oki]

Attachment: switch_call_display.pdf

Hi, Juwei.

I got it.
However, if NOT change the Switch Call button's width, the Switch Call
button covers character string on Emulator.
Specifically, please confirm an attached image.

Thanks.

Comment 44

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kotaro Oki from comment #43)
> However, if NOT change the Switch Call button's width, the Switch Call
> button covers character string on Emulator.
> Specifically, please confirm an attached image.

Nice catch! The issue is the size of the font used for the "Switch calls" string, it should be much smaller than that. Can you fix it alongside this issue? Use the font size specified in attachment 792676 [details]. It would also be interesting to figure out when we regressed this.

Comment 45

[Naoya Matsumoto]

Hi Hsin-Yi.
By the way, the topic of your test patch on Comment 36, 
before dialing out the 2nd call, was the 1st call the state in which 
it was holding even if the 1st FLASH command was not sent out?

Comment 46

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

(In reply to Naoya Matsumoto from comment #45)
> Hi Hsin-Yi.
> By the way, the topic of your test patch on Comment 36, 
> before dialing out the 2nd call, was the 1st call the state in which 
> it was holding even if the 1st FLASH command was not sent out?

Thanks for the question!

No, 1st call state won't be changed to 'held' until the FLASH command is sent out and the success response is got.

Comment 47

[Naoya Matsumoto]

Thank's your answer.
Sorry to trouble you one more time. 
I just need to know that despite the fact that 1st FLASH command is not sent, 
the terminal used by the 1st call is changed to "hold" when 2nd FLASH command is sent.  

The point is the status of the terminal used by the 1st call.
I assume it is on hold, but need to know if you checked.

Comment 48

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

(In reply to Naoya Matsumoto from comment #47)
> Thank's your answer.
> Sorry to trouble you one more time. 
> I just need to know that despite the fact that 1st FLASH command is not
> sent, 
> the terminal used by the 1st call is changed to "hold" when 2nd FLASH
> command is sent.  
> 
> The point is the status of the terminal used by the 1st call.
> I assume it is on hold, but need to know if you checked.

Okay, I hope I got your question this time :)

Now, we, Dialer (A), wants to have a 3way call with 1st remote party (B) and 2nd party (C).

You were saying that my comment 36 mentioned we, Dialer(A) should send FLASH two times, 1st one is for making a 2nd call and 2nd one is merging them, that is somehow different from the spec. And you are concerned that if the 1st remote party (B) is correctly put on hold. Is that right?

My answer is: yes, I checked. 1st remote party (B) herd from operator that it's put on hold, and 1st remote party (B) didn't hear the voice either from the caller (A) or (C). After merging, 1st remote party (B) can hear the caller (A) and the 2nd remote party (C).

Did I answer your question correctly this time?

Comment 49

[Naoya Matsumoto]

Thank's your answer.
I was confirmed that I want to hear.
Thank you so much, Hsin-Yi.

Comment 50

[Juwei Huang[:juwei]]

Hi Kotaro,
Yes as Gabriele said, the font size in the visual spec is smaller. And it also has a secondary line below "Switch call".
It would be great if you could help to fix it, thanks :)

Comment 51

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

(In reply to Naoya Matsumoto from comment #49)
> Thank's your answer.
> I was confirmed that I want to hear.
> Thank you so much, Hsin-Yi.

Happy to help out. Let me know if you have other questions.

Comment 52

[Kotaro Oki]

Attachment: CDMA_3way_conferenceUI_02.patch

Hi, Gabriele.

Thank you for many advice.
I made a patch again.
This patch is corresponding to only a UI part.
I am still coding the part about the call function to make a call feature on telephony_helper.js.

Then, I'd like you to review my UI side patch again.

Thanks.

Comment 53

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8428626 [details] [diff] [review]
CDMA_3way_conferenceUI_02.patch

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

Sorry if this took a while. We're getting there but there's still some issues to address, see my comments below.

::: apps/callscreen/index.html
@@ +43,4 @@
>      <script defer type="application/javascript" src="/js/calls_handler.js"></script>
>      <script defer type="application/javascript" src="/js/conference_group_handler.js"></script>
>      <script defer type="application/javascript" src="/js/handled_call.js"></script>
> +    <script defer type="application/javascript" src="/shared/js/sim_settings_helper.js"></script>

This is not needed, see below.

::: apps/callscreen/js/call_screen.js
@@ +73,5 @@
>      this.calls.classList.toggle('big-duration', enabled);
> +
> +    var self = this;
> +    CallsHandler.isCdmaNetwork(function(ret) {
> +      if (ret && self.calls.classList.contains('small-duration')) {

I don't think we need to change the UI here, you should remove this, see my comment below on the CSS code.

::: apps/callscreen/js/calls_handler.js
@@ +128,5 @@
> +          CallScreen.hideAddCallButton = true;
> +        } else {
> +          CallScreen.hideAddCallButton = false;
> +        }
> +      }

The part about the conference group should belong to the ConferenceGroupHandler IMHO, that being said try to harmonize this with the CDMA call waiting code which also hides the add call button.

@@ +733,5 @@
> +   */
> +  function isCdmaNetwork(callback) {
> +    var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
> +
> +    SimSettingsHelper.getCardIndexFrom('outgoingCall', function(ci) {

You can do this check synchronously by using the |serviceId| field that is contained in the TelephonyCall object and which indicates which SIM the call is active on. So this function could become:

var ci = handledCalls[0].call.serviceId;
var type = navigator.mozMobileConnections[ci].voice.type;

return (cdmaTypes.indexOf(type) !== -1);

::: apps/callscreen/js/conference_group_handler.js
@@ +35,4 @@
>      }
>  
>      LazyL10n.get(function localized(_) {
> +      CallsHandler.isCdmaNetwork(function(ret) {

Use the synchronous function I mentioned above instead.

@@ +55,5 @@
>      if (telephony.conferenceGroup.calls.length >= 2) {
>        CallsHandler.checkCalls();
>      }
> +
> +    CallsHandler.isCdmaNetwork(function(ret) {

Likewise.

@@ +68,4 @@
>      groupLine.classList.remove('ended');
>      groupLine.classList.remove('held');
>      groupDurationChildNode.textContent = null;
> +    CallsHandler.isCdmaNetwork(function(ret) {

Likewise, you should also not add the 'small-duration' class directly as I described above.

@@ +85,4 @@
>      });
>      groupLine.classList.add('ended');
>      groupLine.classList.remove('held');
> +    CallsHandler.isCdmaNetwork(function(ret) {

Likewise x2.

::: apps/callscreen/style/oncall.css
@@ +777,4 @@
>    }
>  
>    #group-call .hangup-button,
> +  #calls:not(.big-duration):not(.small-duration) #group-call > .duration.isTimer,

As I mentioned in my post above I'm not sure we want to change this at all and comment 42 seems to confirm this. Leave the duration at its normal size and position as per the original UI spec.

Comment 54

[Kotaro Oki]

Attachment: https://github.com/mozilla-b2g/gaia/pull/19976

Hi, Gabriele.

I have answered your comment, please check.
And I have sent PR, please review.

>> ::: apps/callscreen/js/calls_handler.js
>> @@ +128,5 @@
>> +          CallScreen.hideAddCallButton = true;
>> +        } else {
>> +          CallScreen.hideAddCallButton = false;
>> +        }
>> +      }
> The part about the conference group should belong to the >ConferenceGroupHandler IMHO, that being said try to harmonize this with the CDMA call waiting code which also hides the add call button.

2nd call in CDMA_conferencecall_v1.1.pdf's P6 was say below.
>> ** Please note that CDMA only support 2 people in one line, so there is no “Add call”button here
So I put this part in calls_handler.js.

Comment 55

[Kotaro Oki]

How might it be the result of the review?
Is there any pointing out?

Comment 56

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kotaro Oki from comment #55)
> How might it be the result of the review?
> Is there any pointing out?

Sorry Kotaro, I hadn't got enough time to review this due to the pending deadlines and I'd like to have one of the dialer peers have a look too.

Comment 57

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8433973 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19976

I've left some comments on GitHub, most are nits but there's still a couple of bits I don't understand.

Once you've addressed the issues I've highlighted before giving you an r+ I'd like to hear Rik's opinion about the way in which we hide the "Group show" button. The current PR sets its |hidden| property to true but I seem to remember that you preferred these kind of things to be addressed via CSS instead of programmatically. Can you have a look at the patch? BTW if there's something I've missed in my review feel free to chime in. I'm reviewing this just because I'm somewhat familiar with the CDMA functionality but I'm still not a dialer/callscreen peer so in theory I can't r+ this on my own.

Comment 58

[Kotaro Oki]

(In reply to Gabriele Svelto [:gsvelto] from comment #57)
> Comment on attachment 8433973 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/19976
> 
> I've left some comments on GitHub, most are nits but there's still a couple
> of bits I don't understand.
> 
> Once you've addressed the issues I've highlighted before giving you an r+
> I'd like to hear Rik's opinion about the way in which we hide the "Group
> show" button. The current PR sets its |hidden| property to true but I seem
> to remember that you preferred these kind of things to be addressed via CSS
> instead of programmatically. Can you have a look at the patch? BTW if
> there's something I've missed in my review feel free to chime in. I'm
> reviewing this just because I'm somewhat familiar with the CDMA
> functionality but I'm still not a dialer/callscreen peer so in theory I
> can't r+ this on my own.

I have left some comments on GitHub.
After confirming the Rik's comment, I'm going to do the PR again with some fixed.

Comment 59

[Anthony Ricaud (:rik)]

Gabriele: Thanks for asking. I've left a lot of comments that I think should be addressed. And yes, I'd prefer another way to hide that button.

Comment 60

[Kotaro Oki]

Hi Rik and Gabriele,

Thank you for review.
I have left some comments on GitHub.
Please check it.

Thanks.

Comment 61

[Kotaro Oki]

Attachment: PR review: Bug970187

Hi Rik,

I have fixed all issues in https://bugzilla.mozilla.org/attachment.cgi?id=8433973.
Please review new PR.

Thanks.

Comment 62

[Anthony Ricaud (:rik)]

Comment on attachment 8439166 [details] [review]
PR review: Bug970187

A few comments left to address and I also had a question about the logic of the patch.

Comment 63

[Kotaro Oki]

(In reply to Anthony Ricaud (:rik) from comment #62)
> Comment on attachment 8439166 [details] [review]
> PR review: Bug970187
> 
> A few comments left to address and I also had a question about the logic of
> the patch.

I have confirmed and fixed.
Please review again.

Thanks.

Comment 64

[Anthony Ricaud (:rik)]

Comment on attachment 8439166 [details] [review]
PR review: Bug970187

I think we're missing two tests:
- Conference call on CDMA should trigger hidePlaceNewCallButton
- Call on non-CDMA should trigger showPlaceNewCallButton

Comment 65

[Kotaro Oki]

(In reply to Anthony Ricaud (:rik) from comment #64)
> Comment on attachment 8439166 [details] [review]
> PR review: Bug970187
> 
> I think we're missing two tests:
> - Conference call on CDMA should trigger hidePlaceNewCallButton
> - Call on non-CDMA should trigger showPlaceNewCallButton

1st, I think it have conference_group_handler_test.js.
Function of onCallsChanged() in conference_group_handler.js invoke cdmaConferenceCall(), and cdmaConferenceCall() invoke hidePlaceNewCallButton.

2nd, you are correct.
But do we should have it?
I do not have idea for this case.

Finaly, I have fixed and left comment on Github.
Please check it.

Comment 66

[Kotaro Oki]

I have updated the PR that fixed GitHub comment.

Comment 67

[Anthony Ricaud (:rik)]

Comment on attachment 8439166 [details] [review]
PR review: Bug970187

Forwarding to Gabriele while I'm away. 

I think we should remove the logic from the mocks and use "sinon" instead.

Comment 68

[Kotaro Oki]

(In reply to Anthony Ricaud (:rik) (out until July 1st) from comment #67)
> Comment on attachment 8439166 [details] [review]
> PR review: Bug970187
> 
> Forwarding to Gabriele while I'm away. 
> 
> I think we should remove the logic from the mocks and use "sinon" instead.

Hi Rik,

Please tell me more detail.
Where the line are you talking about?

Hi Gabriele,

Do you have any comment on my patch?

Thanks.

Comment 69

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8439166 [details] [review]
PR review: Bug970187

This is r+ with the following changes:

- Address the nits I've written down on GitHub, these are mostly about comments plus using sinon mocks instead of adding extra code to the call_screen_mock.js. I've left an example in the comments on how to use them but if you need more information you can refer to the documentation here http://sinonjs.org/docs/ or just ping me.

- Add a test that ensures that showPlaceNewCallButton() is called when in non-CDMA mode, the test should be added as part of the "receiving a first incoming call" suite, so here for example after the last test:

https://github.com/mozilla-b2g/gaia/blob/d5463e856e088ef9751deae76157744733c648a8/apps/callscreen/test/unit/calls_handler_test.js#L134

Once you've done these changes update the PR and notify me again. If Travis turns green we should be good to land. Thanks for all your work here!

Comment 70

[Gabriele Svelto [:gsvelto] (PTO)]

Clearing Anthony's needinfo as I've already commented on that on GitHub.

Comment 71

[Kotaro Oki]

Attachment: PR2 review: Bug970187

(In reply to Gabriele Svelto [:gsvelto] from comment #69)
> Comment on attachment 8439166 [details] [review]
> PR review: Bug970187
> 
> This is r+ with the following changes:
> 
> - Address the nits I've written down on GitHub, these are mostly about
> comments plus using sinon mocks instead of adding extra code to the
> call_screen_mock.js. I've left an example in the comments on how to use them
> but if you need more information you can refer to the documentation here
> http://sinonjs.org/docs/ or just ping me.
> 
> - Add a test that ensures that showPlaceNewCallButton() is called when in
> non-CDMA mode, the test should be added as part of the "receiving a first
> incoming call" suite, so here for example after the last test:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> d5463e856e088ef9751deae76157744733c648a8/apps/callscreen/test/unit/
> calls_handler_test.js#L134
> 
> Once you've done these changes update the PR and notify me again. If Travis
> turns green we should be good to land. Thanks for all your work here!

I confirmed your comment and fixed all issue.
And I make new PR, because some conflict was happened old PR.
Please review new attachment.

Thanks.

Comment 72

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8446470 [details] [review]
PR2 review: Bug970187

Excellent, let's wait for Travis to turn green and then let's land this.

Comment 73

[Kotaro Oki]

(In reply to Gabriele Svelto [:gsvelto] from comment #72)
> Comment on attachment 8446470 [details] [review]
> PR2 review: Bug970187
> 
> Excellent, let's wait for Travis to turn green and then let's land this.

Hi Gabriele,

I have confirmed some error in Travis.
I think there error is not my patches fault, and I do not know why some error happened.
Do you have idea for fix?

Thanks.

Comment 74

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kotaro Oki from comment #73)
> (In reply to Gabriele Svelto [:gsvelto] from comment #72)
> I have confirmed some error in Travis.
> I think there error is not my patches fault, and I do not know why some
> error happened.

Yes, the failures are unrelated and probably caused by intermittent issues.

> Do you have idea for fix?

I've re-triggered the failed jobs, I'll merge your PR as soon as they turn green.

Comment 75

[Gabriele Svelto [:gsvelto] (PTO)]

It seems that we've fully switched away from Travis and to TBPL; I had read something related to the transition but I didn't remember this happening so soon. Anyway the Try-Gaia run for this PR is fully green so I'll go ahead and merge it:

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=08692331d7f9338f1300f5a85aa246a7ba68d45b

Comment 76

[Gabriele Svelto [:gsvelto] (PTO)]

Merged to gaia/master 9f58418907f46af337782951e847198a9b3cf932

https://github.com/mozilla-b2g/gaia/commit/9f58418907f46af337782951e847198a9b3cf932