978639 - [Sora][Call][MPTY][GCF] GCF case 31.4.2.1.3 : Terminate the entire MultiParty call failed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[sync-1]

Firefox OS v1.3
 Mozilla build ID: 20140208004002
 
 DEFECT DESCRIPTION:
  GCF case 31.4.2.1.3 : Terminate the entire MultiParty call failed
 
  REPRODUCING PROCEDURES:
  Precondition:Run Anite test case 31.4.2.1.3
  1.Make an active multiparty call with two RP A-B, A-C.
  2.End the multiParty call with menu or with end key -> Can not end the multiparty call with menu（K.O）
  
  Note:Beetle Lite FF no MPTY function.
 
  EXPECTED BEHAVIOUR:
  2.On receipt of a RELEASE message in respect of each transaction identifier, the MS shall respond with a RELEASE COMPLETE message, andenter state U0 Null.
  MultiParty call is ended
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[Jason Smith [:jsmith]]

This sounds like a QC RIL only bug potentially.

Can someone confirm this doesn't reproduce on the Moz RIL?

Comment 2

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Per discussion with Ming, this issue cannot be reproduced on the real network, so it could be:

1. Something wrong with the Anite test equipment configuration
2. Something wrong with QCT RIL

Either case, should not block 1.3 release. So 1.3-- first, Ming, please re-nominate again if you find anything strange in the logs

Comment 3

[Michael Schwartz [:m4]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #2)
> Per discussion with Ming, this issue cannot be reproduced on the real
> network, so it could be:
> 
> 1. Something wrong with the Anite test equipment configuration
> 2. Something wrong with QCT RIL
> 
> Either case, should not block 1.3 release. So 1.3-- first, Ming, please
> re-nominate again if you find anything strange in the logs

In a GCF scenario the network does not respond to incorrect requests whereas a live network may try to make a best effort.  GCF has shown the device to break the spec and we should do our best to conform as carriers only allow a certain number of failures.

This GCF test case continues to fail.  The fix could be to add a endConference method to the nsTelephonyService API.

Comment 4

[Andrew Overholt [:overholt]]

CAF p2 = 2.1+

Comment 5

[bhavana bajaj [:bajaj]]

Ni : ken to get started here from Moz to investigate this.. 

In parallel, CAF is trying to get info on how did we pass certification on prior releases and this becoming a blocker for 2.1. Was something changed in telephony platform on Moz or CAF side that is raising this ? Anshul can you also comment on comment #2 about the validity of this issue in production env?

Comment 6

[Michael Schwartz [:m4]]

(In reply to bhavana bajaj [:bajaj] from comment #5)
> In parallel, CAF is trying to get info on how did we pass certification on
> prior releases and this becoming a blocker for 2.1.

Good question.  Seems like there was a coverage hole in prior runs.

Comment 7

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

Hi Michael,

In current FxOS design, as you understand, there's no specific API for releasing a conference call. Dialer app is releasing individual calls in a conference instead. However, referring to the steps #2 and #3 of 31.4.2.1.3, two separate messages are sent to SS in any order. It looks our current design of sending individual hangup commands is acceptable, no? Please let me know if I am getting something wrong, and please point me where/in which step the current FxOS design fails to help have a right solution, thank you!

Comment 8

[Michael Schwartz [:m4]]

You're right.  The current API should conform to the test and I think I've found the issue.  I'll verify with the tech teams and get back to you.  Thank you!

Comment 9

[Michael Schwartz [:m4]]

Looks like the lower layers assume behaviour as per 27.007 (or 07.07) where a specific message to end all calls would be used ie AT+CHUP.  It seems reasonable to enable these messages as this is closer to the UI/user intent in some cases.  Hsin-Yi, please let me know what you think.

Comment 10

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

Thanks for the reply, Michael.

(In reply to Michael Schwartz [:m4] from comment #9)
> Looks like the lower layers assume behaviour as per 27.007 (or 07.07) where
> a specific message to end all calls would be used ie AT+CHUP.  

Is this behaviour documented as standard or de facto standard? I am still concerned how this GCF test passed for previous releases but not this time. Has the behaviour somehow been changed?

> It seems
> reasonable to enable these messages as this is closer to the UI/user intent
> in some cases.  Hsin-Yi, please let me know what you think.

If the above behaviour is standard, then to empower fxos, it is reasonable to introduce a single specific message to hang up all calls at a time. However, I still think that message is not necessary from the UI point of view as it's proven the current API design works quite good.

Last concern is, from AOSP ril.h, I don't see which parcel request is designed for this capability. Are we going to add a new parcel request for Flame as well?

Comment 11

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> I am still concerned how this GCF test passed for previous releases but not this time.
> Has the behaviour somehow been changed?

No, just a coverage hole in our testing.

> Is this behaviour documented as standard or de facto standard?
> 
> If the above behaviour is standard, then to empower fxos, it is reasonable
> to introduce a single specific message to hang up all calls at a time.
> However, I still think that message is not necessary from the UI point of
> view as it's proven the current API design works quite good.
> 
> Last concern is, from AOSP ril.h, I don't see which parcel request is
> designed for this capability. Are we going to add a new parcel request for
> Flame as well?

It's the de facto standard as you can see that is the behaviour in AOSP.  Please excuse a correction: it's not a AT+CHUP but actually AT+CHLD=1 which is available in ril.h.  TelephonyCallGroup already has hold and resume methods so an end method wouldn't be out of place.

Comment 12

[Michael Schwartz [:m4]]

Note, you'll probably need to add support for this anyway as part of the unify dial/mmi bug.

Comment 13

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

(In reply to Michael Schwartz [:m4] from comment #11)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > I am still concerned how this GCF test passed for previous releases but not this time.
> > Has the behaviour somehow been changed?
> 
> No, just a coverage hole in our testing.
> 
> > Is this behaviour documented as standard or de facto standard?
> > 
> > If the above behaviour is standard, then to empower fxos, it is reasonable
> > to introduce a single specific message to hang up all calls at a time.
> > However, I still think that message is not necessary from the UI point of
> > view as it's proven the current API design works quite good.
> > 
> > Last concern is, from AOSP ril.h, I don't see which parcel request is
> > designed for this capability. Are we going to add a new parcel request for
> > Flame as well?
> 
> It's the de facto standard as you can see that is the behaviour in AOSP. 
> Please excuse a correction: it's not a AT+CHUP but actually AT+CHLD=1 which
> is available in ril.h.  TelephonyCallGroup already has hold and resume
> methods so an end method wouldn't be out of place.

Okay... per ril.h, we take advantage of RIL_REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND (AT+CHLD=1). Also, we need a new API telephonyCallGroup.hangUp(), and gaia work is required as well. Sounds reasonable?

(In reply to Michael Schwartz [:m4] from comment #12)
> Note, you'll probably need to add support for this anyway as part of the
> unify dial/mmi bug.

We already have hangUpAll() in ril_worker.js. We don't necessarily need a new API for this.

Comment 14

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

Adding Dialer developers in cc as gaia work might be needed, see comment 13.

Comment 15

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

(In reply to Michael Schwartz [:m4] from comment #11)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > I am still concerned how this GCF test passed for previous releases but not this time.
> > Has the behaviour somehow been changed?
> 
> No, just a coverage hole in our testing.
Hi Michael,
Just want to be more sure whether this should block or not at this moment.
We are hitting FC now. 

> 
> > Is this behaviour documented as standard or de facto standard?
> > 
> > If the above behaviour is standard, then to empower fxos, it is reasonable
> > to introduce a single specific message to hang up all calls at a time.
> > However, I still think that message is not necessary from the UI point of
> > view as it's proven the current API design works quite good.
> > 
> > Last concern is, from AOSP ril.h, I don't see which parcel request is
> > designed for this capability. Are we going to add a new parcel request for
> > Flame as well?
> 
> It's the de facto standard as you can see that is the behaviour in AOSP. 
> Please excuse a correction: it's not a AT+CHUP but actually AT+CHLD=1 which
> is available in ril.h.  TelephonyCallGroup already has hold and resume
> methods so an end method wouldn't be out of place.

Comment 16

[Anshul]

(In reply to Wesley Huang [:wesley_huang] from comment #15)
> (In reply to Michael Schwartz [:m4] from comment #11)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > > I am still concerned how this GCF test passed for previous releases but not this time.
> > > Has the behaviour somehow been changed?
> > 
> > No, just a coverage hole in our testing.
> Hi Michael,
> Just want to be more sure whether this should block or not at this moment.
> We are hitting FC now. 
Hey Wesley, yes this is considered blocking for this release.

> 
> > 
> > > Is this behaviour documented as standard or de facto standard?
> > > 
> > > If the above behaviour is standard, then to empower fxos, it is reasonable
> > > to introduce a single specific message to hang up all calls at a time.
> > > However, I still think that message is not necessary from the UI point of
> > > view as it's proven the current API design works quite good.
> > > 
> > > Last concern is, from AOSP ril.h, I don't see which parcel request is
> > > designed for this capability. Are we going to add a new parcel request for
> > > Flame as well?
> > 
> > It's the de facto standard as you can see that is the behaviour in AOSP. 
> > Please excuse a correction: it's not a AT+CHUP but actually AT+CHLD=1 which
> > is available in ril.h.  TelephonyCallGroup already has hold and resume
> > methods so an end method wouldn't be out of place.

Comment 17

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13)
> Okay... per ril.h, we take advantage of
> RIL_REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND (AT+CHLD=1). Also, we need a
> new API telephonyCallGroup.hangUp(), and gaia work is required as well.
> Sounds reasonable?

Yup, sounds perfect.

> We already have hangUpAll() in ril_worker.js. We don't necessarily need a
> new API for this.

Ah, gotcha.

Comment 18

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13)
> Okay... per ril.h, we take advantage of
> RIL_REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND (AT+CHLD=1). Also, we need a
> new API telephonyCallGroup.hangUp(), and gaia work is required as well.
We need to file a gaia bug, right?
Hi Wesley, I think you need to bring this to comms team.

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add hangUp method (webidl)

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Add hangUp method (dom)

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: Add hangUpConference method (internal interface)

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: Add hangUpConference method (ipc)

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: Add hangUpConference method (ril)

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6: Add test case

Comment 25

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

setting target milestone per the ETA 10/17.

Comment 26

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

Comment on attachment 8505324 [details] [diff] [review]
Part 1: Add hangUp method (webidl)

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

Given there's already a request for returning a error response to gaia (Bug 1077075), let's return a promise for this new API. Sorry for not mentioning this earlier.

Comment 27

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#2: Add hangUp method (webidl)

It's promise version

Comment 28

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Add hangUp method (dom)

Comment 29

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#2: Add hangUpConference method (internal interface)

Comment 30

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#2: Add hangUpConference method (ipc)

Comment 31

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#2: Add hangUpConference method (ril)

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 6#2: Add test case

Comment 33

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

Comment on attachment 8506848 [details] [diff] [review]
Part 3#2: Add hangUpConference method (internal interface)

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

::: dom/telephony/nsITelephonyService.idl
@@ +295,5 @@
>    void resumeCall(in unsigned long clientId, in unsigned long callIndex);
>  
>    void conferenceCall(in unsigned long clientId);
>    void separateCall(in unsigned long clientId, in unsigned long callIndex);
> +  void hangUpConference(in unsigned long clientId, 

nit: trailing w.s.

Comment 34

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

Comment on attachment 8506847 [details] [diff] [review]
Part 2#2: Add hangUp method (dom)

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

r=me with comment addressed, thank you!

::: dom/telephony/TelephonyCallGroup.cpp
@@ +291,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +
> +  nsCOMPtr<nsITelephonyCallback> callback = new TelephonyCallback(promise);
> +  aRv = mTelephony->Service()->HangUpConference(mCalls[0]->ServiceId(),

Don't we need Promise.reject in a case of failure?

Comment 35

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

Comment on attachment 8506852 [details] [diff] [review]
Part 6#2: Add test case

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

Thank you thank you!

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#3: Add hangUp method (dom)

Instead of returning a rejected promise, I think we can just return null.

Comment 37

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

Comment on attachment 8507639 [details] [diff] [review]
Part 2#3: Add hangUp method (dom)

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

Looks good to me, too.

Comment 38

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 1: Add hangUp method (webidl). r=hsinyi

Comment 39

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 2: Add hangUp method (dom). r=hsinyi

Comment 40

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 3: Add hangUpConference method (internal interface). r=hsinyi

Comment 41

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 4: Add hangUpConference method (ipc). r=hsinyi

Comment 42

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 5: Add hangUpConference method (ril). r=hsinyi

Comment 43

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 6: Add test case. r=hsinyi

Comment 44

[Szu-Yu Chen [:aknow] (inactive)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2c6c0817c022

Comment 45

[Ryan VanderMeulen [:RyanVM]]

webidl changes need DOM peer review

Comment 46

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8507781 [details] [diff] [review]
[final] Part 1: Add hangUp method (webidl). r=hsinyi

Hi Kyle,
May I have your review for the webidl part? Thank you.

Comment 47

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8507781 [details] [diff] [review]
[final] Part 1: Add hangUp method (webidl). r=hsinyi

Cancel the review. Hsinyi's review should be OK.
https://www.mail-archive.com/[e-mail]/msg11294.html

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #45)
> webidl changes need DOM peer review

Hsin-Yi's review is sufficient for Telephony/RIL/etc stuff.  See https://groups.google.com/forum/#!topic/mozilla.dev.platform/DYlmmbWyOJ8.

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/48b004974cd2
https://hg.mozilla.org/integration/b2g-inbound/rev/a5c28b69a261
https://hg.mozilla.org/integration/b2g-inbound/rev/b64604a829ff
https://hg.mozilla.org/integration/b2g-inbound/rev/b20c4afb0c7e
https://hg.mozilla.org/integration/b2g-inbound/rev/d4a7883e0e92
https://hg.mozilla.org/integration/b2g-inbound/rev/0964acec6851

Comment 50

[Carsten Book [:Tomcat]]

sorry had to backout this changes for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=670274&repo=b2g-inbound

Comment 51

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 2#2: Add hangUp method (dom). r=hsinyi

Comment 52

[Szu-Yu Chen [:aknow] (inactive)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=58097f2cadca

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5bd0c9eec7d6
https://hg.mozilla.org/integration/b2g-inbound/rev/bc96cff67ac9
https://hg.mozilla.org/integration/b2g-inbound/rev/d8248fc099f8
https://hg.mozilla.org/integration/b2g-inbound/rev/fb2b6629a57c
https://hg.mozilla.org/integration/b2g-inbound/rev/eb6ead190d74
https://hg.mozilla.org/integration/b2g-inbound/rev/2cdf87a14e8c

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bd0c9eec7d6
https://hg.mozilla.org/mozilla-central/rev/bc96cff67ac9
https://hg.mozilla.org/mozilla-central/rev/d8248fc099f8
https://hg.mozilla.org/mozilla-central/rev/fb2b6629a57c
https://hg.mozilla.org/mozilla-central/rev/eb6ead190d74
https://hg.mozilla.org/mozilla-central/rev/2cdf87a14e8c

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Please request b2g34 approval on these patches when you get a chance :)

Comment 56

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 7: Fix a bug

Sorry....... just found a bug

Comment 57

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

Comment on attachment 8510212 [details] [diff] [review]
Part 7: Fix a bug

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

Thanks for the catch.

Comment 58

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 7: Fix a bug. r=hsinyi

Comment 59

[Szu-Yu Chen [:aknow] (inactive)]

Please help me land part 7
Thank you

Comment 60

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

https://hg.mozilla.org/integration/b2g-inbound/rev/b038fb6537e1  part7

Comment 61

[Szu-Yu Chen [:aknow] (inactive)]

ni myself for b2g34 patches

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b038fb6537e1

Comment 63

[Tamara Hills [:thills]]

Attachment: Uplift request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
Note that this should be landed before bug 1081854 as there is a dependency.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): Found by gap in test coverage
User impact if declined: I believe this is requested by a partner.
Testing completed: yes - patch includes unit tests
Risk to taking this patch (and alternatives if risky): low/medium
String or UUID changes made by this patch:

Comment 64

[bhavana bajaj [:bajaj]]

(In reply to Michael Schwartz [:m4] from comment #11)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > I am still concerned how this GCF test passed for previous releases but not this time.
> > Has the behaviour somehow been changed?
> 
> No, just a coverage hole in our testing.
> 
> > Is this behaviour documented as standard or de facto standard?
> > 
> > If the above behaviour is standard, then to empower fxos, it is reasonable
> > to introduce a single specific message to hang up all calls at a time.
> > However, I still think that message is not necessary from the UI point of
> > view as it's proven the current API design works quite good.
> > 
> > Last concern is, from AOSP ril.h, I don't see which parcel request is
> > designed for this capability. Are we going to add a new parcel request for
> > Flame as well?
> 
> It's the de facto standard as you can see that is the behaviour in AOSP. 
> Please excuse a correction: it's not a AT+CHUP but actually AT+CHLD=1 which
> is available in ril.h.  TelephonyCallGroup already has hold and resume
> methods so an end method wouldn't be out of place.

:m4/Inder once this patch lands, can you please help with verification on your side confirming this fixes the issue you reported? Thanks!

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d81ecd0478bc
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3968fe833f9f
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/eb23dfa303a4
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/03d92424a02e
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/12ecc76c487f
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4e4459c95a51
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b5c384119e53

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b0182ac643ea

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=14152&repo=mozilla-b2g34_v2_1

Comment 67

[Michael Schwartz [:m4]]

What's the status here?  We need the fix for 2.1.  Should we reopen the issue?

Comment 68

[Michael Schwartz [:m4]]

(In reply to bhavana bajaj [:bajaj] from comment #64)
> :m4/Inder once this patch lands, can you please help with verification on
> your side confirming this fixes the issue you reported? Thanks!

Yes, we'll be integrating with this change anyway.

Comment 69

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 0: Adapting layer

Provide several changes as an adapting layer so that the patches could be applied on 2.1

Comment 70

[Josh Cheng [:josh]]

Hi Szu-Yu,

Partner also reported same issue on 2.0M. 
Is the patch for 2.1 also good for 2.0M?

Comment 72

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

Comment on attachment 8512446 [details] [diff] [review]
Part 0: Adapting layer

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

The code in m-c and v2.1 diverges a lot... Thanks for the great effort to prepare this 2.1-branch patch.

::: dom/telephony/TelephonyDialCallback.cpp
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "TelephonyDialCallback.h"
> +
> +#include "mozilla/dom/DOMMMIError.h"

We don't need this.

@@ +4,5 @@
> +
> +#include "TelephonyDialCallback.h"
> +
> +#include "mozilla/dom/DOMMMIError.h"
> +#include "nsServiceManagerUtils.h"

don't need this, either.

@@ +30,5 @@
> +                                             const nsAString& aNumber)
> +{
> +  nsRefPtr<TelephonyCallId> id = mTelephony->CreateCallId(aNumber);
> +  nsRefPtr<TelephonyCall> call =
> +      mTelephony->CreateCall(id, mServiceId, aCallIndex,

nit: indention

Comment 73

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Josh Cheng [:josh] from comment #70)
> Hi Szu-Yu,
> 
> Partner also reported same issue on 2.0M. 
> Is the patch for 2.1 also good for 2.0M?

I am afraid not. Let me provide the version for 2.0M later.

Comment 74

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 0: Adapting layer. r=hsinyi

Comment 75

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 1: Add hangUp method (webidl). r=hsinyi

Comment 76

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 2: Add hangUp method (dom). r=hsinyi

Comment 77

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 3: Add hangUpConference method (internal interface). r=hsinyi

Comment 78

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 4: Add hangUpConference method (ipc). r=hsinyi

Comment 79

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 5: Add hangUpConference method (ril). r=hsinyi

Comment 80

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.1) [final] Part 6: Add test case. r=hsinyi

Comment 81

[Szu-Yu Chen [:aknow] (inactive)]

2.1 try
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ea2b4d449bb

Comment 82

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) Part 0: Adapting layer

Comment 83

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

Comment on attachment 8513218 [details] [diff] [review]
(2.0m) Part 0: Adapting layer

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

r=me with comment addressed, thank you!

::: dom/telephony/gonk/TelephonyService.js
@@ +477,5 @@
>          return false;
>        }
>  
>        if (response.isCdma) {
> +        onCdmaDialSuccess.call(this, response.callIndex, response.number);

In the case of 'cdma 3-way call'  there's no response.callIndex coming from ril_worker when we dial out the 2nd call. Please correct this, thank you.

Comment 84

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 0: Adapting layer. r=hsinyi

Hsinyi,
I remember that we can avoid the warning by this kind of fix.

Comment 85

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 1: Add hangUp method (webidl). r=hsinyi

Comment 86

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 2: Add hangUp method (dom). r=hsinyi

Comment 87

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 3: Add hangUpConference method (internal interface). r=hsinyi

Comment 88

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 4: Add hangUpConference method (ipc). r=hsinyi

Comment 89

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 5: Add hangUpConference method (ril). r=hsinyi

Comment 90

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [final] Part 6: Add test case. r=hsinyi

Comment 91

[Szu-Yu Chen [:aknow] (inactive)]

2.0m try
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c8bc787b7aa2

Comment 92

[Szu-Yu Chen [:aknow] (inactive)]

check-in needed,
 2.1: part 0 ~ part 6
2.0m: part 0 ~ part 6

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/464149f4a6dc
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/bd428aee6324
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/c25c6be5f625
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/051e2099f812
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2a7fa1773415
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/abc72ad593f7
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/29d0bdb98ed5

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e2dcec45fb56
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/dd43047ad566
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/486aa36c827f
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/96d77d1d9f3e
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/4ce3973866ba
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/65cb222c1eee
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/66d41be7c319

Comment 94

[Anshul]

(In reply to Szu-Yu Chen [:aknow] from comment #69)
> Created attachment 8512446 [details] [diff] [review]
> Part 0: Adapting layer
> 
> Provide several changes as an adapting layer so that the patches could be
> applied on 2.1

By adding this adapting layer, you have increased the scope of this bug unnecessarily. The code change is much bigger now and risky for 2.1 so close to QC CS.

Comment 95

[Coler]

This issue has been verified successfully on Woodduck 2.0; Flame2.1&2.2.
Reproducing rate: 0/5
See attachment: Verify_Woodduck_MultiPartycall.mp4

Woodduck build version:
Gaia-Rev        d742e375aca6dc1bf3a36638000ad7f5338ef457
Gecko-Rev       d049d4ef127844121c9cf14d2e8ca91fd9045fcb
Build-ID        20141127050313
Version         32.0

Flame2.1 build version:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0

Flame 2.2 build version:
Gaia-Rev        41b7be7c67167f367c3c4982ff08651d55455373
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/ff4a63d66806
Build-ID        20141126160201
Version         36.0a1

Comment 96

[Coler]

Attachment: Verify_Woodduck_MultiPartycall.MP4