998147 - [Dialer] A Missed Call notification is given when chosing to hang up on an incoming call

Status: VERIFIED FIXED

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

Description

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Attachment: logcat_20140417_1731_not_missed.txt

Description:
If the user presses the red ignore button when receiving a call, a missed call notification will appear.
On similar android devices, no notification occurs.

Repro Steps:
1) Update a Tarako to BuildID: 20140417004002
2) Call the phone with another device
3) Choose to hang up the call when prompted to answer.
4) Note that a missed call notification occurs.

Actual:
A missed call notification is received.

Expected:
No notification occurs as the user has chosen not to receive the call.

1.3T Environmental Variables:
Device: Tarako 1.3T MOZ
BuildID: 20140417004002
Gaia: a8d2d399f2939f4845abaa0df57abab241a2c782
Gecko: d97dad54cb61
Version: 28.1
Firmware Version: sp6821

Repro frequency: 100%
See attached: Logcat, Firewatch report

Comment 1

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue does not occur on 1.3 Buri.

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140417004003
Gaia: 11d027ec28d8e8f09c76b35661222499e124abc8
Gecko: 69851ef3849c
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 2

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Attachment: firewatch_20140417_1731_not_missed.txt

Comment 3

[Jason Smith [:jsmith]]

Isn't this correct behavior? I would expect to have a missed call notification if I declined an incoming call.

UX - Can you clarify what the expected behavior here is?

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note: The notification on buri doesn't seem to occur, having said that it is in the missed call log for buri.

Comment 5

[Stephany Wilkes]

Flagging Carrie to clarify expected Dialer behavior.

Comment 6

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

Hi all, I think since the user actively decline it on purpose, then we shouldn't display the missed call notification. However, we'll have the discussion of providing badges on the APP icons. Users can be notified in that way, but no notification for now. Thanks!

Comment 8

[Gabriele Svelto [:gsvelto]]

Setting more tracking flags as per bug 1021339, we should probably verify all versions again if we have time.

Comment 10

[Anthony Ricaud (:rik)]

Isn't this a regression of moving to a callscreen app (bug 990003 and specifically bug 992948) ?

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Anthony Ricaud (:rik) from comment #10)
> Isn't this a regression of moving to a callscreen app (bug 990003 and
> specifically bug 992948) ?

Good point, it could be since the two are now decoupled.

Comment 12

[Tamara Hills [:thills]]

Attachment: Gaia portion of WIP patch -- Gecko portion follows

Comment 13

[Tamara Hills [:thills]]

Attachment: Gecko portion of WIP patch

Hi Hsin-Yi,

Wanted to get your feedback for a WIP patch for this bug.  Basically I'm adding a new attribute to the telephony-call-ended event that tells whether the call was rejected or not.  This is needed because only callscreen app has that information and dialer app is the one that has the logic as to whether or not to notify the user of the missed call.  And the two don't have interfaces setup for this type of communication.  

I'm determining when the call is rejected in the rejectcall in the ril_worker.  

If you can let me know your thoughts on this approach, that would be great.  If this is ok approach, I still need to 1) see what is different for cdma here and 2) look at call waiting cases.

Thanks,
-tamara

Comment 14

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

(In reply to Tamara Hills [:thills] from comment #13)
> Created attachment 8452877 [details] [diff] [review]
> Gecko portion of WIP patch
> 
> Hi Hsin-Yi,
> 
> Wanted to get your feedback for a WIP patch for this bug.  Basically I'm
> adding a new attribute to the telephony-call-ended event that tells whether
> the call was rejected or not.  This is needed because only callscreen app
> has that information and dialer app is the one that has the logic as to
> whether or not to notify the user of the missed call.  And the two don't
> have interfaces setup for this type of communication.  
> 
> I'm determining when the call is rejected in the rejectcall in the
> ril_worker.  
> 
> If you can let me know your thoughts on this approach, that would be great. 
> If this is ok approach, I still need to 1) see what is different for cdma
> here and 2) look at call waiting cases.
> 
> Thanks,
> -tamara

Basically, I don't want to expose a new attribute to telephony-call-ended system message. I am concerned that we will be overusing the concept of system message which was designed for launching an app orignally.

For this case, I'd rather look for a way to exposing the information to telephony webapi. Currently we have telephonyCall.error and the event telephonyCall.onerror which is fired when a call is released abnormally or unexpectedly. 

I feel that .onerror is kinda duplicate with .ondisconnected and the semantic is not that right. For example, .onerror is fired with the error message "BusyError" when the remote party is too busy to answering the call. So I've been thinking about replacing |telephonyCall.error| with |telephonyCall.disconnectedReason| and removing .onerror. |HangUpLocally| could be one of the disconnected reasons. And the well-defined reasons, gaia could know if a call is a missed one or not.

The change makes more sense to me. How do you think?

Comment 15

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8452859 [details] [diff] [review]
Gaia portion of WIP patch -- Gecko portion follows

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

::: apps/communications/dialer/js/dialer.js
@@ +281,5 @@
>  
>      NavbarManager.ensureResources(function() {
>        // Missed call
>        if (incoming && !data.duration) {
> +        if(!reject) {

Actually you can use |if (data.rejected)| directly here. In JavaScript if the |rejected| field is undefined it will evaluate as false when put in a boolean expression.

Comment 16

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to Tamara Hills [:thills] from comment #13)
> > Created attachment 8452877 [details] [diff] [review]
> > Gecko portion of WIP patch
> > 
> > Hi Hsin-Yi,
> > 
> > Wanted to get your feedback for a WIP patch for this bug.  Basically I'm
> > adding a new attribute to the telephony-call-ended event that tells whether
> > the call was rejected or not.  This is needed because only callscreen app
> > has that information and dialer app is the one that has the logic as to
> > whether or not to notify the user of the missed call.  And the two don't
> > have interfaces setup for this type of communication.  
> > 
> > I'm determining when the call is rejected in the rejectcall in the
> > ril_worker.  
> > 
> > If you can let me know your thoughts on this approach, that would be great. 
> > If this is ok approach, I still need to 1) see what is different for cdma
> > here and 2) look at call waiting cases.
> > 
> > Thanks,
> > -tamara
> 
> Basically, I don't want to expose a new attribute to telephony-call-ended
> system message. I am concerned that we will be overusing the concept of
> system message which was designed for launching an app orignally.
> 
> For this case, I'd rather look for a way to exposing the information to
> telephony webapi. Currently we have telephonyCall.error and the event
> telephonyCall.onerror which is fired when a call is released abnormally or
> unexpectedly. 
> 
> I feel that .onerror is kinda duplicate with .ondisconnected and the
> semantic is not that right. For example, .onerror is fired with the error
> message "BusyError" when the remote party is too busy to answering the call.
> So I've been thinking about replacing |telephonyCall.error| with
> |telephonyCall.disconnectedReason| and removing .onerror. |HangUpLocally|
> could be one of the disconnected reasons. And the well-defined reasons, gaia
> could know if a call is a missed one or not.
> 
> The change makes more sense to me. How do you think?

Agreed. I personally think that |telephonyCall.disconnectedReason| is better than |error|.

Comment 17

[Tamara Hills [:thills]]

Hi Hsin-Yi,

I had originally looked at modifying the telephony call, but then figured out I could do the whole thing without changing the idl so I can look at this route.

I had one question about whether it's possible to get to the TelephonyCall object from the data in the call-ended-event.  If so, then the disconnectedReason could be accessed from that object.  If not, then probably we need to make changes to the events that Gaia receives and it might not be possible to handle both a reject from a user hangup and a normal hangup (due to voicemail) in the same function.

Thanks,

-tamara

Comment 18

[Anthony Ricaud (:rik)]

Hsin-Yi: We need to wake up the Dialer app in some way when a call is missed. How do you propose we do that if we move to events rather than system-messages?

Comment 19

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

(In reply to Anthony Ricaud (:rik) from comment #18)
> Hsin-Yi: We need to wake up the Dialer app in some way when a call is
> missed. How do you propose we do that if we move to events rather than
> system-messages?

Oh, when I stated, I was not aware of the fact that Dialer app isn't launched when there comes an incoming call. :(

Reconsidering the current structure, we could
1) Add a new attribute in telephony-call-ended system message as Tamara proposed. 
2) Keep using telephony-call-ended message to launch Dialer but not add a new attribute in this case. But how could we guarantee Dialer still gets the call log later? The only solution I could come out with is taking advantage of DataStore API. And the call log data store should be created by System or TelephonyService as opposed to Dialer app. 

I am trying to not abuse system messages but I am not even sure if the benefit 2) offers far outweighs the refactoring effort... So 1) seems okay for me now.

Comment 21

[Tamara Hills [:thills]]

Attachment: Updated Gecko Patch

Hi Hsin-Yi,

This is an update to the earlier Gecko patch I posted for this.  I thought I can make use of the existing "hangUpLocal" attribute used in ril_worker.js instead of introducing a new one.

Thanks,

-tamara

Comment 22

[Tamara Hills [:thills]]

Attachment: 998147gaiapatch.diff

Hi Gabriele,

Can you review the Gaia portion of this change? I took your suggestion on the comment 15 as well.

Also, I'm not sure what the process is for a two-part patch like this.  I asked Hsin-Yi to review the Gecko portion and you to review the Gaia portion.  Let me know if I need to correct my approach here.

Thanks,

-tamara

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 8456027 [details] [diff] [review]
998147gaiapatch.diff

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

We did the review IRL.

Comment 24

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

Comment on attachment 8456021 [details] [diff] [review]
Updated Gecko Patch

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

::: dom/telephony/gonk/TelephonyService.js
@@ +722,5 @@
>      aCall.clientId = aClientId;
>      aCall.state = nsITelephonyService.CALL_STATE_DISCONNECTED;
>      let duration = ("started" in aCall && typeof aCall.started == "number") ?
>        new Date().getTime() - aCall.started : 0;
> +    let rejected = ('hangUpLocal' in aCall && aCall.hangUpLocal) ?

|aCall.hangUpLocal ? true : false;| is enough.

@@ +731,5 @@
>        serviceId: aClientId,
>        emergency: aCall.isEmergency,
>        duration: duration,
> +      direction: aCall.isOutgoing ? "outgoing" : "incoming",
> +      rejected: rejected

I think we should use 'hangUpLocal' instead of 'rejected' since the semantic 'rejected' implies only the case that user doesn't accept the incoming call. And 'hangUpLocal' covers any case when user releases a call.

Or do you only care about the case of rejecting an incoming call?

Comment 25

[Tamara Hills [:thills]]

H(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> Comment on attachment 8456021 [details] [diff] [review]
> Updated Gecko Patch
> 
> Review of attachment 8456021 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> @@ +731,5 @@
> >        serviceId: aClientId,
> >        emergency: aCall.isEmergency,
> >        duration: duration,
> > +      direction: aCall.isOutgoing ? "outgoing" : "incoming",
> > +      rejected: rejected
> 
> I think we should use 'hangUpLocal' instead of 'rejected' since the semantic
> 'rejected' implies only the case that user doesn't accept the incoming call.
> And 'hangUpLocal' covers any case when user releases a call.
> 
> Or do you only care about the case of rejecting an incoming call?

Hi Hsin-Yi,
I think that makes sense to call it hangUpLocal since it's the Gaia layer that is really figuring out if it's a rejected call (using the duration).  Secondly, might make it easier to keep the names consistent.  Also, there might be other reasons to know that the local party executed the hangup.. though I can't think of any off-hand.

Anthony, are you ok with calling it hangUpLocal instead of rejected in Gaia?

Thanks,
-tamara

Comment 26

[Anthony Ricaud (:rik)]

Yup. I also can't think of another way to use hangUpLocal for now but it makes more sense to me.

Hsin-Yi: About comment 19, let's do 1) for now. I'll discuss with Dialer people if it makes sense to move the call log to a datastore. But yeah, this is not a trivial refactor :)

Comment 27

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

(In reply to Anthony Ricaud (:rik) from comment #26)
> Yup. I also can't think of another way to use hangUpLocal for now but it
> makes more sense to me.
> 
> Hsin-Yi: About comment 19, let's do 1) for now. I'll discuss with Dialer
> people if it makes sense to move the call log to a datastore. But yeah, this
> is not a trivial refactor :)

Totally agree. Thank you!

Comment 28

[Tamara Hills [:thills]]

Attachment: Updated patch with parameter changed to hangUpLocal

Hi Hsin-Yi,

Updated patch to change the parameter name (also included commit message).

Thank you,

-tamara

Comment 29

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

Comment on attachment 8456764 [details] [diff] [review]
Updated patch with parameter changed to hangUpLocal

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

Thank you :)

Comment 30

[Tamara Hills [:thills]]

Attachment: Pull Request for Gaia Portion

Hi Anthony,
Here is the updated pull request from what we looked at last week.

Thanks,
-tamara

Comment 31

[Anthony Ricaud (:rik)]

Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

I've already co-authored this so redirecting the review to Doug.

Comment 32

[Tamara Hills [:thills]]

Checkin-needed on the *Gecko portion only*.  We need to "leave-open" so we can wait for the Gecko portion to land before landing the Gaia portion..

Comment 33

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/0fb3c6714933

Comment 34

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

LGTM. I left a few nit comments on the PR. Let me know if you'd like for me to land this.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0fb3c6714933

Comment 36

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/c9546cc0cbd3cbd8ce764507ab4759ab22f63eb6

Comment 39

[Anthony Ricaud (:rik)]

Doug: We have two duplicates for 2.0, we probably want to uplift this.

Comment 40

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8456764 [details] [diff] [review]
Updated patch with parameter changed to hangUpLocal

Please land on v2.0m only.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): None.
[User impact] if declined: Users will see a missed call notification when rejecting a call.
[Testing completed]: Has been on v2.1 and later for quite a while.
[Risk to taking this patch] (and alternatives if risky): Low.
[String changes made]: None.

Comment 41

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

See comment 40.

Comment 42

[Lingling Zhao]

Doug,

I try to add the patch attachment 8456764 [details] [diff] [review] into my V2.0 project, but it doesnot work. It looks like the file including the patch doesnot work because no logs printed. Can you please try to merge your patch into V2.0 and verify it is ok?

Thanks.

Comment 43

[Doug Sherk (:drs) (inactive)]

Redirecting to Tamara. Please see comment 42.

Comment 44

[Tamara Hills [:thills]]

Hi Lingling,

Did you apply both the gaia and gecko patches?  From reading, it looks like you might have just applied gecko.

Thanks,

-tamara

Comment 45

[Lingling Zhao]

I apply both the gaia and gecko patches, and add some logs in gecko, but it doesnot work.

Comment 46

[Tamara Hills [:thills]]

Hi Lingling,

I tested this out on top of gecko 2.0 and gaia 2.0 and it works fine for me.  Just to check, I added some logging in gaia to check that indeed the hangUpLocal parameter is getting set.  

I'm using a v188 base image for flame.  So, that got me the 2.0 Firefox OS.  Then I build gecko2.0m (added the hangUpLocal) and build gaia2.0m (and added the extra check).

If you still can't get it to work can you 1) send me the copies of the telephonyservice.js and dialer.js after the patches have been applied and 2) let me know how you applied the patches so I can try the exact same thing.

Thanks,

-tamara

Comment 47

[Lingling Zhao]

Attachment: js.zip

Sorry for reply so late, I just come back.
Please see the two files with the patch in the attachment. I download a new build with the patch into the test device, and make a call to it, hangup the call in the test device side, then the notification displays.

Comment 48

[Tamara Hills [:thills]]

Hi Lingling,

I only saw one file (TelephonyService.js) in the .zip attachment.  Are you sure the changes to Gaia in dialer.js got applied as well?  Without that, it will not work.

If you can send me the dialer.js file as well, just to make sure it got applied, that will be great.

If you're still having problems after making sure you have both gecko and gaia applied:
Can you also let me know what build you are using and point me to that?  Then I can make sure I have the exact build that you are applying on top of (including base image), then apply the changes and test it out.    I want to make sure I'm trying exactly what you're trying.

Thanks,

-tamara

Comment 49

[Lingling Zhao]

Attachment: js.zip

Sorry to forget the file, please see them in the new attachment.
I am sure I applied both of them,  the build ID is FFOS2.0 baseline BuildID: 20140916000205.

Thanks for your support.

Comment 50

[Lingling Zhao]

Tamara,

Do you have any update for it?
Thanks.

Comment 51

[Tamara Hills [:thills]]

Attachment: Pull request for 2.0

Comment 52

[Tamara Hills [:thills]]

Attachment: Gecko portion on 2.0

Comment 53

[Tamara Hills [:thills]]

Hi Johan,

Would you be able to verify the last two attachments for 2.0?  

Thanks,

-tamara

Comment 54

[Lingling Zhao]

Johan,

Can you please provide the result?
Thanks.

Comment 55

[Johan Lorenzo [:jlorenzo]]

I tested this revision[1] without applying the patch. If I hang up while I'm prompted to answer or not, a notification pops afterwards.

After applying attachment 8520625 [details] [review] and attachment 8520632 [details] [diff] [review] and rebuilding it, no notification appears.

[1] Gaia-Rev        ab83632c92f9fc571b11d8468b6901cc4ed905c0
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e21bf45e6c44
Build-ID        20141113000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.jlorenzo.20141002.104456
FW-Date         Thu Oct  2 10:45:09 CEST 2014
Bootloader      L1TC0001188

Comment 56

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8520625 [details] [review]
Pull request for 2.0

To make comment 55 more clear, this patches fixes the issue on 2.0.

Comment 57

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8520632 [details] [diff] [review]
Gecko portion on 2.0

To make comment 55 more clear, this patches fixes the issue on 2.0.

Comment 58

[bhavana bajaj [:bajaj]]

Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

PLease ensure to verify on 2.0 post landing.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4defa91c67fb

v2.0: https://github.com/mozilla-b2g/gaia/commit/a4fa2a996b3c73b1cf9778e0e29b5675cb351e25

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/4defa91c67fb

Comment 61

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.1.

Result: No "missed call" notification appears after the user rejects an incoming call.

Device: Flame 2.1 (319mb, KK, Shallow Flash)
BuildID: 20141118001204
Gaia: 1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko: 95fbd7635152
Version: 34.0 (2.1) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

==========================================================
This issue still reproduces on today's Flame 2.0 nightly. I'll check back again tomorrow.

Comment 62

[Yeojin Chung [:YeojinC]]

This issue still reproduces on Flame 2.0.

Result: "Missed call" notification appears after the user rejects an incoming call.

Device: Flame 2.0 (319mb, KK, Shallow Flash)
Build ID: 20141119000207
Gaia: 1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gecko: faa64077b0c2
Version: 32.0 (2.0)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 63

[Coler]

Verify passed, this issue can't be repro on Woodduck 2.0.
Attached: Verify_Woodduck_Misscall.mp4
Reproducing rate: 0/5

Woodduck build:
Gaia-Rev        3a98f1287fa7b604891220ba5d86982ae8f9971e
Gecko-Rev       03d3ab62d5b07b915434f2d1d68495ad5915ecd2
Build-ID        20141120103003
Version         32.0

Comment 64

[Coler]

Attachment: Verify_Woodduck_Misscall.MP4