812391 - [b2g-bluetooth] Support audio stream of bt_sco

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

We have a volume settings for bt_sco, 'audio.volume.bt_sco', which is an integer between 0 to 15.

When there's a connected bluetooth headset, we can adjust this volume value by pressing volume buttons from headset. Plus, we can also press hardware buttons on our phone to change its value during a phone call.

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Support audio stream of bt_sco

Comment 2

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

Comment on attachment 682289 [details] [diff] [review]
Patch 1(v1): Support audio stream of bt_sco

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

r+ with nits addressed and questions answered.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +19,5 @@
>  #include "mozilla/StaticPtr.h"
>  #include "nsContentUtils.h"
>  #include "nsIAudioManager.h"
>  #include "nsIObserverService.h"
> +#include "nsISettingsService.h"

nit: please keep these in alphabetical order

@@ +138,5 @@
>      Shutdown();
>    }
>  };
>  
> +class BluetoothHfpManager::GetVolumeTask : public nsISettingsServiceCallback

Since we don't use any member variable or non-public functions in this class, maybe we can move it out of BluetoothHfpManager.

@@ +398,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    // The string that we're interested in will be a JSON string that looks like:
> +  //  {"key":"volumeup", "value":10}
> +  //  {"key":"volumedown", "value":}

Not-even-a-nit: miss the value of "value" here?

@@ +463,1 @@
>    SendCommand("+VGS: ", mCurrentVgs);

How about changing the condition of the previous if-expression to "if (GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTED)" and calling SendCommand() in the block? It needs less code.

::: dom/bluetooth/BluetoothHfpManager.h
@@ +40,4 @@
>  
>  private:
> +  class GetVolumeTask;
> +  friend class GetVolumeTask;

We can remove these two lines if we move class GetVolumeTask out of BluetoothHfpManager.

::: dom/bluetooth/BluetoothUtils.cpp
@@ +9,2 @@
>  #include "BluetoothUtils.h"
> +#include "BluetoothReplyRunnable.h"

nit: please keep these in alphabetical order (I think lines below need to be revised as well)

@@ +119,5 @@
> +void
> +mozilla::dom::bluetooth::DispatchBluetoothReply(
> +                                              BluetoothReplyRunnable* aRunnable,
> +                                                   const BluetoothValue& aValue,
> +                                                     const nsAString& aErrorStr)

The function name seems to be too long. I found that functions in BluetoothUtils.cpp all belong to namespace mozilla::dom::bluetooth, how about using BEGIN_BLUETOOTH_NAMESPACE/END_BLUETOOTH_NAMESPACE to wrap all these functions? Then we can put the first parameter at the same line as the function name.

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1: Support audio stream of bt_sco, r=echou

try:
https://tbpl.mozilla.org/?tree=Try&rev=f5d6a516380f
https://tbpl.mozilla.org/?tree=Try&rev=2b4233a9c39a

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/314db04e1fa7

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/314db04e1fa7

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9218c2841ead

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Had to back this out due to build failures. Looks like this may need a branch-specific patch.
https://hg.mozilla.org/releases/mozilla-beta/rev/28a256a273f8

https://tbpl.mozilla.org/php/getParsedLog.php?id=17265745&tree=Mozilla-Beta

/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp: In function 'nsresult mozilla::dom::bluetooth::SetJsObject(JSContext*, JSObject*, const InfallibleTArray<mozilla::dom::bluetooth::BluetoothNamedValue>&)':
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:24: error: new declaration 'nsresult mozilla::dom::bluetooth::SetJsObject(JSContext*, JSObject*, const InfallibleTArray<mozilla::dom::bluetooth::BluetoothNamedValue>&)'
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.h:32: error: ambiguates old declaration 'bool mozilla::dom::bluetooth::SetJsObject(JSContext*, JSObject*, const InfallibleTArray<mozilla::dom::bluetooth::BluetoothNamedValue>&)'
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:29: error: 'aCx' was not declared in this scope
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:30: error: 'aGlobal' was not declared in this scope
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:34: error: 'aSourceArray' was not declared in this scope
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:63: error: 'aResultArray' was not declared in this scope
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp: At global scope:
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:69: error: 'BluetoothDevice' was not declared in this scope
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:69: error: template argument 1 is invalid
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:69: error: template argument 1 is invalid
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:70: error: explicit qualification in declaration of 'nsresult mozilla::dom::bluetooth::BluetoothDeviceArrayToJSArray(JSContext*, JSObject*, const int&, JSObject**)'
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp: In function 'nsresult mozilla::dom::bluetooth::BluetoothDeviceArrayToJSArray(JSContext*, JSObject*, const int&, JSObject**)':
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:80: error: request for member 'IsEmpty' in 'aSourceArray', which is of non-class type 'const int'
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:83: error: request for member 'Length' in 'aSourceArray', which is of non-class type 'const int'
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:87: error: invalid types 'const int[uint32_t]' for array subscript
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp: At global scope:
/builds/slave/m-beta-ics-armv7a-gecko/build/dom/bluetooth/BluetoothUtils.cpp:111: error: explicit qualification in declaration of 'bool mozilla::dom::bluetooth::SetJsObject(JSContext*, JSObject*, const InfallibleTArray<mozilla::dom::bluetooth::BluetoothNamedValue>&)'

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

I think it's because Bug798123 is not merged into Mozilla-beta(mozilla 18). Add Bug798123 as blocker, can we mark it as a bb+, too?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2714e1ebb8b2