Last Comment Bug 761533 - WebTelephony: busy event is never fired
: WebTelephony: busy event is never fired
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-05 02:19 PDT by Hsin-Yi Tsai [:hsinyi]
Modified: 2012-06-20 08:02 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (5.05 KB, patch)
2012-06-13 21:31 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
patch: fire busy event (7.69 KB, text/plain)
2012-06-14 01:50 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details
patch (part 2): delete test scripts (4.76 KB, patch)
2012-06-14 02:20 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review-
Details | Diff | Review
patch (part1) v2 (6.86 KB, patch)
2012-06-14 02:54 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: feedback+
Details | Diff | Review
patch (v3) (3.83 KB, patch)
2012-06-18 03:52 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Review

Description Hsin-Yi Tsai [:hsinyi] 2012-06-05 02:19:45 PDT
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.
Comment 1 Hsin-Yi Tsai [:hsinyi] 2012-06-07 21:32:22 PDT
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 :)
Comment 2 Hsin-Yi Tsai [:hsinyi] 2012-06-08 02:57:45 PDT
(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 :)
Comment 3 Hsin-Yi Tsai [:hsinyi] 2012-06-13 21:31:42 PDT
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.
Comment 4 Hsin-Yi Tsai [:hsinyi] 2012-06-14 01:50:38 PDT
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.
Comment 5 Hsin-Yi Tsai [:hsinyi] 2012-06-14 02:20:23 PDT
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.
Comment 6 Hsin-Yi Tsai [:hsinyi] 2012-06-14 02:21:46 PDT
(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.
Comment 7 Hsin-Yi Tsai [:hsinyi] 2012-06-14 02:31:23 PDT
(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.
Comment 8 Hsin-Yi Tsai [:hsinyi] 2012-06-14 02:43:29 PDT
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.
Comment 9 Hsin-Yi Tsai [:hsinyi] 2012-06-14 02:54:38 PDT
Created attachment 633082 [details] [diff] [review]
patch (part1) v2
Comment 10 Philipp von Weitershausen [:philikon] 2012-06-15 16:04:37 PDT
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.
Comment 11 Philipp von Weitershausen [:philikon] 2012-06-15 16:07:28 PDT
(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 12 Philipp von Weitershausen [:philikon] 2012-06-15 16:07:58 PDT
Comment on attachment 633075 [details] [diff] [review]
patch (part 2): delete test scripts

Let's not remove these, see comment 11.
Comment 13 Hsin-Yi Tsai [:hsinyi] 2012-06-15 20:19:30 PDT
(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!
Comment 14 Hsin-Yi Tsai [:hsinyi] 2012-06-15 20:22:00 PDT
(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!
Comment 15 Hsin-Yi Tsai [:hsinyi] 2012-06-18 03:52:38 PDT
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.
Comment 16 Philipp von Weitershausen [:philikon] 2012-06-18 15:37:38 PDT
Comment on attachment 633986 [details] [diff] [review]
patch (v3)

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

Nice! Much cleaner and shorter, no? :)
Comment 17 Hsin-Yi Tsai [:hsinyi] 2012-06-18 18:45:50 PDT
(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.
Comment 18 Hsin-Yi Tsai [:hsinyi] 2012-06-20 00:39:56 PDT
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=205babe29243
Comment 20 :Ehsan Akhgari (out sick) 2012-06-20 08:02:22 PDT
https://hg.mozilla.org/mozilla-central/rev/f7e10a29e5c6

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