Jayme Mercado [:JMercado] (Inactive 3/4/2016) Reporter
	Description • 10 years ago

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

	Jayme Mercado [:JMercado] (Inactive 3/4/2016) Reporter
	Comment 1 • 10 years ago

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

	Jason Smith [:jsmith]
	Comment 3 • 10 years ago

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?

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 4 • 10 years ago

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

	Stephany Wilkes
	Comment 5 • 10 years ago

Flagging Carrie to clarify expected Dialer behavior.

	Carrie Wang [New account: carrie75521@gmail.com]
	Comment 6 • 10 years ago

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!

	Gabriele Svelto [:gsvelto]
	Comment 8 • 10 years ago

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

	Anthony Ricaud (:rik)
	Comment 10 • 10 years ago

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

	Gabriele Svelto [:gsvelto]
	Comment 11 • 10 years ago

(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.

	Tamara Hills [:thills] Assignee
	Comment 13 • 10 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 14 • 10 years ago

(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?

	Gabriele Svelto [:gsvelto]
	Comment 15 • 10 years ago

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.

	Szu-Yu Chen [:aknow] (inactive)
	Comment 16 • 10 years ago

(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|.

	Tamara Hills [:thills] Assignee
	Comment 17 • 10 years ago

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

	Anthony Ricaud (:rik)
	Comment 18 • 10 years ago

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?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 19 • 10 years ago

(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.

	Tamara Hills [:thills] Assignee
	Comment 21 • 10 years ago

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

	Tamara Hills [:thills] Assignee
	Comment 22 • 10 years ago

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

	Anthony Ricaud (:rik)
	Comment 23 • 10 years ago

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

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

We did the review IRL.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 24 • 10 years ago

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?

	Tamara Hills [:thills] Assignee
	Comment 25 • 10 years ago

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

	Anthony Ricaud (:rik)
	Comment 26 • 10 years ago

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 :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 27 • 10 years ago

(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!

	Tamara Hills [:thills] Assignee
	Comment 28 • 10 years ago

Hi Hsin-Yi,

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

Thank you,

-tamara

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 29 • 10 years ago

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

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

Thank you :)

	Tamara Hills [:thills] Assignee
	Comment 30 • 10 years ago

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

Thanks,
-tamara

	Anthony Ricaud (:rik)
	Comment 31 • 10 years ago

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

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

	Tamara Hills [:thills] Assignee
	Comment 32 • 10 years ago

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..

	Doug Sherk (:drs) (inactive)
	Comment 33 • 10 years ago

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

	Doug Sherk (:drs) (inactive)
	Comment 34 • 10 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 10 years ago

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

	Doug Sherk (:drs) (inactive)
	Comment 36 • 10 years ago

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

	Anthony Ricaud (:rik)
	Comment 39 • 10 years ago

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

	Doug Sherk (:drs) (inactive)
	Comment 40 • 10 years ago

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.

	Doug Sherk (:drs) (inactive)
	Comment 41 • 10 years ago

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

See comment 40.

	Lingling Zhao
	Comment 42 • 10 years ago

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.

	Doug Sherk (:drs) (inactive)
	Comment 43 • 10 years ago

Redirecting to Tamara. Please see comment 42.

	Tamara Hills [:thills] Assignee
	Comment 44 • 10 years ago

Hi Lingling,

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

Thanks,

-tamara

	Lingling Zhao
	Comment 45 • 10 years ago

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

	Tamara Hills [:thills] Assignee
	Comment 46 • 10 years ago

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

	Lingling Zhao
	Comment 47 • 10 years ago

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.

	Tamara Hills [:thills] Assignee
	Comment 48 • 10 years ago

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

	Lingling Zhao
	Comment 49 • 10 years ago

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.

	Lingling Zhao
	Comment 50 • 10 years ago

Tamara,

Do you have any update for it?
Thanks.

	Tamara Hills [:thills] Assignee
	Comment 53 • 10 years ago

Hi Johan,

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

Thanks,

-tamara

	Lingling Zhao
	Comment 54 • 10 years ago

Johan,

Can you please provide the result?
Thanks.

	Johan Lorenzo [:jlorenzo]
	Comment 55 • 10 years ago

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

	Johan Lorenzo [:jlorenzo]
	Comment 56 • 10 years ago

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.

	Johan Lorenzo [:jlorenzo]
	Comment 57 • 10 years ago

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.

	bhavana bajaj [:bajaj]
	Comment 58 • 10 years ago

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

PLease ensure to verify on 2.0 post landing.

	Ryan VanderMeulen [:RyanVM]
	Comment 59 • 10 years ago

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

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

	Ryan VanderMeulen [:RyanVM]
	Comment 60 • 10 years ago

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

	Yeojin Chung [:YeojinC]
	Comment 61 • 10 years ago

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.

	Yeojin Chung [:YeojinC]
	Comment 62 • 10 years ago

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

	Coler
	Comment 63 • 10 years ago

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