876335 - [Buri][Call]The first call number dispaly error

Status: RESOLVED FIXED

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

Description

[sync-1]

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.111
 Firefox os  v1.0.1
 Mozilla build ID:20130518070208
 
 +++ This bug was initially created as a clone of Bug #458334 +++
 
 Created an attachment (id=417292)
 step-3
 
 DEFECT DESCRIPTION:
 The first call number dispaly error
 
  REPRODUCING PROCEDURES:
 1.add two phone numbers into the Contacts(for example A is 02988320198, B is 18706895844) 
 2.enable the call waiting 
 3.dial a call using A -> it is display "mobile, 02988320198" under the "A"
 4.incoming a call from B(18706895844) -> answer the call 
 5.hang up the second call(B) -> it is display "mobile, 18706895844" 
 under the "A" ==> KO.
 please refer the snapshot.
 
  EXPECTED BEHAVIOUR:
 5.it should display "mobile,02988320198"
 
 ASSOCIATE SPECIFICATION:
 TEST PLAN REFERENCE:
 TOOLS AND PLATFORMS USED:
 USER IMPACT:
 REPRODUCING RATE:5/5
 For FT PR, Please list reference mobile's behavior:
 
 Li Xin
 Tel:[phone]
 
 ++++++++++ end of initial bug #458334 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Clone from brother

Comment 2

[sync-1]

Attachment: step-5

Clone from brother

Comment 3

[sync-1]

Clone from brother

Comment 4

[sync-1]

Attachment: step-3

Clone from brother

Comment 5

[Mingming ZHAO]

Attachment: please review the patch

Please review the patch.

Comment 6

[Mingming ZHAO]

Dears,

How about this issue? Whether to fix it?

Comment 7

[Daniel Coloma:dcoloma]

I cannot reproduce it, in step 5:

"5.hang up the second call(B) -> it is display "mobile, 18706895844" 
 under the "A" ==> KO."

where I should hang up, in the DUT, in B...?

Can you please try to reproduce again?

Comment 8

[sync-1]

Comment from Mozilla:I cannot reproduce it, in step 5:
 
 "5.hang up the second call(B) -> it is display "mobile, 18706895844" 
  under the "A" ==> KO."
 
 where I should hang up, in the DUT, in B...?
 
 Can you please try to reproduce again?

Comment 9

[Alex Keybl [:akeybl]]

Given the user impact, we're strongly considering punting to v1.1. Waiting for followup from [e-mail] on reproduction.

Comment 10

[sync-1]

Comment from Mozilla:Given the user impact, we're strongly considering punting to v1.1. Waiting for followup from [e-mail] on reproduction.

Comment 11

[sync-1]

 5.hang up the second call(B) in the DUT -> it is display "mobile, 18706895844" 
  under the "A" ==> KO.

Comment 12

[Daniel Coloma:dcoloma]

I can't still reproduce it, qawanted to check if you can do it.

Comment 13

[Al Tsai [:atsai]]

I cannot reproduce that on unagi v1.0.1.

Target build: Mozilla RIL on unagi v1.0.1
BUILDID: 20130530070213
gaia:   ac293ce59
gecko:d0bae83a0

Comment 14

[sync-1]

Created an attachment (id=423799)
 snapshot

Comment 15

[sync-1]

Maybe you can follow the steps to repreduce:
 
 1.prepare two phone numbers(02988320198, 18706895844), add one into the Contacts(for exapmle: save the number 18706895844 as "Test") 
 2.enable the call waiting 
 3.use the DuT dial the number 02988320198 -> it is display "02988320198"
 4.incoming a call from 18706895844 -> answer the call 
 5.hang up the call(18706895844) in the DuT -> it is display "mobile, 18706895844" 
 under the "02988320198" ==> KO.
 
 Please refer to the attachments.

Comment 16

[sync-1]

Created an attachment (id=423802)
 snapshot

Comment 17

[Mingming ZHAO]

Attachment: call waiting

Comment 18

[Mingming ZHAO]

Attachment: hung up the "Test"

Comment 19

[Mingming ZHAO]

Dears,

Whether the new STR can reprodece?
 
 1.prepare two phone numbers(02988320198, 18706895844), add one into the Contacts(for exapmle: save the number 18706895844 as "Test") 
 2.enable the call waiting 
 3.use the DuT dial the number 02988320198 -> it is display "02988320198"
 4.incoming a call from 18706895844 -> answer the call 
 5.hang up the call(18706895844) in the DuT -> it is display "mobile, 18706895844" 
 under the "02988320198" ==> KO.
 
 Please refer to the attachments.

Comment 20

[sync-1]

So, how about this issue now?

Comment 21

[Daniel Coloma:dcoloma]

qawanted to check this with new STR

Comment 22

[Mingming ZHAO]

In addition, if you hang up the first the call(02988320198) in the DuT first, it will display nothing under the "Test".

I think this is because of the code: https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/communications/dialer/js/oncall.js#L13
It is be executed when the oncall.html is loaded, and retrun the first call's elements (without attribute of ".held").

If the second call is connected, the oncall.html has been loaded, so the code will not be executed. In addition, when the second call is connected, the sencond call's elements will be added the attribute of ".held", see: https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/communications/dialer/js/handled_call.js#L66
So, even if the code (https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/communications/dialer/js/oncall.js#L13) will be executed, it will not retrun the second call's element.

When the code : https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/communications/dialer/js/handled_call.js#L131 will gets the sencond call's addtional info (mobile, 18706895844). Then call "KeypadManager.updateAdditionalContactInfo(additionalInfo)" to update the additional info. But the code: https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/communications/dialer/js/keypad.js#L548
always get the first call's ".additionalContactInfo" element. So it will add the sencond call's additional info to the first call's.

What do you think?

Comment 23

[buri.blff]

Attachment: snapshot

Dears,

I also can reproduce this on Sumsung galaxy 2 with mozilla's code.Please refer to the attachment.

My operation:
1.Add the number 02988868024 as "Rose" into contacts in the DuT.
2.Dial a call 10010(without add into the contacts) in the DuT.(the top picture)
3.Use anther phone(its number is 02988868024) dial the DuT,and answer the call.(the middle picture)
4.In DuT, hang up the call of "Rose". The number 02988868024 display under the "10010".(the bottom picture)

Comment 24

[Etienne Segonzac (:etienne)]

I can reproduce the issue, I'll try to reformulate the STR, it's a basic call waiting scenario:

- you need 2 phones, PhoneA (b2g), PhoneB (anything)
- create a contact on the PhoneA with the number of PhoneB
- from PhoneA, call any number, your voicemail will be just fine
- from PhoneB call PhoneA
- accept the call waiting on PhoneA (your voicemail call is put on held)
- hang up from PhoneB
- The voicemail call is resumed, but has the additional infos of your PhoneB contact

The regression was introduce by https://github.com/mozilla-b2g/gaia/commit/af10024d2f234447522819e75c080d2282c9f61c

German, since you're the author of that patch I'll let you comment.
But it's a really bad smell to have the KepadManager directly manipulate the DOM of the handledCall.
The design is based on the fact that the handledCall owns it's DOM.

Mingming, your patch does fix the issue but not the code design issue, I want to fix both :)

Comment 25

[Amelie Kong]

As it is can be reproduced, mark tef? again.

Comment 26

[Mingming ZHAO]

Attachment: patch

Dear Etienne,

How about this patch? :-)

Comment 27

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Etienne Segonzac (:etienne) from comment #24)
> I can reproduce the issue, I'll try to reformulate the STR, it's a basic
> call waiting scenario:
> 
> - you need 2 phones, PhoneA (b2g), PhoneB (anything)
> - create a contact on the PhoneA with the number of PhoneB
> - from PhoneA, call any number, your voicemail will be just fine
> - from PhoneB call PhoneA
> - accept the call waiting on PhoneA (your voicemail call is put on held)
> - hang up from PhoneB
> - The voicemail call is resumed, but has the additional infos of your PhoneB
> contact
> 
> The regression was introduce by
> https://github.com/mozilla-b2g/gaia/commit/
> af10024d2f234447522819e75c080d2282c9f61c
> 
> German, since you're the author of that patch I'll let you comment.
> But it's a really bad smell to have the KepadManager directly manipulate the
> DOM of the handledCall.
> The design is based on the fact that the handledCall owns it's DOM.
> 
> Mingming, your patch does fix the issue but not the code design issue, I
> want to fix both :)

Geez! "gtorodelvalle authored 7 months ago"... I do not even know if I was alive 7 months ago... :-p Kidding!

KeypadManager is kind of an special case since it is used from the dialer screen but also from the on call attention screen. Anyhow, I would totally in favor of some refactoring here. In fact, there are many |if (this._onCall)| and I guess what Etienne suggests is moving this code to handled_call.js (or to oncall.js's CallScreen) where it may be more suited. On the other hand, there is code which has little sense having in KeypadManager such as updateAdditionalContactInfo() and _updateAdditionalContactInfoView().  Is that what you had in mind, Etienne?

On the other hand, since this is a tef+ bug and Mingming may be not confortable facing this refactoring (if you are, great of course ;-)), I guess we have 2 options:
1) Pass the bug over to me. Right now the bug is not assigned, BTW ;-)
2) Using Mingming's patch to solve the problem and to open a new bug for this additional refactoring.

What do you think?

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 760284 [details] [diff] [review]
patch

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

We can't do this.
|CallScreen.activeCall| sould be a getter as it dynamically changes.

Comment 29

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #27)
> 1) Pass the bug over to me. Right now the bug is not assigned, BTW ;-)
> 2) Using Mingming's patch to solve the problem and to open a new bug for
> this additional refactoring.
> 
> What do you think?

The patch is breaking something else (CallScreen.activeCall wouldn't represent the call screen's active call anymore...).

We need a proper fix, if nobody is available I can take the bug. But a proper fix implies that the handledCall owns its DOM.

Comment 31

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! Is anyone working on this bug right now?

Comment 32

[Jason Smith [:jsmith]]

comment 24 has STR. Removing qawanted as such.

Comment 33

[Mingming ZHAO]

(In reply to gtorodelvalle from comment #31)
> Hi guys! Is anyone working on this bug right now?

I am working on it, but I have no idea how to resolve it, and my patchs have been rejected both. hehe..;-)

Comment 34

[Arthur Chen [:arthurcc][inactive after 6/30]]

I am working on the duplicate bug 877077. I'll refine my patch to address the design issue. HandledCall should be able to update its own DOM in different call states.

Comment 35

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/10310

Etienne, could you review the patch? Thanks!

KeypadManager should have its own node for displaying instead of trying to manipulate nodes owned by HandledCall. However, it is too risky to do the refactoring at this time. In this patch I simply added a new method for updating additional information. Suggest to create a follow-up issue for refactoring.

Comment 36

[Etienne Segonzac (:etienne)]

Comment on attachment 760779 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10310

We're getting there! Comments on github.

Comment 37

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 760779 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10310

Patch updated. Etienne, could you take a look at it again? Thanks!

Comment 38

[Etienne Segonzac (:etienne)]

Comment on attachment 760779 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10310

Thank you so much for carrying through this!
Much much better.

There are some minor nits on github, but r=me!

Comment 39

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment addressed. Etienne, thanks for reviewing!
master: https://github.com/mozilla-b2g/gaia/commit/ea92e6758ad1cfbda60d721017652deead91a788

Comment 40

[John Ford [:jhford] CET/CEST Berlin Time]

This bug was partially uplifted.

Uplifted ea92e6758ad1cfbda60d721017652deead91a788 to:
v1-train: b378e4b7e6b0b6bf2fc89347028eb7bab087cc8c

Commit ea92e6758ad1cfbda60d721017652deead91a788 didn't uplift to branch v1.0.1

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd b378e4b7e6b0b6bf2fc89347028eb7bab087cc8c

Comment 42

[Mingming ZHAO]

Dears,

Can you uplift this patch to branch v1.0.1?

Comment 43

[Arthur Chen [:arthurcc][inactive after 6/30]]

v1.0.1: https://github.com/mozilla-b2g/gaia/commit/7bca377199e7c73480a97f3c33d3ed9d5cbd26e3