Closed Bug 936732 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 5 - 11/22
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

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

References

Details

Attachments

(3 files, 9 obsolete files)

517.72 KB, patch
echou
: review+
Details | Diff | Splinter Review
9.06 KB, patch
Details | Diff | Splinter Review
969 bytes, patch
Details | Diff | Splinter Review
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).
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.
Assignee: nobody → btian
Blocks: 911943
Attached patch Patch 1/2 (v1): relocate files (obsolete) — Splinter Review
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.
Attachment #831365 - Flags: review?(echou)
Attachment #831365 - Flags: review?(echou)
Attached patch Patch 1/2 (v2): relocate files (obsolete) — Splinter Review
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.
Attachment #831365 - Attachment is obsolete: true
Attachment #831378 - Flags: review?(echou)
Attachment #831378 - Flags: feedback?(shuang)
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.
Attachment #831380 - Flags: review?(mchen)
Attachment #831380 - Flags: review?(echou)
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
Attachment #831380 - Flags: feedback?(shuang)
> 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 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.
Attachment #831380 - Flags: feedback?(shuang) → feedback+
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.
Attachment #831378 - Flags: feedback?(shuang) → feedback+
> 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.
Attached patch Patch 1/3 (v1): Relocate files (obsolete) — Splinter Review
Re-split patches:
- 1/3: relocate files
- 2/3: bt file changes
- 3/3: system/gonk make file change
Attachment #831378 - Attachment is obsolete: true
Attachment #831380 - Attachment is obsolete: true
Attachment #831378 - Flags: review?(echou)
Attachment #831380 - Flags: review?(mchen)
Attachment #831380 - Flags: review?(echou)
Attachment #831991 - Flags: review?(echou)
Attachment #831991 - Flags: feedback?(shuang)
Attached patch Patch 2/3: BT file changes (obsolete) — Splinter Review
BT file changes based on Shawn's feedback, including Makefile.in indent removal.
Attachment #831992 - Flags: review?(echou)
Attachment #831992 - Flags: feedback?(shuang)
system/gonk/moz.build change to include different BT HFP header files based on blueZ or bluedroid.
Attachment #831993 - Flags: review?(mchen)
Attachment #831993 - Attachment description: Patch 2/3: system/gonk file change → Patch 3/3: system/gonk file change
Attachment #831993 - Flags: feedback?(shuang)
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.
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Attachment #831993 - Attachment description: Patch 3/3: system/gonk file change → Patch 3/3: Separate bluez/bluedroid header files for system/gonk
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.
Attachment #831992 - Flags: feedback?(shuang) → feedback+
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.
Attachment #831991 - Flags: review?(echou)
(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 on attachment 831991 [details] [diff] [review]
Patch 1/3 (v1): Relocate files

Re-request for patch 1 review.
Attachment #831991 - Flags: review?(echou)
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']
Attached patch Patch 2/3 (v2): BT file changes (obsolete) — Splinter Review
Simplify moz.build with elif statement.
Attachment #831992 - Attachment is obsolete: true
Attachment #831992 - Flags: review?(echou)
Attachment #832083 - Flags: review?(echou)
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?
Attachment #831993 - Attachment is obsolete: true
Attachment #831993 - Flags: review?(mchen)
Attachment #831993 - Flags: feedback?(shuang)
Attachment #832094 - Flags: review?(khuey)
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.
Attachment #832083 - Flags: review?(echou) → review+
Attached patch Patch 2/3 (v3): BT file changes (obsolete) — Splinter Review
Revise license statement and comment in Makefile.in.
Attachment #832083 - Attachment is obsolete: true
Attachment #832097 - Flags: review?(echou)
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.
Attachment #832097 - Flags: review?(echou) → review?(mh+mozilla)
Blocks: 938524
No longer blocks: 938524
Blocks: 938521
Rebase patch 1 based on imminent bug 937973 comment 9 change.
Attachment #831991 - Attachment is obsolete: true
Attachment #831991 - Flags: review?(echou)
Attachment #832136 - Flags: review?(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 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.
Attachment #832136 - Flags: review?(echou) → review+
(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 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.
Attachment #832097 - Flags: review?(mh+mozilla) → review?(khuey)
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.
Attachment #832097 - Flags: review?(khuey) → review+
Revise based on reviewer's comment 29.
Attachment #832097 - Attachment is obsolete: true
Rebase onto the latest system/gonk.
Attachment #832094 - Attachment is obsolete: true
Attachment #832136 - Attachment description: Patch 1/3 (v2): Relocate files → [final] Patch 1/3: Relocate files, r=echou
Keywords: checkin-needed
Depends on: 949843
You need to log in before you can comment on or make changes to this bug.