Closed Bug 881142 Opened 7 years ago Closed 7 years ago

WebTelephony: remove Busy state

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: hsinyi, Assigned: aknow)

References

Details

Attachments

(5 files, 8 obsolete files)

2.51 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
Details | Diff | Splinter Review
4.19 KB, patch
Details | Diff | Splinter Review
1.93 KB, patch
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
When we dial someone busy, we will hear busy tone from the network. But there is NO signal for 'busy' state transition accompanying that voice tone from the carrier. The call state remains alerting until user hangs it up, the call is redirected/forwarded, or the carrier releases it.

"Busy" information can be obtained by querying the carrier about the fail cause of the last call, but at the moment, the call has been already released at an earlier time. This is what we have done to transfer the call state to 'busy' in B2G, but we shall re-think about it, IMHO.

Removing busy state but providing a reason seems appropriate, if we do want to provide this 'busy' information.

Related discussion in W3C sysapps:
[1] http://lists.w3.org/Archives/Public/public-sysapps/2012Nov/0011.html
[2] http://lists.w3.org/Archives/Public/public-sysapps/2012Nov/0020.html
Dialer listens to busy event in [1] for playing busy tone. 
CC some gaia devs. to catch their attention and see whether the proposal in comment #0 sounds good to them. More ideas are welcomed! :)


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/handled_call.js#L68
Proposal:
1. remove busy state.
2. carry the disconnect cause in the call event 'disconnected'.

if dialer need the busy information, it can do something like:

call.ondisconnected = function (evt) {
  if (evt.cause == busy) {
    busy();
  }
};
Another proposal.

'busy' should be one of the call error.
We notify it through call's error event.

call.onerror = function errorCB(evt) {
  if (evt.call.error.name == 'BusyError') busy();
};

I prefer this solution because except normal clearance, any other disconnect cause should be regarded as a call error.
Assignee: nobody → szchen
Attached patch WIP: remove ril call state busy (obsolete) — Splinter Review
Modification based on Comment 3.

1. Remove call state busy in RIL, but still keep busy state in telephony for backward compatible.
  => tested. got busy tone
2. Fire error event 'BusyError'
  => app://communications.gaiamobile.org/dialer/js/telephony_helper.js:66 in errorCB: Unexpected error:  BusyError

future work: Keep going and remove the busy state in telephony after we figure out how we like to get notified of busy in dialer
Cc'ing Beatriz who originally reported bug 857951.
(In reply to Szu-Yu Chen [:aknow] from comment #3)
> Another proposal.
> 
> 'busy' should be one of the call error.
> We notify it through call's error event.
> 
> call.onerror = function errorCB(evt) {
>   if (evt.call.error.name == 'BusyError') busy();
> };
> 
> I prefer this solution because except normal clearance, any other disconnect
> cause should be regarded as a call error.

This sounds great to me as well :)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> (In reply to Szu-Yu Chen [:aknow] from comment #3)
> > I prefer this solution because except normal clearance, any other disconnect
> > cause should be regarded as a call error.
> 
> This sounds great to me as well :)
Will this proposal add the possibility to manage other disconnection reasons such as: No response(when called did not pick up the call) or Rejected (when called party rejects the incoming call)?
(In reply to Szu-Yu Chen [:aknow] from comment #3)
> Another proposal.
> 
> 'busy' should be one of the call error.
> We notify it through call's error event.
> 
> call.onerror = function errorCB(evt) {
>   if (evt.call.error.name == 'BusyError') busy();
> };
> 
> I prefer this solution because except normal clearance, any other disconnect
> cause should be regarded as a call error.

Hi guys! I definitely consider more intuitive the comment 2 option also by :aknow ;-) Is the busy state really an error? :-)
(In reply to Beatriz Rodríguez [:brg] from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> > (In reply to Szu-Yu Chen [:aknow] from comment #3)
> > > I prefer this solution because except normal clearance, any other disconnect
> > > cause should be regarded as a call error.
> > 
> > This sounds great to me as well :)
> Will this proposal add the possibility to manage other disconnection reasons
> such as: No response(when called did not pick up the call) or Rejected (when
> called party rejects the incoming call)?

contains following errors
Ref: 3GPP TS 24.008: Cause information element values

"BadNumberError";            
"BusyError";                 <= called is busy
"NoUserRespondingError";     <= network cannot reach the called
"UserAlertingNoAnswerError"; <= called ring, user no answer
"CallRejectedError";         <= user reject
"NumberChangedError";        
"PreEmptionError";           
"DestinationOutOfOrderError";
"InvalidNumberFormatError";  
"FacilityRejectedError";     
"CongestionError";           
"NetworkOutOfOrderError";    
"NetworkTempFailureError";   
"IncomingCallExceededError"; 
"BarredError";               
"FDNBlockedError";           
"SubscriberUnknownError";    
"DeviceNotAcceptedError";    
"UnspecifiedError";
(In reply to Szu-Yu Chen [:aknow] from comment #9)
> (In reply to Beatriz Rodríguez [:brg] from comment #7)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> > > (In reply to Szu-Yu Chen [:aknow] from comment #3)
> > > > I prefer this solution because except normal clearance, any other disconnect
> > > > cause should be regarded as a call error.
> > > 
> > > This sounds great to me as well :)
> > Will this proposal add the possibility to manage other disconnection reasons
> > such as: No response(when called did not pick up the call) or Rejected (when
> > called party rejects the incoming call)?
> 
> contains following errors
> Ref: 3GPP TS 24.008: Cause information element values
> 
> "BadNumberError";            
> "BusyError";                 <= called is busy
> "NoUserRespondingError";     <= network cannot reach the called
> "UserAlertingNoAnswerError"; <= called ring, user no answer
> "CallRejectedError";         <= user reject
> "NumberChangedError";        
> "PreEmptionError";           
> "DestinationOutOfOrderError";
> "InvalidNumberFormatError";  
> "FacilityRejectedError";     
> "CongestionError";           
> "NetworkOutOfOrderError";    
> "NetworkTempFailureError";   
> "IncomingCallExceededError"; 
> "BarredError";               
> "FDNBlockedError";           
> "SubscriberUnknownError";    
> "DeviceNotAcceptedError";    
> "UnspecifiedError";

Thanks :aknow, if we can handle all the disconnection reasons, IMHO it is the best approach. Shall we file a new bug for this or it is already somewhere there?
(In reply to Beatriz Rodríguez [:brg] from comment #10)
> Thanks :aknow, if we can handle all the disconnection reasons, IMHO it is
> the best approach. Shall we file a new bug for this or it is already
> somewhere there?

It's already existed in current code except "BusyError".

The purpose of this bug is to remove the busy state and get it back as one of the error cause. Thus we can handle busy/reject/no response... all in call.onerror().
(In reply to Szu-Yu Chen [:aknow] from comment #11)
> The purpose of this bug is to remove the busy state and get it back as one
> of the error cause. Thus we can handle busy/reject/no response... all in
> call.onerror().
Thanks for the clarification. I got confused because they are disconnection causes/reasons but not all of them are "errors". But now I got it!
See Also: → 883619
I like the solution of removing the busy state, but instead signaling through the error state that the error was due to the callee being busy.
Attached patch Remove call state busy (obsolete) — Splinter Review
Attachment #760788 - Attachment is obsolete: true
Attachment #769554 - Flags: review?(htsai)
File a bug for dialer modification - Bug 888799
Dialer should remove the reference to busy state.
Comment on attachment 769554 [details] [diff] [review]
Remove call state busy

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

Don't we need to modify Telephony.cpp and nsIDOMTelephony.idl accordingly?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> Comment on attachment 769554 [details] [diff] [review]
> Remove call state busy
> 
> Review of attachment 769554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't we need to modify Telephony.cpp and nsIDOMTelephony.idl accordingly?

Sure. I just forgot this part. Thanks for the reminder.
I will add another patch later.
Attachment #769619 - Flags: superreview?(mounir)
Attachment #769619 - Flags: review?(htsai)
Attachment #769622 - Flags: review?(htsai)
Attachment #769622 - Flags: review?(bent.mozilla)
Hi Gina,

CALL_STATE_BUSY is removed.
I add a new parameter in hfp->HandleCallStateChanged() to pass the error code with busy information. Please help me check whether the modifications and new param order are appropriate. It's welcome to suggest a better way for this issue because I'm not familiar with this part. Thanks.
Attachment #769626 - Flags: review?(gyeh)
Attachment #769554 - Attachment is obsolete: true
Attachment #769554 - Flags: review?(htsai)
Attachment #769628 - Flags: review?(htsai)
Attachment #769619 - Flags: superreview?(mounir) → superreview?(bent.mozilla)
Comment on attachment 769619 [details] [diff] [review]
Part 1: Remove call state busy (idl)

I'm not an sr! -> sicking.
Attachment #769619 - Flags: superreview?(bent.mozilla) → superreview?(jonas)
Comment on attachment 769619 [details] [diff] [review]
Part 1: Remove call state busy (idl)

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

Looks good to me!
Attachment #769619 - Flags: review?(htsai) → review+
Attachment #769622 - Flags: review?(htsai) → review+
Attachment #769628 - Flags: review?(htsai) → review+
Comment on attachment 769629 [details] [diff] [review]
Part 5: Modify test_outgoing_busy for removing call state busy

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

Thank you :)
Attachment #769629 - Flags: review?(htsai) → review+
Attachment #769619 - Flags: superreview?(jonas) → superreview+
Comment on attachment 769626 [details] [diff] [review]
Part 3: Remove call state busy (bluetooth dom)

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

Thanks for your help, aknow. I had no problem with this patch, but I'd like to test it before landing, and btain is helping on this. I'll r+ after getting his feedback.
aknow,

I've verified patch part 3 works correctly. Still one minor change is required: patch part 3 conflicts on the latest m-c as following, probably due to our recent change on BluetoothTelephonyListener.cpp.

--
dom/bluetooth/BluetoothTelephonyListener.cpp.rej:

--- dom/bluetooth/BluetoothTelephonyListener.cpp
+++ dom/bluetooth/BluetoothTelephonyListener.cpp
@@ -75,7 +75,7 @@
   // via setting CALL_STATE_DISCONNECTED
   hfp->HandleCallStateChanged(aCallIndex,
                               nsITelephonyProvider::CALL_STATE_DISCONNECTED,
-                              EmptyString(), false, true);
+                              aError, EmptyString(), false, true);
   NS_WARNING("Reset the call state due to call transition ends abnormally");
   NS_WARNING(NS_ConvertUTF16toUTF8(aError).get());
   return NS_OK;
--

Also is dialer app aware of BUSY state removal? Dialer app didn't play busy tone when I was verifying.
Comment on attachment 769626 [details] [diff] [review]
Part 3: Remove call state busy (bluetooth dom)

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

Please update patch based on comment 27. Would like to review the patch again after revisioning.
Attachment #769626 - Attachment is obsolete: true
Attachment #769626 - Flags: review?(gyeh)
Hi Ben,

> Also is dialer app aware of BUSY state removal? Dialer app didn't play busy
> tone when I was verifying.

Bug 888799 (not yet ready now), this is the bug for dialer modification.
Comment on attachment 771255 [details] [diff] [review]
Part 3#2: Remove call state busy (bluetooth dom)

rebase done
Attachment #771255 - Flags: review?(gyeh)
Comment on attachment 771255 [details] [diff] [review]
Part 3#2: Remove call state busy (bluetooth dom)

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

r+ with nits addressed. Thanks!

::: dom/bluetooth/BluetoothHfpManager.h
@@ +86,5 @@
>     * @param aSend A boolean indicates whether we need to notify headset or not
>     */
>    void HandleCallStateChanged(uint32_t aCallIndex, uint16_t aCallState,
> +                              const nsAString& aError, const nsAString& aNumber,
> +                              const bool aIsOutgoing, bool aSend);

Please help to remove the "const" for aIsOutgoing.
Attachment #771255 - Flags: review?(gyeh) → review+
Attachment #769622 - Flags: review?(bent.mozilla) → review+
Blocks: 888799
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.