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.

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

(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 :)

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.

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.

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.

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

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

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

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

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

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 on attachment 633986 [details] [diff] [review] patch (v3) Review of attachment 633986 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Much cleaner and shorter, no? :)

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

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=205babe29243