WebTelephony: notify errors

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: philikon, Assigned: Price Tseng)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Right now the WebTelephony API has no error notifications, e.g. when a call is placed and the radio isn't online, there's no SIM card, etc. Not sure we want to transfer that detailed state to content. But just saying that there *was* an error would be good IMHO. (We can't just have .dial() or so throw because most of this stuff is discovered asynchronously.)

Comment 1

5 years ago
It there a draft Telephony API definition to specify error cases?

Comment 2

5 years ago
Hi Philipp,

Below two bugs seems related to this one, I wonder that should we made some dependency between these issues?  Looks bug 712944 is talking about same thing as this one.
Bug 712944 - B2G telephony: ensure error scenarios are covered
Bug 725099 - RIL: Handle and report errors

What kind of error notification is expected?  Is below implementation in current Telephony.cpp an expected example?  It returns error with a warning message.

NS_IMETHODIMP
Telephony::Dial(const nsAString& aNumber, nsIDOMTelephonyCall** aResult)
{
  NS_ENSURE_ARG(!aNumber.IsEmpty());

  for (PRUint32 index = 0; index < mCalls.Length(); index++) {
    const nsRefPtr<TelephonyCall>& tempCall = mCalls[index];
    if (tempCall->IsOutgoing() &&
        tempCall->CallState() < nsIRadioInterfaceLayer::CALL_STATE_CONNECTED) {
      // One call has been dialed already and we only support one outgoing call
      // at a time.
      NS_WARNING("Only permitted to dial one call at a time!");
      return NS_ERROR_NOT_AVAILABLE;
    }
  }
(In reply to Shian-Yow Wu from comment #2)
> Below two bugs seems related to this one, I wonder that should we made some
> dependency between these issues?  Looks bug 712944 is talking about same
> thing as this one.

Not really. Bug 712944 is about the RadioInterfaceLayer.js and its worker. This bug here is about the DOM part (hence the "WebTelephony" in the name.)

> Bug 712944 - B2G telephony: ensure error scenarios are covered

This is indeed the ground work for all of this.

> What kind of error notification is expected?

I think we want to define an 'error' event that's dispatched on the call object.

> Is below implementation in
> current Telephony.cpp an expected example?  It returns error with a warning
> message.

This would constrict us to synchronous error checking only. I think we want an error event.
Summing up an IRC explanation:

1. The RIL system should be able to report errors for requests and be able to associate those errors with the original message that was submitted. This is bug 725099.
2. Then on top of that we want to implement error handling for the telephony part of RadioInterfaceLayer. This is bug 712944.
3. And then, once RadioInterfaceLayer knows about errors, we can inform the DOM which can send events. That's bug 717462
(Assignee)

Comment 5

5 years ago
Created attachment 615270 [details] [diff] [review]
WebTelephony notify errors

Hi Philipp,

According to Bug 712944, the Web Telephony can be notified when errors occur in the lower layer. After our discussion, I implement the error handling for DOM API.
I think I should propose the modification so that people can discuss this on dev-webapi
Attachment #615270 - Flags: feedback?(philipp)
Comment on attachment 615270 [details] [diff] [review]
WebTelephony notify errors

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

Good start. Comments below.

::: dom/telephony/Telephony.cpp
@@ +522,1 @@
>      mCalls[index]->ChangeState(nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED);

You're forgetting to notify the "error" event here. You probably want to create a `NotifyError` method on the `TelephonyCall` class that
(1) sets the error string
(2) does the state transitions
(3) notifies the error event.
Then you only have to call that method here.

::: dom/telephony/TelephonyCall.cpp
@@ +61,5 @@
>  
>    call->mTelephony = aTelephony;
>    call->mNumber = aNumber;
>    call->mCallIndex = aCallIndex;
> +  call->mError = EmptyString();

I think the default value should be `null` (in JS) which you can indicate with call->mError->SetIsVoid(true).

@@ +212,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +TelephonyCall::GetError(nsAString & aError)

No space before the &. Please follow the coding style of the file!

@@ +219,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +TelephonyCall::SetError(const nsAString & aError)

No need for this since it should be readonly!

::: dom/telephony/nsIDOMTelephonyCall.idl
@@ +48,5 @@
>  
>    readonly attribute DOMString state;
>  
> +  attribute DOMString error;
> +

readonly!
Attachment #615270 - Flags: feedback?(philipp)
(Assignee)

Comment 7

5 years ago
Created attachment 616072 [details] [diff] [review]
WebTelephony notify errors V2

Modify the errors according to comment 6
Attachment #615270 - Attachment is obsolete: true
Attachment #616072 - Flags: feedback?(philipp)
Comment on attachment 616072 [details] [diff] [review]
WebTelephony notify errors V2

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

Looks good to me! Paging sicking for super review on the interface changes.

::: dom/telephony/TelephonyCall.cpp
@@ +82,5 @@
> +  nsRefPtr<CallEvent> event = CallEvent::Create(this);
> +  NS_ASSERTION(event, "This should never fail!");
> +
> +  if (NS_FAILED(event->Dispatch(ToIDOMEventTarget(),
> +	                            NS_LITERAL_STRING("error")))) {

nit: align arguments
Attachment #616072 - Flags: superreview?(jonas)
Attachment #616072 - Flags: review+
Attachment #616072 - Flags: feedback?(philipp)
Comment on attachment 616072 [details] [diff] [review]
WebTelephony notify errors V2

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

Let's make the nsIDOMTelephonyCall.error property an nsIDOMError (which currently is just an object with a single string property on it, so you can keep all the same logic and error values). That way we can add further technical information in the future about the error if we want to. And checking if something went wrong would still just be

if (call.error) { ... }
Attachment #616072 - Flags: superreview?(jonas) → superreview+
Price, can you pleas make the changes that Jonas suggested so that we wrap up this bug? Thanks!
(Assignee)

Updated

5 years ago
Assignee: nobody → ptseng
(Assignee)

Comment 11

5 years ago
Created attachment 620214 [details] [diff] [review]
WebTelephony notify errors, V3

Hi Philipp,
Sorry for responsing late!
Please review the patch

Thanks
Attachment #616072 - Attachment is obsolete: true
Attachment #620214 - Flags: review?(philipp)
(Reporter)

Updated

5 years ago
Attachment #620214 - Flags: review?(philipp) → review+
Btw, now that we're using DOMError, we should change the GECKO_CALL_ERROR_* constants in bug 712944 to match the DOMError naming convention, e.g. "BadNumberError", etc.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf0744de30a
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/ddf0744de30a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.