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?

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

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

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

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";

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

Hi Szu-Yu, Yes, that's fine. You can take it. Sorry, I've been busy with dialer work lately. Thanks, -tamara

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

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

Set assignee to default per comment 8

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

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

(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 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 on attachment 8582929 [details] [diff] [review] Part 4#2: Update test cases Review of attachment 8582929 [details] [diff] [review]: ----------------------------------------------------------------- Truly nice!

Correct the bug dependency, see comment 25.

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

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

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

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

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

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!