The default bug view has changed. See this FAQ.

WebTelephony: busy event is never fired

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: hsinyi, Assigned: hsinyi)

Tracking

unspecified
mozilla16
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
There is a "busy" event in WebTelephony API. That is needed to let UI application know that the one we wanna call is busy. However, the implementation for this event is missing so far.
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
(Assignee)

Updated

5 years ago
Assignee: nobody → htsai
(Assignee)

Comment 1

5 years ago
Hi, I'm going to work on this. Here's my rough thought about how i gonna proceed. 

Use 'REQUEST_LAST_CALL_FAIL_CAUSE' in 'ril_worker.js' to get the information about 'call busy.' Then, send this info along with 'callDisconnected' message. WebTelephony can be eventually notified and further dispatch 'busy' event to UI. When dispatching 'busy' event, the call state is 'nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED' instead of 'nsIRadioInterfaceLayer::CALL_STATE_BUSY' 

Also, in the current implementation, the handler of 'REQUEST_LAST_CALL_FAIL_CAUSE' sends 'callError' message to DOM. However, IMO this is not always right because call fails doesn't necessarily imply to call error. 'Call busy' is an example. So, some modification will be done for this.

How do you think about my thought? Feedbacks are welcome as always. Thanks :)
(Assignee)

Comment 2

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Hi, I'm going to work on this. Here's my rough thought about how i gonna
> proceed. 
> 
> Use 'REQUEST_LAST_CALL_FAIL_CAUSE' in 'ril_worker.js' to get the information
> about 'call busy.' Then, send this info along with 'callDisconnected'
> message. WebTelephony can be eventually notified and further dispatch 'busy'
> event to UI. When dispatching 'busy' event, the call state is
> 'nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED' instead of
> 'nsIRadioInterfaceLayer::CALL_STATE_BUSY' 
> 
> Also, in the current implementation, the handler of
> 'REQUEST_LAST_CALL_FAIL_CAUSE' sends 'callError' message to DOM. However,
> IMO this is not always right because call fails doesn't necessarily imply to
> call error. 'Call busy' is an example. So, some modification will be done
> for this.
> 
After investigating more, I think I will just follow the current error-notifying mechanism, and view 'call busy' as a call error, broadly. 

> How do you think about my thought? Feedbacks are welcome as always. Thanks :)
(Assignee)

Comment 3

5 years ago
Created attachment 633012 [details] [diff] [review]
WIP

WIP: request "REQUEST_LAST_CALL_FAIL_CAUSE" before changing call state to 'disconnected' to cover 'BusyError'.

Need to enhance 'Telephony::NotifyError' to update mActiveCall consequently.
(Assignee)

Comment 4

5 years ago
Created attachment 633066 [details]
patch: fire busy event

Use 'REQUEST_LAST_CALL_FAIL_CAUSE' for each disconnected call, then fire notify WebTelephony 'CallError' if the call didn't fail normally. WebTelephony checks 'aError' and fire specific event, i.e. 'busy' event.
Attachment #633012 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 633075 [details] [diff] [review]
patch (part 2): delete test scripts

We fire error event based on 'REQUEST_LAST_CALL_FAIL_CAUSE' but it seems emulator doesn't support this advanced request. In emulator, the length of response of 'REQUEST_LAST_CALL_FAIL_CAUSE' is zero, hence we cannot get any further information about call error. Then, of course, no following 'onerror' and 'onbusy'. 

Also, "gsm call <phoneNumber>" cannot tell whether <phoneNumber> is valid or not. That is, "gsm call <phoneNumber>" always makes an  'active' call even <phoneNumber> is a bad number or string.    

Due to the above reasons, I reason it is unobtainable to test 'busy' and 'error' events by emulator. That's why I provide this patch.
(Assignee)

Comment 6

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Created attachment 633066 [details]
> patch: fire busy event
> 
> Use 'REQUEST_LAST_CALL_FAIL_CAUSE' for each disconnected call, then fire
> notify WebTelephony 'CallError' if the call didn't fail normally.
> WebTelephony checks 'aError' and fire specific event, i.e. 'busy' event.

I cannot test this patch by marionette test and emulator according to Comment #5. But I tested the patch on phone, and it works fine.
(Assignee)

Updated

5 years ago
Attachment #633075 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #633066 - Flags: review?(philipp)
(Assignee)

Comment 7

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Created attachment 633075 [details] [diff] [review]
> patch (part 2): delete test scripts
> 
> We fire error event based on 'REQUEST_LAST_CALL_FAIL_CAUSE' but it seems
> emulator doesn't support this advanced request. In emulator, the length of
> response of 'REQUEST_LAST_CALL_FAIL_CAUSE' is zero, hence we cannot get any
> further information about call error. Then, of course, no following
> 'onerror' and 'onbusy'. 
> 
> Also, "gsm call <phoneNumber>" cannot tell whether <phoneNumber> is valid or
> not. That is, "gsm call <phoneNumber>" always makes an  'active' call even
> <phoneNumber> is a bad number or string.    
> 
> Due to the above reasons, I reason it is unobtainable to test 'busy' and
> 'error' events by emulator. That's why I provide this patch.

One more observation, we get 'unspecifiedError', instead of 'badNumberError', from RIL when dialing an invalid number.
(Assignee)

Comment 8

5 years ago
Comment on attachment 633066 [details]
patch: fire busy event

Sorry, just realized that this patch covers some part in Bug 751550. 
I should restrict this firmly to 'busy event'. 
Provide a modified patch later.
Attachment #633066 - Attachment is obsolete: true
Attachment #633066 - Attachment is patch: false
Attachment #633066 - Flags: review?(philipp)
(Assignee)

Comment 9

5 years ago
Created attachment 633082 [details] [diff] [review]
patch (part1) v2
Attachment #633082 - Flags: review?(philipp)
Comment on attachment 633082 [details] [diff] [review]
patch (part1) v2

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

Excellent work! Please note my comment about the "busy" event below.

::: dom/telephony/Telephony.cpp
@@ +477,5 @@
> +  if (aIsActive) {
> +    mActiveCall = nsnull;
> +  }
> +
> +  // The connection is not established yet, remove the latest call object.

Might be worth amending the comment to mention that NotifyError will set the call state to "disconnected" which will automatically remove the call from the `calls` list.

@@ +486,5 @@
>      }
> +    return NS_OK;
> +  }
> +
> +  // The connection has been establisehd. Get the failed call.

Nit: typo in "established"

::: dom/telephony/TelephonyCall.cpp
@@ +140,5 @@
>                                  NS_LITERAL_STRING("error")))) {
>      NS_WARNING("Failed to dispatch error event!");
>    }
> +
> +  if (aError.EqualsLiteral("BusyError")) {

"busy" is actually supposed to be a state. So if there's a busy call, we don't really want to dispatch an "error" event and set call.error. Instead we want to set call.state = "busy" and dispatch the "busy" event.

We could do this here, e.g. with

  void
  TelephonyCall::NotifyError(const nsAString& aError)
  {
    if (aError.EqualsLiteral("BusyError")) {
      ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_BUSY, true);
      ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED, true);
      return;
    }
    // Set the error string
    NS_ASSERTION(!mError, "Already have an error?");
    ...

or we could do this already in `ril_worker.js` when we analyse the fail cause. I would prefer doing it as early as possible, but it might be tricky getting the disconnected/callschanged case right.
Attachment #633082 - Flags: review?(philipp) → feedback+
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> We fire error event based on 'REQUEST_LAST_CALL_FAIL_CAUSE' but it seems
> emulator doesn't support this advanced request. In emulator, the length of
> response of 'REQUEST_LAST_CALL_FAIL_CAUSE' is zero, hence we cannot get any
> further information about call error. Then, of course, no following
> 'onerror' and 'onbusy'. 
> 
> Also, "gsm call <phoneNumber>" cannot tell whether <phoneNumber> is valid or
> not. That is, "gsm call <phoneNumber>" always makes an  'active' call even
> <phoneNumber> is a bad number or string.    
> 
> Due to the above reasons, I reason it is unobtainable to test 'busy' and
> 'error' events by emulator. That's why I provide this patch.

Thanks for the explanation, Hsinyi. Too bad the emulator is lacking this functionality. I would still like to keep the tests around, though. Let's just disable them in the manifest and file a follow-up bug about making them work in the future (e.g. after hacking the emulator.)
Comment on attachment 633075 [details] [diff] [review]
patch (part 2): delete test scripts

Let's not remove these, see comment 11.
Attachment #633075 - Flags: review?(philipp) → review-
(Assignee)

Comment 13

5 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 633082 [details] [diff] [review]
> patch (part1) v2
> 
> Review of attachment 633082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent work! Please note my comment about the "busy" event below.
> 
> ::: dom/telephony/Telephony.cpp
> @@ +477,5 @@
> > +  if (aIsActive) {
> > +    mActiveCall = nsnull;
> > +  }
> > +
> > +  // The connection is not established yet, remove the latest call object.
> 
> Might be worth amending the comment to mention that NotifyError will set the
> call state to "disconnected" which will automatically remove the call from
> the `calls` list.
>
Good, will make the comment more detailed.
 
> @@ +486,5 @@
> >      }
> > +    return NS_OK;
> > +  }
> > +
> > +  // The connection has been establisehd. Get the failed call.
> 
> Nit: typo in "established"

Aha, thanks!
> 
> ::: dom/telephony/TelephonyCall.cpp
> @@ +140,5 @@
> >                                  NS_LITERAL_STRING("error")))) {
> >      NS_WARNING("Failed to dispatch error event!");
> >    }
> > +
> > +  if (aError.EqualsLiteral("BusyError")) {
> 
> "busy" is actually supposed to be a state. So if there's a busy call, we
> don't really want to dispatch an "error" event and set call.error. Instead
> we want to set call.state = "busy" and dispatch the "busy" event.
> 
> We could do this here, e.g. with
> 
>   void
>   TelephonyCall::NotifyError(const nsAString& aError)
>   {
>     if (aError.EqualsLiteral("BusyError")) {
>       ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_BUSY, true);
>       ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED,
> true);
>       return;
>     }
>     // Set the error string
>     NS_ASSERTION(!mError, "Already have an error?");
>     ...
> 
> or we could do this already in `ril_worker.js` when we analyse the fail
> cause. I would prefer doing it as early as possible, but it might be tricky
> getting the disconnected/callschanged case right.
Hmmm, I will see how to make the call state change as early as possible. 
Thanks for the comments!
(Assignee)

Comment 14

5 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> Comment on attachment 633075 [details] [diff] [review]
> patch (part 2): delete test scripts
> 
> Let's not remove these, see comment 11.
OK, agree!
(Assignee)

Comment 15

5 years ago
Created attachment 633986 [details] [diff] [review]
patch (v3)

Updated patch according to Comment 10: no 'error' event, but changing to 'busy state' followed by a dispatched 'busy' event. The call state is changed to 'disconnected' eventually.
Attachment #633075 - Attachment is obsolete: true
Attachment #633082 - Attachment is obsolete: true
Attachment #633986 - Flags: review?(philipp)
Comment on attachment 633986 [details] [diff] [review]
patch (v3)

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

Nice! Much cleaner and shorter, no? :)
Attachment #633986 - Flags: review?(philipp) → review+
(Assignee)

Comment 17

5 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Comment on attachment 633986 [details] [diff] [review]
> patch (v3)
> 
> Review of attachment 633986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! Much cleaner and shorter, no? :)

Ha, yes, good suggestion. Thanks, Philipp.
(Assignee)

Comment 18

5 years ago
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=205babe29243
(Assignee)

Comment 19

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f7e10a29e5c6
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f7e10a29e5c6
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.