Closed Bug 808879 Opened 7 years ago Closed 7 years ago

expose Bluetooth earphone connection status to GAIA

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: alive, Assigned: gyeh)

Details

Attachments

(1 file, 2 obsolete files)

This bug is separated from bug 791642.

We are going to support multiple audio streams and the bluetooth device setting is superior to the phone setting.
So Gaia System app sound manager needs to know the state of bluetooth earphone's connection, therefore hardware volume buttons change could be applied to the respective settings.
Assignee: nobody → alive
blocking-basecamp: --- → ?
For Gaia side, we send a chrome event with a value from shell.js to sound_manager.js.

Now, we have multiple audio streams and we should only 'audio.volume.bt_sco' whenever a bluetooth headset is connected, and its value is changed either pressing the volume buttons on bluetooth headsets or pressing volume-up/down buttons on our phones. So, we need to expose bluetooth connection status for Gaia.

I guess we probably want to expose this function in BluetoothManager rather than in BluetoothAdapter. Because it's easier for Gaia to call mozBluetooth.isConnected with a profile id, like what we did in Connect(), instead of get an adapter each time when they want to check bluetooth connection status. (If we don't do this, the instance of BluetoothAdapter might be expired after toggling bluetooth)
Assignee: alive → gyeh
Attachment #678636 - Flags: review?(kyle)
Comment on attachment 678636 [details] [diff] [review]
Patch 1(v1): Expose connection status for system app

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

I don't think we need the new IsConnected() functions here. UnixSocketConsumer has a public GetConnectionStatus() function, which is exposed via the public inheritance thru the managers. So that should trim down the code needed since you can remove the IsConnected() functions from the Hfp/Opp managers. Other than that, this looks decent, but is also doing to need a DOM Peer review. So, r-'ing but fix that up plus the nit, and assign me and mrbkap for review, and this should go thru. :)

::: dom/bluetooth/BluetoothOppManager.h
@@ +47,5 @@
>     */
>    bool Connect(const nsAString& aDeviceObjectPath,
>                 BluetoothReplyRunnable* aRunnable);
>    void Disconnect();
> +  bool IsConnected()

Why is the code for OPP manager different than HfpManager?

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2392,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");
> +
> +  nsString errorStr;
> +  BluetoothValue v = true;

nit: Doesn't look like this gets used anywhere?
Attachment #678636 - Flags: review?(kyle) → review-
You are right! We could just call GetConnectionStatus from BluetoothService. It makes the implementation much cleaner :)
Attachment #678636 - Attachment is obsolete: true
Attachment #678644 - Flags: review?(kyle)
Comment on attachment 678644 [details] [diff] [review]
Patch 1(v2): Expose connection status for system app

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

Looks good to me! Assigning DOM peer review to mrbkap. We can assault him to get it done in the morning.

::: dom/bluetooth/BluetoothHfpManager.h
@@ +27,5 @@
>    bool Connect(const nsAString& aDeviceObjectPath,
>                 const bool aIsHandsfree,
>                 BluetoothReplyRunnable* aRunnable);
>    void Disconnect();
> +

Nit: extra line
Attachment #678644 - Flags: review?(mrbkap)
Attachment #678644 - Flags: review?(kyle)
Attachment #678644 - Flags: review+
Comment on attachment 678644 [details] [diff] [review]
Patch 1(v2): Expose connection status for system app

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

Nits only.

::: dom/bluetooth/BluetoothManager.cpp
@@ +276,5 @@
> +  }
> +
> +  *aConnected = bs->IsConnected(aProfileId);
> +  return NS_OK;
> +}

Nit: Please add a newline after the closing brace.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2394,5 @@
> +
> +  if (aProfileId == (uint16_t)(BluetoothServiceUuid::Handsfree >> 32)
> +      || aProfileId == (uint16_t)(BluetoothServiceUuid::Headset >> 32)) {
> +    BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +    return (hfp->GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTED)

Nit: No need to do "boolean_expression ? true : false". That reads better as simply "boolean_expression".

@@ +2396,5 @@
> +      || aProfileId == (uint16_t)(BluetoothServiceUuid::Headset >> 32)) {
> +    BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +    return (hfp->GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTED)
> +            ? true : false;
> +  } else if (aProfileId == (uint16_t)(BluetoothServiceUuid::ObjectPush >> 32)) {

No need for else after return.
Attachment #678644 - Flags: review?(mrbkap) → review+
blocking-basecamp: ? → +
https://hg.mozilla.org/mozilla-central/rev/63b7d971934c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.