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)
Tracking
(tracking-b2g:backlog, firefox39 fixed)
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: hsinyi, Assigned: aknow)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [grooming][priority2])
Attachments
(4 files, 5 obsolete files)
2.78 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
6.56 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
5.96 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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•10 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!
Updated•10 years ago
|
Assignee: nobody → thills
Status: NEW → ASSIGNED
Reporter | ||
Updated•10 years ago
|
Blocks: b2g-ril-interface
Reporter | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 3•10 years ago
|
||
Hi Tamara,
Are you working on this bug? Would you mind me taking it?
Flags: needinfo?(thills)
Assignee | ||
Comment 4•10 years ago
|
||
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.
Comment 6•10 years ago
|
||
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)
Updated•10 years ago
|
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2?
Whiteboard: [priority2]
Comment 7•10 years ago
|
||
[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:
--- → ?
Updated•10 years ago
|
Whiteboard: [priority2] → [grooming][priority2]
Comment 8•10 years ago
|
||
Unassigning myself as I'm busy with dialer and other project at the moment.
Status: ASSIGNED → NEW
Updated•10 years ago
|
blocking-b2g: backlog → ---
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → szchen
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
Summary: [B2G] refine TelephonyCall.onerror API → [B2G] refine TelephonyCall.onerror API: Provide TelephonyCall.disconnectedReason
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8579184 -
Flags: review?(htsai)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8579186 -
Flags: review?(htsai)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8579187 -
Flags: review?(htsai)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8579188 -
Flags: review?(htsai)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8579186 -
Attachment is obsolete: true
Attachment #8579186 -
Flags: review?(htsai)
Attachment #8579934 -
Flags: review?(htsai)
Reporter | ||
Comment 16•10 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•10 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•10 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•10 years ago
|
Attachment #8579187 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
(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)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8579184 -
Attachment is obsolete: true
Attachment #8582926 -
Flags: review?(htsai)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8579934 -
Attachment is obsolete: true
Attachment #8582927 -
Flags: review?(htsai)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8579187 -
Attachment is obsolete: true
Attachment #8582928 -
Flags: review?(htsai)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8579188 -
Attachment is obsolete: true
Attachment #8582929 -
Flags: review?(htsai)
Reporter | ||
Updated•10 years ago
|
Attachment #8582926 -
Flags: review?(htsai) → review+
Reporter | ||
Comment 25•10 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•10 years ago
|
Attachment #8582928 -
Flags: review?(htsai) → review+
Reporter | ||
Comment 26•10 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•10 years ago
|
||
Correct the bug dependency, see comment 25.
No longer blocks: b2g-ril-interface, 1000485
Assignee | ||
Comment 28•10 years ago
|
||
(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•10 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 | ||
Comment 30•10 years ago
|
||
Assignee | ||
Comment 31•10 years ago
|
||
Comment 32•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S9 (3apr)
Comment 33•10 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•