Closed Bug 911986 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Blocks: 873006
Assignee: nobody → btian
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.
Attachment #799237 - Flags: review?(echou)
Attachment #799237 - Flags: feedback?(htsai)
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.
Attachment #799237 - Attachment is obsolete: true
Attachment #799237 - Flags: review?(echou)
Attachment #799237 - Flags: feedback?(htsai)
Attachment #799243 - Flags: review?(echou)
Attachment #799243 - Flags: feedback?(htsai)
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.
Attachment #799243 - Flags: feedback?(htsai)
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.
Attachment #799243 - Flags: review?(echou) → review+
Depends on: 864485
Fix nits. The final patch depends on patches of bug b864485.
Attachment #799243 - Attachment is obsolete: true
Attachment #800007 - Attachment description: [final] Patch 1: bluetooth ril listener for icc info and voice conn change → [final] Patch 1: bluetooth ril listener for icc info and voice conn change, r=echou
Blocks: 912005
No longer blocks: 873006
Update patch to avoid conflict on the latest code.

try server link: https://tbpl.mozilla.org/?tree=Try&rev=147be8ff795b
Attachment #800007 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/28c292c41cb7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.