948337 - [gonk-kk]Build breaks in dom/system/gonk/AudioManager.cpp if flag MOZ_B2G_BT and MOZ_B2G_BT_BLUEZ/MOZ_B2G_BT_BLUEDROID were removed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

All Bluetooth dependency required to use #ifdef MOZ_B2G_BT to block.

Comment 1

[amitav.anand]

Attachment: configure.in

Please check the attached file in B2G/gecko/configure.in. defined MOZ_B2G_BT = 0. The build went on fine for nexus-4. Kindly comment.

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This was reported when build non phone product.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Keven,
Would you mind to provide more detail (such as build configuration)?

Comment 4

[Keven Kuo [:kkuo]]

Hi! Shawn,

The build break is caused by removing "MOZ_B2G_BT" define in gecko/configure.in at API version 17/18 (JB).
Error message is not able to find file "BluetoothHfpManager.h".
My temporary solution is as following.

diff --git a/dom/system/gonk/AudioManager.cpp b/dom/system/gonk/AudioManager.cpp
index cb3ab83..8a01091 100644
--- a/dom/system/gonk/AudioManager.cpp
+++ b/dom/system/gonk/AudioManager.cpp
@@ -32,7 +32,9 @@
 
 #include "BluetoothCommon.h"
 #include "BluetoothProfileManagerBase.h"
+#ifdef MOZ_B2G_BT
 #include "BluetoothHfpManager.h"
+#endif
 
 #include "nsJSUtils.h"
 #include "nsCxPusher.h"

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

In dom/system/gonk/moz.build
# include different BluetoothHfpManager.h under dom/bluetooth/
if CONFIG['MOZ_B2G_BT_BLUEZ']:
    LOCAL_INCLUDES += [
        '/dom/bluetooth/bluez',
    ]
elif CONFIG['MOZ_B2G_BT_BLUEDROID']:
    LOCAL_INCLUDES += [
        '/dom/bluetooth/bluedroid',
    ]

I guess Comment 4 'by removing "MOZ_B2G_BT" define', MOZ_B2G_BT_BLUEDROID or MOZ_B2G_BT_BLUEZ flag was removed.
Keven, does our backend flag was been removed? Set MOZ_B2G_BT=0 shall build pass.
Any reason to remove bluetooth backend flag?

Comment 6

[Keven Kuo [:kkuo]]

Hi! Shawn,

If I set MOZ_B2G_BT=0, MOZ_B2G_BT_BLUEDROID=0 or MOZ_B2G_BT_BLUEZ=0, the build break still occurs.

/home/ymkuo/github_keven/B2G/gecko/dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp:13:28: fatal error: hardware/bt_rc.h: No such file or directory

--
Keven

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This is because we moved AVRCP supportness to Bluedroid based on Android 4.3, so Bluetooth HAL is based on Android 4.3 instead of 4.2. I guess you're using 4.2 below. I will open another for AVRCP 1.3 if supporting Android 4.2 is necessary.

Comment 8

[Keven Kuo [:kkuo]]

yes, it is 4.2 based.

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #7)
> This is because we moved AVRCP supportness to Bluedroid based on Android
> 4.3, so Bluetooth HAL is based on Android 4.3 instead of 4.2. I guess you're
> using 4.2 below. I will open another for AVRCP 1.3 if supporting Android 4.2
> is necessary.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Per discussion with mwu, we will add BluetoothHfpManager header file no matter what bluetooth backend is.

Comment 11

[Ben Tian (inactive)]

Attachment: WIP patch (v1): create BluetoothHfpManagerBase for AudioManager access

The patch creates BluetoothHfpManagerBase.h under dom/bluetooth/. AudioManager is able to look it up no matter which backend is used. Will verify further on bluez and bluedroid platforms.

Comment 12

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): create BluetoothHfpManagerBase for AudioManager access

Create BluetoothHfpManagerBase.h for AudioManager including no matter which backend is used.

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8359078 [details] [diff] [review]
Patch 1 (v2): create BluetoothHfpManagerBase for AudioManager access

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

Overall, it looks good to me.

Comment 14

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

Comment on attachment 8359078 [details] [diff] [review]
Patch 1 (v2): create BluetoothHfpManagerBase for AudioManager access

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

In my opinion, we should unhook AudioManager and Bluetooth*Manager as much as possible. We can send the new status as an argument of the notification to achieve this goal. Accordingly we may need a ',' separated string as the argument of the notification because the handler can only take a PRUnichar*. Ben didn't like that solution and we couldn't come to a conclusion. I'm ok with landing this for now and we should keep in mind that there may be a better solution for this.

::: dom/bluetooth/BluetoothHfpManagerBase.h
@@ +21,5 @@
> +};
> +
> +#define BT_DECL_HFP_MGR_BASE                  \
> +  BT_DECL_PROFILE_MGR_BASE                    \
> +  virtual bool IsScoConnected() MOZ_OVERRIDE; \

The last '\' seems unnecessary.

::: dom/system/gonk/moz.build
@@ -108,5 @@
> -elif CONFIG['MOZ_B2G_BT_BLUEDROID']:
> -    LOCAL_INCLUDES += [
> -        '/dom/bluetooth/bluedroid',
> -    ]
> -

Although this looks good to me, I don't think I have rights to review moz.build under dom/system/gonk. Please redirect the review to the module owner.

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8359078 [details] [diff] [review]
Patch 1 (v2): create BluetoothHfpManagerBase for AudioManager access

:mwu, can you review the system/gonk/moz.build change that removes BT build flags?

Comment 16

[Andreas Gal :gal]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c0a98a6255

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04c0a98a6255