Closed Bug 1043165 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(tracking-b2g:backlog, firefox39 fixed)

RESOLVED FIXED
2.2 S9 (3apr)
tracking-b2g backlog
Tracking Status
firefox39 --- fixed

People

(Reporter: hsinyi, Assigned: aknow)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [grooming][priority2])

Attachments

(4 files, 5 obsolete files)

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!
Blocks: 1000485
Assignee: nobody → thills
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
Hi Tamara, Are you working on this bug? Would you mind me taking it?
Flags: needinfo?(thills)
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
Flags: needinfo?(thills)
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2?
Whiteboard: [priority2]
[Tracking Requested - why for this release]: Very likely won't make it in 2.2. this also depends on GAIA.
feature-b2g: 2.2? → ---
tracking-b2g: --- → ?
Whiteboard: [priority2] → [grooming][priority2]
Unassigning myself as I'm busy with dialer and other project at the moment.
Status: ASSIGNED → NEW
Set assignee to default per comment 8
Assignee: thills → nobody
blocking-b2g: backlog → ---
Assignee: nobody → szchen
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|
Summary: [B2G] refine TelephonyCall.onerror API → [B2G] refine TelephonyCall.onerror API: Provide TelephonyCall.disconnectedReason
Attached patch Part 4: Update test cases (obsolete) — Splinter Review
Attachment #8579188 - Flags: review?(htsai)
Attachment #8579186 - Attachment is obsolete: true
Attachment #8579186 - Flags: review?(htsai)
Attachment #8579934 - Flags: review?(htsai)
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?
Attachment #8579184 - Flags: review?(htsai)
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?
Attachment #8579934 - Flags: review?(htsai)
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.
Attachment #8579188 - Flags: review?(htsai)
Attachment #8579187 - Flags: review?(htsai) → review+
(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?
Flags: needinfo?(htsai)
(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'.
Flags: needinfo?(htsai)
Attachment #8579184 - Attachment is obsolete: true
Attachment #8582926 - Flags: review?(htsai)
Attachment #8579934 - Attachment is obsolete: true
Attachment #8582927 - Flags: review?(htsai)
Attachment #8579187 - Attachment is obsolete: true
Attachment #8582928 - Flags: review?(htsai)
Attachment #8579188 - Attachment is obsolete: true
Attachment #8582929 - Flags: review?(htsai)
Attachment #8582926 - Flags: review?(htsai) → review+
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.
Attachment #8582927 - Flags: review?(htsai) → review+
Attachment #8582928 - Flags: review?(htsai) → review+
Comment on attachment 8582929 [details] [diff] [review] Part 4#2: Update test cases Review of attachment 8582929 [details] [diff] [review]: ----------------------------------------------------------------- Truly nice!
Attachment #8582929 - Flags: review?(htsai) → review+
Correct the bug dependency, see comment 25.
No longer blocks: b2g-ril-interface, 1000485
(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.
Blocks: 1147735
Blocks: 1042484
Blocks: 1100856
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!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: