Last Comment Bug 717462 - WebTelephony: notify errors
: WebTelephony: notify errors
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Price Tseng
:
Mentors:
Depends on:
Blocks: webtelephony
  Show dependency treegraph
 
Reported: 2012-01-11 16:43 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-05-15 06:30 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WebTelephony notify errors (6.17 KB, patch)
2012-04-16 01:18 PDT, Price Tseng
no flags Details | Diff | Review
WebTelephony notify errors V2 (6.09 KB, patch)
2012-04-18 04:12 PDT, Price Tseng
philipp: review+
jonas: superreview+
Details | Diff | Review
WebTelephony notify errors, V3 (7.21 KB, patch)
2012-05-02 00:07 PDT, Price Tseng
philipp: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-01-11 16:43:35 PST
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] 2012-02-14 02:59:11 PST
It there a draft Telephony API definition to specify error cases?
Comment 2 Shian-Yow Wu [:swu] 2012-02-15 03:10:13 PST
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] 2012-02-15 18:17:10 PST
(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] 2012-02-15 18:38:58 PST
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 2012-04-16 01:18:33 PDT
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
Comment 6 Philipp von Weitershausen [:philikon] 2012-04-17 22:46:41 PDT
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 2012-04-18 04:12:13 PDT
Created attachment 616072 [details] [diff] [review]
WebTelephony notify errors V2

Modify the errors according to comment 6
Comment 8 Philipp von Weitershausen [:philikon] 2012-04-27 19:18:09 PDT
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) PTO Until July 5th 2012-04-30 15:00:08 PDT
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] 2012-05-01 17:21:15 PDT
Price, can you pleas make the changes that Jonas suggested so that we wrap up this bug? Thanks!
Comment 11 Price Tseng 2012-05-02 00:07:59 PDT
Created attachment 620214 [details] [diff] [review]
WebTelephony notify errors, V3

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

Thanks
Comment 12 Philipp von Weitershausen [:philikon] 2012-05-02 17:15:31 PDT
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] 2012-05-14 21:14:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf0744de30a
Comment 14 Ed Morley [:emorley] 2012-05-15 06:30:36 PDT
https://hg.mozilla.org/mozilla-central/rev/ddf0744de30a

Note You need to log in before you can comment on or make changes to this bug.