717462 - WebTelephony: notify errors

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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

[Shian-Yow Wu [:swu]]

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

Comment 2

[Shian-Yow Wu [:swu]]

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

Comment 3

[Philipp von Weitershausen [:philikon]]

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

Comment 4

[Philipp von Weitershausen [:philikon]]

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

Comment 5

[Price Tseng]

Attachment: 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

Comment 6

[Philipp von Weitershausen [:philikon]]

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!

Comment 7

[Price Tseng]

Attachment: WebTelephony notify errors V2

Modify the errors according to comment 6

Comment 8

[Philipp von Weitershausen [:philikon]]

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

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) { ... }

Comment 10

[Philipp von Weitershausen [:philikon]]

Price, can you pleas make the changes that Jonas suggested so that we wrap up this bug? Thanks!

Comment 11

[Price Tseng]

Attachment: WebTelephony notify errors, V3

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

Thanks

Comment 12

[Philipp von Weitershausen [:philikon]]

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.

Comment 13

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf0744de30a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ddf0744de30a