936732 - [Bluetooth] Refactor dom/bluetooth for both BlueZ and Bluedroid compatibility

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Refactor dom/bluetooth to compile different files of BluetoothService and profile managers for BlueZ and Bluedroid, but keep the file names identical for other common files’ use (e.g., profile controller, BluetoohUuid, a nd AudioManager).

Comment 1

[Ben Tian (inactive)]

My initial thought is to categorize files into 3 folders: common, bluez, and bluedroid
- common
    all files are common for BlueZ and Bluedroid, including profile controller, files under /ipc, etc.
- bluez
    all files specific to BlueZ, including current profile manager files and files under /linux and /gonk.
- bluedroid
    all files specific to Bluedroid, including BluetoothServiceBluedroid.h/.cpp and profile manager files for Bluedroid.

Also need to rewrite moz.build to compile files under bluez/bluedroid folder based on BlueZ/Bluedroid.

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 1/2 (v1): relocate files

New folders: bluez, bluedroid, interfaces
bluez
- gonk/, linux/, and hfp manager.
bluedroid
- gonk/ (BluetoothServiceBluedroid) and hfp manager.
interfaces
- .idl files

Note bluedroid/hfp manager will be replaced in bug 911943.

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1/2 (v2): relocate files

3 new folders: bluez, bluedroid, and interfaces
- move gonk/ and linux/ into bluez/
- move BluetoothServiceBluedroid into bluedroid/
- move BluetoothHfpManager into both bluez/ and bluedroid/
- move .idl files into interfaces
- revise moz.build

TODO: replace bluedroid/BleutoothHfpManager for bluedroid in bug 911943.

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 2/2 (v1): build file changes

Change:
- refine logic of bluetooth configuration flags
    if MOZ_B2G_BT:
      if BLUEZ:
      else if BLUEDROID:
    else if MOZ_BLUETOOTH_DBUS
- revise system/gonk/moz.build to check different 'include' folders according to bluez/bluedroid
- remove redundant header including

Marco, please help review system/gonk/moz.build change. Thanks.

Comment 5

[Ben Tian (inactive)]

I've verified the build pass of these patches on ICS and JB debug builds. Note the latest desktop build with b2g-bt enabled [1] has build faliure and we'll solve it in another follow-up bug.

[1] https://wiki.mozilla.org/B2G/Bluetooth#Example_for_.mozconfig
    ac_add_options --enable-b2g-bt

Comment 6

[Ben Tian (inactive)]

> Note the latest desktop build with b2g-bt enabled [1] has build faliure and
> we'll solve it in another follow-up bug.
Track desktop build failure on bug 938122.

Comment 7

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

Comment on attachment 831380 [details] [diff] [review]
Patch 2/2 (v1): build file changes

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

In general it looks good to me, i think you can combine this patch into Patch 1 except dom/system/gonk/moz.build. It will be good if you use clear change title.

::: dom/bluetooth/BluetoothService.cpp
@@ +45,5 @@
>  #if defined(MOZ_B2G_BT)
> +  #ifdef MOZ_B2G_BT_BLUEZ
> +    #include "BluetoothGonkService.h"
> +  #else
> +    #ifdef MOZ_B2G_BT_BLUEDROID

how about #elif defined MOZ_B2G_BT_BLUEDROID

@@ +55,2 @@
>  # else
> +  #  error No_suitable_backend_for_bluetooth!

super nit: Although it's not related to your patch, maybe we can have the same indentation style?

@@ +312,5 @@
> +    #ifdef MOZ_B2G_BT_BLUEDROID
> +      return new BluetoothServiceBluedroid();
> +    #endif
> +  #endif
> +# elif defined(MOZ_BLUETOOTH_DBUS)

super nit: '#elif' instead of '# elif'. It would be good to comment when MOZ_BLUETOOTH_DBUS will be used. Since these flags are very confused.

::: dom/bluetooth/Makefile.in
@@ +30,5 @@
> +		LOCAL_INCLUDES += $(MOZ_DBUS_CFLAGS)
> +		CFLAGS += $(MOZ_DBUS_GLIB_CFLAGS)
> +		CXXFLAGS += $(MOZ_DBUS_GLIB_CFLAGS) -DHAVE_PTHREADS
> +		DEFINES += -DMOZ_BLUETOOTH_DBUS
> +	endif

Indentation in Makefile.in seems not common. I don't think it's a good idea to indent using TAB here, since i checked the whole codebase, I don't see much Please don't indent here.

::: dom/bluetooth/moz.build
@@ +67,5 @@
> +                ]
> +                LOCAL_INCLUDES += [
> +                    'bluedroid',
> +                    'bluedroid/gonk',
> +                ]

Can we move this part into patch 1 instead of leaving this piece? And do we need bluedroid/gonk?

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

I think it's better to combine this piece code into Patch 1.

Comment 8

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

Comment on attachment 831378 [details] [diff] [review]
Patch 1/2 (v2): relocate files

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

If we are going to spread files to access bluetooth HAL APIs in profiles level, such as BluetoothHfpManager, BluetoothServiceBluedroid, BluetoothA2dpManager, perhaps the folder gonk can ignored? Because we are willing to make thinner GAP implementation in BlueServiceBluedroid.cpp, and all files under bluedroid all access bluedroid api. I think it really depends on how we define gonk folder.

Comment 9

[Ben Tian (inactive)]

> If we are going to spread files to access bluetooth HAL APIs in profiles
> level, such as BluetoothHfpManager, BluetoothServiceBluedroid,
> BluetoothA2dpManager, perhaps the folder gonk can ignored? Because we are
> willing to make thinner GAP implementation in BlueServiceBluedroid.cpp, and
> all files under bluedroid all access bluedroid api. I think it really
> depends on how we define gonk folder.

The bluedroid/gonk/ folder is created to be consistent with original bluez structure. I prefer to keep the consistency at this bug and discuss whether to keep the gonk/ folders as a follow-up.

Comment 10

[Ben Tian (inactive)]

Attachment: Patch 1/3 (v1): Relocate files

Re-split patches:
- 1/3: relocate files
- 2/3: bt file changes
- 3/3: system/gonk make file change

Comment 11

[Ben Tian (inactive)]

Attachment: Patch 2/3: BT file changes

BT file changes based on Shawn's feedback, including Makefile.in indent removal.

Comment 12

[Ben Tian (inactive)]

Attachment: Patch 3/3: Separate bluez/bluedroid header files for system/gonk

system/gonk/moz.build change to include different BT HFP header files based on blueZ or bluedroid.

Comment 13

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

Comment on attachment 831993 [details] [diff] [review]
Patch 3/3: Separate bluez/bluedroid header files for system/gonk

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

How about change this chnageset title to "Separate bluez/bluedroid header files for system/gonk"? "system/gonk file change" looks ambiguous.

Comment 14

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

Comment on attachment 831992 [details] [diff] [review]
Patch 2/3: BT file changes

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

It looks good to me.

Comment 15

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

Comment on attachment 831991 [details] [diff] [review]
Patch 1/3 (v1): Relocate files

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

I'm ok with the movement of Bluetooth* files, but I think your moz.build should be placed in dom/interfaces/bluetooth instead of dom/bluetooth/interfaces. I'll review again once questions are answered. Thanks.

::: dom/bluetooth/interfaces/moz.build
@@ +1,5 @@
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> +# vim: set filetype=python:
> +# Copyright 2013 Mozilla Foundation and Mozilla contributors
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");

Why Apache License? I've checked moz.build under dom/interfaces/* and didn't see anything related to Apache License.

Comment 16

[Ben Tian (inactive)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #15)
> I'm ok with the movement of Bluetooth* files, but I think your moz.build
> should be placed in dom/interfaces/bluetooth instead of
> dom/bluetooth/interfaces. I'll review again once questions are answered.
> Thanks.
I refer to dom/icc, dom/messages, and dom/network to put .idl file into one interfaces folder. The move is to make files under dom/bluetooth more organized.

> ::: dom/bluetooth/interfaces/moz.build
> @@ +1,5 @@
> > +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > +# vim: set filetype=python:
> > +# Copyright 2013 Mozilla Foundation and Mozilla contributors
> > +#
> > +# Licensed under the Apache License, Version 2.0 (the "License");
> 
> Why Apache License? I've checked moz.build under dom/interfaces/* and didn't
> see anything related to Apache License.
I use the same license statement as dom/bluetooth/moz.build.

Comment 17

[Ben Tian (inactive)]

Comment on attachment 831991 [details] [diff] [review]
Patch 1/3 (v1): Relocate files

Re-request for patch 1 review.

Comment 18

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

Comment on attachment 831993 [details] [diff] [review]
Patch 3/3: Separate bluez/bluedroid header files for system/gonk

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

::: dom/system/gonk/moz.build
@@ +85,5 @@
>          'ril_worker.js',
>      ]
>  
> +# include different BluetoothHfpManager.h under dom/bluetooth/
> +if CONFIG['MOZ_B2G_BT_BLUEZ'] or CONFIG['MOZ_ENABLE_DBUS']:

Let's assume "MOZ_ENABLE_DBUS + MOZ_B2G_BT + MOZ_B2G_BLUEZ", it's not possible that MOZ_B2G_BT + MOZ_ENABLE_DBUS without MOZ_B2G_BLUEZ. So we can remove CONFIG['MOZ_ENABLE_DBUS']

Comment 19

[Ben Tian (inactive)]

Attachment: Patch 2/3 (v2): BT file changes

Simplify moz.build with elif statement.

Comment 20

[Ben Tian (inactive)]

Attachment: Patch 3/3 (v2): Separate bluez/bluedroid header files for system/gonk

Modify dom/system/gonk/moz.build to include correspondent BluetoothHfpManager.h in AudioManager.cpp based on different bluetooth backend (blueZ/bluedroid).

Kyle, can you help review my patch?

Comment 21

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

Comment on attachment 832083 [details] [diff] [review]
Patch 2/3 (v2): BT file changes

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

Bluetooth related logic looks good to me. However I would like to suggest that you should ask a build system expert for another review. Mike Hommey might be the person.

Comment 22

[Ben Tian (inactive)]

Attachment: Patch 2/3 (v3): BT file changes

Revise license statement and comment in Makefile.in.

Comment 23

[Ben Tian (inactive)]

Comment on attachment 832097 [details] [diff] [review]
Patch 2/3 (v3): BT file changes

Revise license statement and comment in Makefile.in according to offline discussion with Eric.

--
Mike, can you help review my patch about bluetooth build files?

The patch mainly modifies build files to build correspondent folders based on different bluetooth backends (blueZ/bluedroid). We have MOZ_B2G_BT_BLUEZ and MOZ_B2G_BT_BLUEDROID defined on gonk build to build /bluez and /bluedroid respectfully; MOZ_ENABLE_DBUS is defined on desktop build and implies blueZ is required.

Comment 24

[Ben Tian (inactive)]

Attachment: [final] Patch 1/3: Relocate files, r=echou

Rebase patch 1 based on imminent bug 937973 comment 9 change.

Comment 25

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

Comment on attachment 832136 [details] [diff] [review]
[final] Patch 1/3: Relocate files, r=echou

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

r+ except I still don't think that we need to claim moz.build is licensed under Apache, not pretty sure, though.

Comment 26

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

Comment on attachment 832136 [details] [diff] [review]
[final] Patch 1/3: Relocate files, r=echou

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

r+ except I still don't think that we need to claim moz.build is licensed under Apache, not pretty sure, though.

Comment 27

[Ben Tian (inactive)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #26)
> Comment on attachment 832136 [details] [diff] [review]
> Patch 1/3 (v2): Relocate files
> 
> Review of attachment 832136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ except I still don't think that we need to claim moz.build is licensed
> under Apache, not pretty sure, though.

I update moz.build license statement in patch 2/3 (comment 23) as patch 1/3 only relocates files.

Comment 28

[Ben Tian (inactive)]

Comment on attachment 832097 [details] [diff] [review]
Patch 2/3 (v3): BT file changes

The patch mainly modifies build files to build correspondent folders based on different bluetooth backends (blueZ/bluedroid). We have MOZ_B2G_BT_BLUEZ and MOZ_B2G_BT_BLUEDROID defined on gonk build to build /bluez and /bluedroid respectfully; MOZ_ENABLE_DBUS is defined on desktop build and implies blueZ is required.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 832097 [details] [diff] [review]
Patch 2/3 (v3): BT file changes

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

::: dom/bluetooth/BluetoothService.cpp
@@ +58,2 @@
>  #include "BluetoothServiceBluedroid.h"
>  #endif

There should also be an

#else
#error No backend

for the if MOZ_B2G_BT_BLUEZ else MOZ_B2G_BT_BLUEDROID branch.

@@ +320,5 @@
> +#if defined(MOZ_B2G_BT_BLUEZ)
> +  /**
> +   * B2G blueZ:
> +   *   MOZ_B2G_BT and MOZ_B2G_BT_BLUEZ are both defined.
> +   */

You don't need to repeat these comments everywhere.

Comment 30

[Ben Tian (inactive)]

Attachment: [final] Patch 2/3: BT file changes, r=khuey

Revise based on reviewer's comment 29.

Comment 31

[Ben Tian (inactive)]

Attachment: [final] Patch 3/3: Separate bluez/bluedroid header files for system/gonk, r=khuey

Rebase onto the latest system/gonk.

Comment 32

[Ben Tian (inactive)]

try server: https://tbpl.mozilla.org/?tree=Try&rev=f8e352b9ba79

Comment 33

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

https://hg.mozilla.org/integration/b2g-inbound/rev/480efb125004
https://hg.mozilla.org/integration/b2g-inbound/rev/643b3072fb12
https://hg.mozilla.org/integration/b2g-inbound/rev/fbd99adbd77d

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/480efb125004
https://hg.mozilla.org/mozilla-central/rev/643b3072fb12
https://hg.mozilla.org/mozilla-central/rev/fbd99adbd77d