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

VERIFIED FIXED in Firefox 34

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: JMercado, Assigned: thills)

Tracking

({regression})

unspecified
2.1 S1 (1aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox33 wontfix, firefox34 fixed, b2g-v1.3 unaffected, b2g-v1.3T wontfix, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M verified, b2g-v2.1 verified)

Details

(Whiteboard: [tarako-bug-bash-1.3T][planned-sprint][in-sprint=v2.0-S6])

Attachments

(9 attachments, 4 obsolete attachments)

139.54 KB, text/plain
Details
42.44 KB, text/plain
Details
1.29 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
drs
: review+
Details | Review
7.05 KB, application/zip
Details
13.83 KB, application/zip
Details
46 bytes, text/x-github-pull-request
jlorenzo
: qa-approval+
Details | Review
1.16 KB, patch
jlorenzo
: qa-approval+
Details | Diff | Splinter Review
2.58 MB, video/mp4
Details
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
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
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?
Flags: needinfo?(firefoxos-ux-bugzilla)
Note: The notification on buri doesn't seem to occur, having said that it is in the missed call log for buri.
Flagging Carrie to clarify expected Dialer behavior.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(cawang)
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!
Flags: needinfo?(cawang)
Duplicate of this bug: 1021339
Setting more tracking flags as per bug 1021339, we should probably verify all versions again if we have time.
Duplicate of this bug: 1023092
Isn't this a regression of moving to a callscreen app (bug 990003 and specifically bug 992948) ?
(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.
Whiteboard: [tarako-bug-bash-1.3T] → [tarako-bug-bash-1.3T][planned-sprint][c=3]
Target Milestone: --- → 2.0 S6 (18july)
Assignee: nobody → thills
Summary: [Tarako][B2G][Dialer]A Missed Call notification is given when chosing to hang up on an incoming call → [Dialer] A Missed Call notification is given when chosing to hang up on an incoming call
Status: NEW → ASSIGNED
Posted patch Gecko portion of WIP patch (obsolete) — Splinter Review
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
Attachment #8452877 - Flags: feedback?(htsai)
Flags: needinfo?(htsai)
(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?
Flags: needinfo?(thills)
Flags: needinfo?(szchen)
Flags: needinfo?(htsai)
Attachment #8452877 - Flags: feedback?(htsai)
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.
(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|.
Flags: needinfo?(szchen)
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
Flags: needinfo?(htsai)
Flags: needinfo?(thills)
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?
(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.
Flags: needinfo?(htsai)
Duplicate of this bug: 1017469
Posted patch Updated Gecko Patch (obsolete) — Splinter Review
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
Attachment #8452877 - Attachment is obsolete: true
Attachment #8456021 - Flags: review?(htsai)
Posted patch 998147gaiapatch.diff (obsolete) — Splinter Review
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
Attachment #8452859 - Attachment is obsolete: true
Attachment #8456027 - Flags: review?(gsvelto)
Comment on attachment 8456027 [details] [diff] [review]
998147gaiapatch.diff

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

We did the review IRL.
Attachment #8456027 - Flags: review?(gsvelto) → feedback+
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?
Attachment #8456021 - Flags: review?(htsai)
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
Flags: needinfo?(anthony)
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 :)
Flags: needinfo?(anthony)
(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!
Hi Hsin-Yi,

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

Thank you,

-tamara
Attachment #8456021 - Attachment is obsolete: true
Attachment #8456764 - Flags: review?(htsai)
Comment on attachment 8456764 [details] [diff] [review]
Updated patch with parameter changed to hangUpLocal

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

Thank you :)
Attachment #8456764 - Flags: review?(htsai) → review+
Hi Anthony,
Here is the updated pull request from what we looked at last week.

Thanks,
-tamara
Attachment #8456027 - Attachment is obsolete: true
Attachment #8460339 - Flags: review?(anthony)
Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

I've already co-authored this so redirecting the review to Doug.
Attachment #8460339 - Flags: review?(anthony) → review?(drs+bugzilla)
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 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.
Attachment #8460339 - Flags: review?(drs+bugzilla) → review+
https://github.com/mozilla-b2g/gaia/commit/c9546cc0cbd3cbd8ce764507ab4759ab22f63eb6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [tarako-bug-bash-1.3T][planned-sprint][c=3] → [tarako-bug-bash-1.3T][planned-sprint][in-sprint=v2.0-S6]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Duplicate of this bug: 1083082
Duplicate of this bug: 1086185
Doug: We have two duplicates for 2.0, we probably want to uplift this.
Flags: needinfo?(drs.bugzilla)
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.
Flags: needinfo?(drs.bugzilla)
Attachment #8456764 - Flags: approval-gaia-v2.0?(release-mgmt)
Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

See comment 40.
Attachment #8460339 - Flags: approval-gaia-v2.0?(release-mgmt)
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.
Flags: needinfo?(drs.bugzilla)
Redirecting to Tamara. Please see comment 42.
Flags: needinfo?(drs.bugzilla) → needinfo?(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
I apply both the gaia and gecko patches, and add some logs in gecko, but it doesnot work.
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
Flags: needinfo?(thills)
Flags: needinfo?(zhaolingling)
Flags: needinfo?(sync-1)
Posted file 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.
Flags: needinfo?(zhaolingling)
Flags: needinfo?(thills)
Flags: needinfo?(sync-1)
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
Flags: needinfo?(thills) → needinfo?(zhaolingling)
Posted file 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.
Flags: needinfo?(zhaolingling)
Tamara,

Do you have any update for it?
Thanks.
Flags: needinfo?(thills)
Flags: needinfo?(thills)
Hi Johan,

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

Thanks,

-tamara
Flags: needinfo?(jlorenzo)
Johan,

Can you please provide the result?
Thanks.
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
Flags: needinfo?(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.
Attachment #8520625 - Flags: qa-approval+
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.
Attachment #8520632 - Flags: qa-approval+
Keywords: regression
blocking-b2g: --- → 2.0+
Attachment #8456764 - Flags: approval-gaia-v2.0?(release-mgmt) → approval-gaia-v2.0+
Comment on attachment 8460339 [details] [review]
Pull Request for Gaia Portion

PLease ensure to verify on 2.0 post landing.
Attachment #8460339 - Flags: approval-gaia-v2.0?(release-mgmt) → approval-gaia-v2.0+
Keywords: verifyme
Attachment #8520632 - Attachment is patch: true
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?][failed-verification]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][failed-verification] → [QAnalyst-Triage+][failed-verification]
Flags: needinfo?(ktucker)
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
Blocks: Woodduck
You need to log in before you can comment on or make changes to this bug.