Closed Bug 1095487 Opened 10 years ago Closed 10 years ago

[Bluetooth] Implement A2DP support in daemon backend

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.2+)

Status:
RESOLVED FIXED
Milestone:
2.2 S2 (19dec)
Project Flags:
feature-b2g 2.2+

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(4 files, 6 obsolete files)

[01] Bug 1095487: Add Bluetooth A2DP helpers
3.78 KB, patch
shawnjohnjr
: review+
ben.tian
: feedback+
Details | Diff | Splinter Review
[04] Bug 1095487: Add Blueooth A2DP support when using daemon backend
5.70 KB, patch
shawnjohnjr
: review+
ben.tian
: feedback+
Details | Diff | Splinter Review
[02] Bug 1095487: Added Bluetooth A2DP module for daemon backend (v2)
13.21 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1095487: Added Blueooth A2DP interface for daemon backend (v2)
5.86 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
feature-b2g: --- → 2.2+
Attachment #8522903 - Attachment is obsolete: true
Attachment #8527533 - Flags: review?(shuang)
Attachment #8527533 - Flags: feedback?(btian)
Attachment #8522904 - Attachment is obsolete: true
Attachment #8527534 - Flags: review?(shuang)
Attachment #8527534 - Flags: feedback?(btian)
Attachment #8522905 - Attachment is obsolete: true
Attachment #8527535 - Flags: review?(shuang)
Attachment #8527535 - Flags: feedback?(btian)
Attachment #8522906 - Attachment is obsolete: true
Attachment #8527536 - Flags: review?(shuang)
Attachment #8527536 - Flags: feedback?(btian)
Comment on attachment 8527533 [details] [diff] [review] [01] Bug 1095487: Add Bluetooth A2DP helpers Review of attachment 8527533 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8527533 - Flags: feedback?(btian) → feedback+
Comment on attachment 8527533 [details] [diff] [review] [01] Bug 1095487: Add Bluetooth A2DP helpers Review of attachment 8527533 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8527533 - Flags: review?(shuang) → review+
Comment on attachment 8527534 [details] [diff] [review] [02] Bug 1095487: Added Bluetooth A2DP module for daemon backend Review of attachment 8527534 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8527534 - Flags: feedback?(btian) → feedback+
Comment on attachment 8527536 [details] [diff] [review] [04] Bug 1095487: Add Blueooth A2DP support when using daemon backend Review of attachment 8527536 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8527536 - Flags: feedback?(btian) → feedback+
Comment on attachment 8527534 [details] [diff] [review] [02] Bug 1095487: Added Bluetooth A2DP module for daemon backend Review of attachment 8527534 [details] [diff] [review]: ----------------------------------------------------------------- Overall it looks good to me. ::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.h @@ +58,5 @@ > + // > + // Responses > + // > + > + typedef BluetoothResultRunnable0<BluetoothA2dpResultHandler, void> nit:The naming convention is a bit different from BluetoothHALInterface. In BluetoothHandsfreeHALInterface, typedef BluetoothHALInterfaceRunnable0<BluetoothHandsfreeResultHandler, void> BluetoothHandsfreeHALResultRunnable; Can we make the naming be consistent? @@ +63,5 @@ > + ResultRunnable; > + > + typedef BluetoothResultRunnable1<BluetoothA2dpResultHandler, void, > + BluetoothStatus, BluetoothStatus> > + ErrorRunnable; Ditto. @@ +117,5 @@ > + > + static BluetoothA2dpNotificationHandler* sNotificationHandler; > +}; > + > + nit:Remove one empty line.
Attachment #8527534 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #13) > Comment on attachment 8527534 [details] [diff] [review] > [02] Bug 1095487: Added Bluetooth A2DP module for daemon backend > > Review of attachment 8527534 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall it looks good to me. > > ::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.h > @@ +58,5 @@ > > + // > > + // Responses > > + // > > + > > + typedef BluetoothResultRunnable0<BluetoothA2dpResultHandler, void> > > nit:The naming convention is a bit different from BluetoothHALInterface. In > BluetoothHandsfreeHALInterface, I'd like to keep the naming convention for the daemon backend and I currently also use it for the BlueZ support I investigate in bug 1088574. The HAL backend is sort of legacy already. Once we remove it, the naming will be consistent. Is that OK for you?
Comment on attachment 8527535 [details] [diff] [review] [03] Bug 1095487: Added Blueooth A2DP interface for daemon backend Review of attachment 8527535 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8527535 - Flags: review?(shuang) → review+
Comment on attachment 8527536 [details] [diff] [review] [04] Bug 1095487: Add Blueooth A2DP support when using daemon backend Review of attachment 8527536 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp @@ +1361,5 @@ > , public BluetoothDaemonSetupModule > , public BluetoothDaemonCoreModule > , public BluetoothDaemonSocketModule > , public BluetoothDaemonHandsfreeModule > + , public BluetoothDaemonA2dpModule If we have more profiles which need to support, it looks like a bit strange for inheritance. There will be too many base classes. I afraid when we have too many profiles need to be supported and it seems to be complicated multiple inheritance. What do you think?
Attachment #8527536 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #16) > Comment on attachment 8527536 [details] [diff] [review] > [04] Bug 1095487: Add Blueooth A2DP support when using daemon backend > > Review of attachment 8527536 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp > @@ +1361,5 @@ > > , public BluetoothDaemonSetupModule > > , public BluetoothDaemonCoreModule > > , public BluetoothDaemonSocketModule > > , public BluetoothDaemonHandsfreeModule > > + , public BluetoothDaemonA2dpModule > > If we have more profiles which need to support, it looks like a bit strange > for inheritance. There will be too many base classes. I afraid when we have > too many profiles need to be supported and it seems to be complicated > multiple inheritance. What do you think? It's a standard C++ feature. Why do you think this will not work with more base classes?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17) > (In reply to Shawn Huang [:shawnjohnjr] from comment #16) > > Comment on attachment 8527536 [details] [diff] [review] > > [04] Bug 1095487: Add Blueooth A2DP support when using daemon backend > > > > Review of attachment 8527536 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp > > @@ +1361,5 @@ > > > , public BluetoothDaemonSetupModule > > > , public BluetoothDaemonCoreModule > > > , public BluetoothDaemonSocketModule > > > , public BluetoothDaemonHandsfreeModule > > > + , public BluetoothDaemonA2dpModule > > > > If we have more profiles which need to support, it looks like a bit strange > > for inheritance. There will be too many base classes. I afraid when we have > > too many profiles need to be supported and it seems to be complicated > > multiple inheritance. What do you think? > > It's a standard C++ feature. Why do you think this will not work with more > base classes? It actually can work, but I'm thinking if we can use composition instead of inheritance.
Comment on attachment 8527535 [details] [diff] [review] [03] Bug 1095487: Added Blueooth A2DP interface for daemon backend Review of attachment 8527535 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8527535 - Flags: feedback?(btian) → feedback+
> > It's a standard C++ feature. Why do you think this will not work with more > > base classes? > > It actually can work, but I'm thinking if we can use composition instead of > inheritance. I think this would be more complicated in the end. The base classes have 3 purely virtual methods that are implemented by |BluetoothDaemonProtocol|: - RegisterModule, - UnregisterModule, and - Send This is how modules (de-)announce themselves to the daemon and send PDUs. If we don't use inheritance, we'd have to implement this ourselves (e.g., by using method pointers).
Blocks: 1110049
Changes since v1: - removed an empty line
Attachment #8527534 - Attachment is obsolete: true
Attachment #8534887 - Flags: review+
Changes since v1: - rebased onto [02](v2)
Attachment #8527535 - Attachment is obsolete: true
Attachment #8534889 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: