911986 - [Bluetooth][HFP] Implement voice connection and ICC information change handler for new RIL interfaces

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

In bug 834160 RIL changes interfaces of voice connection and ICC information listener. Bluetooth HFP manager needs to implement new listener, otherwise bluetooth never gets those changes accordingly.

Comment 1

[Ben Tian (inactive)]

Need to implement nsIMobileConnectionListener for voice connection change and nsIIccListener for ICC information change. I'm considering to rename Bluetooth"Telephony"Listener as Bluetooth"Ril"Listener and wrap all 3 listeners into it.

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): bluetooth ril listener for icc info and voice conn change

Changes
- Rename Bluetooth"Telephony"Listener to Bluetooth"Ril"Listener
- Wrap IccInfoListener, MobileConnectionListener, and TelephonyListener from RIL into BluetoothRilListener
- Move EnumerateCalls code into BluetoothRilListener

Hsinyi, please let me know whether BluetoothRilListener manipulates these 3 listeners as RIL expects. Thanks.

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): bluetooth ril listener for icc info and voice conn change

Add commit message.

--
Changes
- Rename Bluetooth"Telephony"Listener to Bluetooth"Ril"Listener
- Wrap IccInfoListener, MobileConnectionListener, and TelephonyListener from RIL into BluetoothRilListener
- Move EnumerateCalls code into BluetoothRilListener

Hsinyi, please let me know whether BluetoothRilListener manipulates these 3 listeners as RIL expects. Thanks.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 799243 [details] [diff] [review]
Patch 1 (v2): bluetooth ril listener for icc info and voice conn change

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

The patch uses ril listeners in the right way. However, Bug 864485 is going to land. We will need according changes here.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +166,5 @@
> +                                      bool aIsActive,
> +                                      bool aIsOutgoing,
> +                                      bool aIsEmergency,
> +                                      bool aIsConference,
> +                                      bool* aResult)

Kindly note Bug 864485 is going to land. There 'boolean enumerateCallState()' is changed to 'void enumerateCallState().'

@@ +171,5 @@
> +{
> +  BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  hfp->HandleCallStateChanged(aCallIndex, aCallState, EmptyString(), aNumber,
> +                              aIsOutgoing, false);
> +  *aResult = true;

The line isn't needed after bug 864485.

@@ +258,5 @@
> +void
> +BluetoothRilListener::EnumerateCalls()
> +{
> +  nsCOMPtr<nsITelephonyProvider> provider =
> +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);

Sorry about architecture changes in RIL recently, but due to bug 864485, here you need:

nsCOMPtr<nsITelephonyProvider> provider = do_GetService(TELEPHONY_PROVIDER_CONTRACTID);

@@ +321,5 @@
> +bool
> +BluetoothRilListener::StartTelephonyListening()
> +{
> +  nsCOMPtr<nsITelephonyProvider> provider =
> +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);

ditto.

@@ +334,5 @@
> +bool
> +BluetoothRilListener::StopTelephonyListening()
> +{
> +  nsCOMPtr<nsITelephonyProvider> provider =
> +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);

ditto.

Comment 5

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 799243 [details] [diff] [review]
Patch 1 (v2): bluetooth ril listener for icc info and voice conn change

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothHfpManager.h
@@ +9,5 @@
>  
>  #include "BluetoothCommon.h"
>  #include "BluetoothProfileManagerBase.h"
>  #include "BluetoothSocketObserver.h"
> +#include "BluetoothRilListener.h"

super-nit: alphabetic order, please

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +273,5 @@
> +    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
> +  NS_ENSURE_TRUE(provider, false);
> +
> +  nsresult rv = provider->RegisterIccMsg(mIccListener);
> +  NS_ENSURE_SUCCESS(rv, false);

NS_FAILED is a macro returning a bool value, so returning NS_FAILED(rv) should be fine. Here and below.

::: dom/bluetooth/BluetoothRilListener.h
@@ +9,5 @@
> +
> +#include "BluetoothCommon.h"
> +
> +#include "nsCOMPtr.h"
> +#include "nsIIccProvider.h"

Forward declaration should be fine. Here and below.

Comment 6

[Ben Tian (inactive)]

Attachment: [final] Patch 1: bluetooth ril listener for icc info and voice conn change, r=echou

Fix nits. The final patch depends on patches of bug b864485.

Comment 7

[Ben Tian (inactive)]

Attachment: [final v2] Patch 1: bluetooth ril listener for icc info and voice conn change, r=echou

Update patch to avoid conflict on the latest code.

try server link: https://tbpl.mozilla.org/?tree=Try&rev=147be8ff795b

Comment 8

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/28c292c41cb7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28c292c41cb7