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

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hsinyi, Assigned: aknow)

Tracking

({dev-doc-complete})

unspecified
2.2 S9 (3apr)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog, firefox39 fixed)

Details

(Whiteboard: [grooming][priority2])

Attachments

(4 attachments, 5 obsolete attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 2

5 years ago
(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!
Reporter

Updated

5 years ago
Blocks: 1000485
Assignee: nobody → thills
Status: NEW → ASSIGNED
Reporter

Updated

5 years ago
Reporter

Updated

5 years ago
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";

Comment 5

5 years ago
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
Reporter

Comment 9

4 years ago
Set assignee to default per comment 8
Assignee: thills → nobody
blocking-b2g: backlog → ---
Assignee

Updated

4 years ago
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|
Assignee

Updated

4 years ago
Summary: [B2G] refine TelephonyCall.onerror API → [B2G] refine TelephonyCall.onerror API: Provide TelephonyCall.disconnectedReason
Posted 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)
Reporter

Comment 16

4 years ago
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)
Reporter

Comment 17

4 years ago
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)
Reporter

Comment 18

4 years ago
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)
Reporter

Updated

4 years ago
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)
Reporter

Updated

4 years ago
Attachment #8582926 - Flags: review?(htsai) → review+
Reporter

Comment 25

4 years ago
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+
Reporter

Updated

4 years ago
Attachment #8582928 - Flags: review?(htsai) → review+
Reporter

Comment 26

4 years ago
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+
Reporter

Comment 27

4 years ago
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'.
Reporter

Comment 29

4 years ago
(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.
Assignee

Updated

4 years ago
Blocks: 1147735
Reporter

Updated

4 years ago
Blocks: 1042484
Reporter

Updated

4 years ago
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.