[Bluetooth][bluedroid] Add ConnectSco/DisconnectSco into BluetoothServiceBluedroid

RESOLVED FIXED in 1.3 Sprint 6 - 12/6

Status

RESOLVED FIXED
5 years ago
5 years ago

People

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

Tracking

unspecified
1.3 Sprint 6 - 12/6
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Implement ConnectSco/DisconnectSco into BluetoothServiceBluedroid for speaker/receiver audiopath switch during call.
(Assignee)

Comment 1

5 years ago
Created attachment 8340198 [details] [diff] [review]
Patch 1 (v1): ConnectSco/DisconnectSco in BluetoothServiceBluedroid
Assignee: nobody → btian
Attachment #8340198 - Flags: review?(echou)
(Assignee)

Updated

5 years ago
Depends on: 915533
Comment on attachment 8340198 [details] [diff] [review]
Patch 1 (v1): ConnectSco/DisconnectSco in BluetoothServiceBluedroid

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

Hi Ben, please see my comments below.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1360,3 @@
>  
> +  BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  NS_ENSURE_TRUE_VOID(hfp);

We can't leave this function without handling aRuannble.

@@ +1363,5 @@
> +  if (!hfp->ConnectSco()) {
> +    NS_NAMED_LITERAL_STRING(replyError,
> +      "SCO exists or HFP is not connected");
> +    DispatchBluetoothReply(aRunnable, BluetoothValue(), replyError);
> +  }

DispatchBluetoothReply is also necessary if hfp->ConnectSco() succeeds.

@@ +1374,3 @@
>  
> +  BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  NS_ENSURE_TRUE_VOID(hfp);

We can't leave this function without handling aRuannble.

@@ +1392,3 @@
>  
> +  BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  NS_ENSURE_TRUE_VOID(hfp);

We can't leave this function without handling aRuannble.
Attachment #8340198 - Flags: review?(echou) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 8340262 [details] [diff] [review]
Patch 1 (v2): ConnectSco/DisconnectSco in BluetoothServiceBluedroid
Attachment #8340198 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 8340859 [details] [diff] [review]
Patch 1 (v3): ConnectSco/DisconnectSco in BluetoothServiceBluedroid

Changes:
- Implemnt ConnectSco, DisconnectSco, and IsScoConnected functions in BluetoothServiceBluedroid
- Revise ConnectSco, DisconnectSco, and IsScoConnected functions in BluetoothDBusService

Note the runnable reply timing of ConnectSco is different between bluez and bluedroid: bluez replies at BluetoothHfpManager::OnScoConnectSuccess/OnScoConnectError, whereas bluedroid replies at BluetoothServiceBluedroid::ConnectSco. The reason is that bluedroid HFP is unaware of Sco connection error but only state change.
Attachment #8340262 - Attachment is obsolete: true
Attachment #8340859 - Flags: review?(echou)
Comment on attachment 8340859 [details] [diff] [review]
Patch 1 (v3): ConnectSco/DisconnectSco in BluetoothServiceBluedroid

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

r=me with nits addressed. Thanks.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1367,5 @@
> +    return;
> +  }
> +
> +  DispatchBluetoothReply(aRunnable,
> +                         BluetoothValue(true), EmptyString());

super-nit: no need to wrap.

@@ +1383,5 @@
> +    return;
> +  }
> +
> +  DispatchBluetoothReply(aRunnable,
> +                         BluetoothValue(true), EmptyString());

ditto.

@@ +1399,5 @@
> +    return;
> +  }
> +
> +  DispatchBluetoothReply(aRunnable,
> +                         hfp->IsScoConnected(), EmptyString());

ditto.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +3028,5 @@
>      return;
>    }
>  
> +  DispatchBluetoothReply(aRunnable,
> +                         BluetoothValue(true), EmptyString());

super-nit: no need to wrap.

@@ +3047,2 @@
>    DispatchBluetoothReply(aRunnable,
>                           hfp->IsScoConnected(), EmptyString());

Ditto. Would you mind revising this as well? :)
Attachment #8340859 - Flags: review?(echou) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 8341439 [details] [diff] [review]
[final] Patch 1: ConnectSco/DisconnectSco in BluetoothServiceBluedroid, r=echou

Revise nits.
Attachment #8340859 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
(In reply to Ben Tian [:btian] from comment #7)
> try server: https://tbpl.mozilla.org/?tree=Try&rev=5828cc1c595b

The JB emulator build failure resulted from certain ccache permission denial error that is irrelate to bluetooth. Re-try: https://tbpl.mozilla.org/?tree=Try&rev=229d1969f686
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb14e88896ea
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
You need to log in before you can comment on or make changes to this bug.