1043165 - [B2G] refine TelephonyCall.onerror API: Provide TelephonyCall.disconnectedReason

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

This was originally cloned from bug 998147 comment 14.

We've seen several complaints about weird dialer error message, such as Bug 1042484. One very root cause is the ambiguous API, telephonyCall.onerror.

[quote from my previous comment]
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. However, that's not actually an error give the call had been dialled out successfully.

"BusyError" is more like the reason of the call disconnection. So I've been thinking about replacing |telephonyCall.error| with |telephonyCall.disconnectedReason| and removing .onerror. 

Once we change the API as this way, Dialer won't receive confusing notification and won't display weird message.  How do you think?

Comment 1

[Anthony Ricaud (:rik)]

If all the cases that a call couldn't be instantiated are handled by a failed promise, I think it makes sense.

Comment 2

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

(In reply to Anthony Ricaud (:rik) from comment #1)
> If all the cases that a call couldn't be instantiated are handled by a
> failed promise, I think it makes sense.

Failed promise should cover all the cases when a call is failed at being instantiated by design. Looks the new API is good to go!

Comment 3

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

Hi Tamara,

Are you working on this bug? Would you mind me taking it?

Comment 4

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

Here is a list of all possible disconnectedReason.

"BadNumberError";
"NormalCallClearingError";
"BusyError";
"NoUserRespondingError";
"UserAlertingNoAnswerError";
"CallRejectedError";
"NumberChangedError";
"PreEmptionError";
"DestinationOutOfOrderError";
"InvalidNumberFormatError";
"FacilityRejectedError";
"CongestionError";
"NetworkOutOfOrderError";
"NetworkTempFailureError";
"IncomingCallExceededError";
"BarredError";
"FDNBlockedError";
"SubscriberUnknownError";
"DeviceNotAcceptedError";
"ModifiedDialError";
"UnspecifiedError";

Comment 5

[Anshul]

Wondering if we should create enums for all of these errors in IPDL to strictly define the error types.

Comment 6

[Tamara Hills [:thills]]

Hi Szu-Yu,

Yes, that's fine.  You can take it.  Sorry, I've been busy with dialer work lately.

Thanks,
-tamara

Comment 7

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

[Tracking Requested - why for this release]:
Very likely won't make it in 2.2. this also depends on GAIA.

Comment 8

[Tamara Hills [:thills]]

Unassigning myself as I'm busy with dialer and other project at the moment.

Comment 9

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

Set assignee to default per comment 8

Comment 10

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

I am going to provide patches that add |disconnectedReason| to TelephonyCall without changing the internal interface.  Also, to land the change smoothly, we should separate the work into three steps:
1. add |TelephonyCall.disconnectedReason|
2. gaia uses |TelephonyCall.disconnectedReason| instead of |TelephonyCall.onerror|
3. phase out |TelephonyCall.onerror|

Comment 11

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

Attachment: Part 1: Add TelephonyCall.disconnectedReason (webidl)

Comment 12

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

Attachment: Part 2: Add TelephonyCall.disconnectedReason (dom)

Comment 13

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

Attachment: Part 3: Add TelephonyCall.disconnectedReason (ril)

Comment 14

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

Attachment: Part 4: Update test cases

Comment 15

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

Attachment: Part 2#2: Add TelephonyCall.disconnectedReason (dom)

Comment 16

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

Comment on attachment 8579184 [details] [diff] [review]
Part 1: Add TelephonyCall.disconnectedReason (webidl)

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

::: dom/webidl/TelephonyCall.webidl
@@ +56,5 @@
>    attribute EventHandler ongroupchange;
>  };
> +
> +enum TelephonyCallDisconnectedReason {
> +  "BadNumberError",

I know we have "Error" postfixes in our internal usage, but couldn't stop thinking of the idea of discarding it.
In such way, the naming sounds natural and more sane to me. How do you think?

Comment 17

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

Comment on attachment 8579934 [details] [diff] [review]
Part 2#2: Add TelephonyCall.disconnectedReason (dom)

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

::: dom/telephony/Telephony.cpp
@@ +684,5 @@
>      return NS_ERROR_UNEXPECTED;
>    }
>  
>    // Set the call state to 'disconnected' and remove it from the calls list.
> +  callToNotify->UpdateDisconnectedReason(aError);

How about the reason "NormalCallClearingError" which isn't notified from nsITelephonyService.notifyError but from nsITelephonyService.callStateChanged?

Comment 18

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

Comment on attachment 8579188 [details] [diff] [review]
Part 4: Update test cases

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

Can we also modify tests when calls are disconnected normally, i.e. reason is "NormalCallClearing"? Moreover, I'd like to see we check if "call.disconnectedReason" is null or not when the state isn't disconnected.

Comment 19

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> Comment on attachment 8579184 [details] [diff] [review]
> Part 1: Add TelephonyCall.disconnectedReason (webidl)
> 
> Review of attachment 8579184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/TelephonyCall.webidl
> @@ +56,5 @@
> >    attribute EventHandler ongroupchange;
> >  };
> > +
> > +enum TelephonyCallDisconnectedReason {
> > +  "BadNumberError",
> 
> I know we have "Error" postfixes in our internal usage, but couldn't stop
> thinking of the idea of discarding it.
> In such way, the naming sounds natural and more sane to me. How do you think?

Hsinyi,

Two options here.
1. remove 'Error' from enum list but keep 'Error' suffix in our internal usage (ril_consts.js)
2. Just simply remove all 'Error' suffix.

I personaly prefer option 2.  How do you think?

Comment 20

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

(In reply to Szu-Yu Chen [:aknow] from comment #19)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> > Comment on attachment 8579184 [details] [diff] [review]
> > Part 1: Add TelephonyCall.disconnectedReason (webidl)
> > 
> > Review of attachment 8579184 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/TelephonyCall.webidl
> > @@ +56,5 @@
> > >    attribute EventHandler ongroupchange;
> > >  };
> > > +
> > > +enum TelephonyCallDisconnectedReason {
> > > +  "BadNumberError",
> > 
> > I know we have "Error" postfixes in our internal usage, but couldn't stop
> > thinking of the idea of discarding it.
> > In such way, the naming sounds natural and more sane to me. How do you think?
> 
> Hsinyi,
> 
> Two options here.
> 1. remove 'Error' from enum list but keep 'Error' suffix in our internal
> usage (ril_consts.js)
> 2. Just simply remove all 'Error' suffix.
> 
> I personaly prefer option 2.  How do you think?

Oh, option 2 is not feasible.  It affects 'freezing interface'.

Comment 21

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

Attachment: Part 1#2: Add TelephonyCall.disconnectedReason (webidl)

Comment 22

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

Attachment: Part 2#3: Add TelephonyCall.disconnectedReason (dom)

Comment 23

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

Attachment: Part 3#2: Add TelephonyCall.disconnectedReason (ril)

Comment 24

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

Attachment: Part 4#2: Update test cases

Comment 25

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

Comment on attachment 8582927 [details] [diff] [review]
Part 2#3: Add TelephonyCall.disconnectedReason (dom)

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

This bug aims at touching only .webidl without  blocking  Bug 1000485 - [meta] Streamline nsITelephonyService API or Bug 959978 - (b2g-ril-interface) [meta] B2G RIL: internal interface changes. 
But we will need a new bug for discarding nsITelephonyListener.notifyError which blocks the above two. Please help file that, thank you.

Comment 26

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

Comment on attachment 8582929 [details] [diff] [review]
Part 4#2: Update test cases

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

Truly nice!

Comment 27

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

Correct the bug dependency, see comment 25.

Comment 28

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> Comment on attachment 8582927 [details] [diff] [review]
> Part 2#3: Add TelephonyCall.disconnectedReason (dom)
> 
> Review of attachment 8582927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This bug aims at touching only .webidl without  blocking  Bug 1000485 -
> [meta] Streamline nsITelephonyService API or Bug 959978 -
> (b2g-ril-interface) [meta] B2G RIL: internal interface changes. 
> But we will need a new bug for discarding nsITelephonyListener.notifyError
> which blocks the above two. Please help file that, thank you.

We need a way to pass the disconnectedReason from TelephonyService to dom after discarding nsITelephonyListener.notifyError.  One of the solutions it to extend nsITelephonyCallInfo and add an attribute 'disconnectedReason'.

Comment 29

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

(In reply to Szu-Yu Chen [:aknow] from comment #28)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> > Comment on attachment 8582927 [details] [diff] [review]
> > Part 2#3: Add TelephonyCall.disconnectedReason (dom)
> > 
> > Review of attachment 8582927 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This bug aims at touching only .webidl without  blocking  Bug 1000485 -
> > [meta] Streamline nsITelephonyService API or Bug 959978 -
> > (b2g-ril-interface) [meta] B2G RIL: internal interface changes. 
> > But we will need a new bug for discarding nsITelephonyListener.notifyError
> > which blocks the above two. Please help file that, thank you.
> 
> We need a way to pass the disconnectedReason from TelephonyService to dom
> after discarding nsITelephonyListener.notifyError.  One of the solutions it
> to extend nsITelephonyCallInfo and add an attribute 'disconnectedReason'.

Correct! Your solution is also what I had in mind.

Comment 30

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

try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2537ee817eee

Comment 31

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

https://hg.mozilla.org/integration/b2g-inbound/rev/a391057b9893
https://hg.mozilla.org/integration/b2g-inbound/rev/7fe43cd64f4d
https://hg.mozilla.org/integration/b2g-inbound/rev/f42d8e5bb0c3
https://hg.mozilla.org/integration/b2g-inbound/rev/bd080330d361

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a391057b9893
https://hg.mozilla.org/mozilla-central/rev/7fe43cd64f4d
https://hg.mozilla.org/mozilla-central/rev/f42d8e5bb0c3
https://hg.mozilla.org/mozilla-central/rev/bd080330d361

Comment 33

[Chris Mills [:cmills]]

I've documented the new property at 

https://developer.mozilla.org/en-US/docs/Web/API/TelephonyCall/disconnectedReason

And updated the TelephonyCall page (and subpages) substantially to being them up to date and explain the current state of things:

https://developer.mozilla.org/en-US/docs/Web/API/TelephonyCall
https://developer.mozilla.org/en-US/docs/Web/API/TelephonyCall/error
https://developer.mozilla.org/en-US/docs/Web/API/TelephonyCall/onerror

A technical review would be lovely, when you get the time. Thanks!