1088574 - [Bluetooth] Implement Bluetooth backend interface with BlueZ 4

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

We added an interface to Gecko's Bluetooth module that allows to add support for the Bluetooth daemon. The interface is independent from the actual backend.

We should investigate how to implement BlueZ support behind the new interface. This would allow us to merge most of the current Profile managers and simplify the Bluetooth code base.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [WIP] proof-of-concept implementation

Completely untested, non-functional proof-of-concept implementation

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I made a quick implementation of the backend interface using BlueZ. The idea is to support BlueZ as drop-in replacement to be used with |BluetoothServiceManagerBluedroid|. The current experiment looks promising, but several things are missing from the interface. The results so far are listed below.

 * |BluetoothService::SetPasskeyInternal| is currently not supported. Bluedroid only returns 'true' in its implementation

 * Is there a way to implement |CancelBond| in BlueZ.

 * DUT and LE are not supported by BlueZ, but that's a known limitation.

 * |BluetoothService::GetAdapterPathInternal| is currently not supported.

 * BlueZ has a 'Manager' interface which represents the local host. It's used for AapterAdded signals. Bluedroid and the current API lacks this feature. We should add something similar.

 * Bluedroid's ServiceManager generates an AdapterAdded signal to be compatible with BlueZ' ServiceManager. This code should be moved into Bluedroid's backend.

 * Several signals are implemented by BlueZ, but not supported by the current interface: DeviceDisappeared, DeviceCreated, DeviceRemoved, AdapterAdded, PropertyChanged for the Manager interface. Since Bluedroid works without them, we should check which are actually required by the frontend code. New notifications will be necessary to support them.

 * We should build generic implementations for notification- and result runnables.

Comment 3

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
>  * BlueZ has a 'Manager' interface which represents the local host. It's
> used for AapterAdded signals. Bluedroid and the current API lacks this
> feature. We should add something similar.
bluedrod only supports 1 adapter, so we probably can simulate by firing AdapterAdded after AdapterStateChanged to ON.
 
>  * Bluedroid's ServiceManager generates an AdapterAdded signal to be
> compatible with BlueZ' ServiceManager. This code should be moved into
> Bluedroid's backend.
Yeah that makes sense.
>  * Several signals are implemented by BlueZ, but not supported by the
> current interface: DeviceDisappeared, DeviceCreated, DeviceRemoved,
> AdapterAdded, PropertyChanged for the Manager interface. Since Bluedroid
> works without them, we should check which are actually required by the
> frontend code. New notifications will be necessary to support them.
> 
>  * We should build generic implementations for notification- and result
> runnables.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01][WIP] proof-of-concept implementation (v2)

Changes since v1

  - rebased onto latest trunk

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02][WIP] proof-of-concept socket implementation

This patch contains an implementation of BluetoothSocketInterface with BlueZ. It simply calls the POSIX socket API on the I/O thread; just as the original implementation would do. The results have to be drop-in replacements that can be used with bluedroid/BluetoothSocket.{cpp,h}.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Here are some more results.

>  * |BluetoothService::SetPasskeyInternal| is currently not supported.
> Bluedroid only returns 'true' in its implementation

Bluedroid uses |bt_ssp_variant| for distinguishing different pairing schemes. We should split up our interface for SSP pairing into multiple notifications and commands/responses. This is how BlueZ does it and it's easier to handle.

>  * Is there a way to implement |CancelBond| in BlueZ.

Yes, it's "Adapter.CancelDeviceCreation."

>  * |BluetoothService::GetAdapterPathInternal| is currently not supported.
> 
>  * BlueZ has a 'Manager' interface which represents the local host. It's
> used for AapterAdded signals. Bluedroid and the current API lacks this
> feature. We should add something similar.
> 
>  * Bluedroid's ServiceManager generates an AdapterAdded signal to be
> compatible with BlueZ' ServiceManager. This code should be moved into
> Bluedroid's backend.

'AdapterAdded' has been removed in the new 'bluetooth2' branch. So we can either removed it completely from our BlueZ code, or handle it internally within the backend.

>  * We should build generic implementations for notification- and result
> runnables.

This code has already been landed in bug 1091577.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Add |BluetoothBlueZSocketInterface|

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] BlueZ helpers

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Added |BluetoothBlueZCoreModule|

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added |BluetoothBlueZInterface|

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Build system

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Another update to the WIP patch set.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Add |BluetoothBlueZSocketInterface|

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] BlueZ helpers

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Added |BluetoothBlueZCoreModule|

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added |BluetoothBlueZInterface|

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Build system

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] WIP: Add ConstantInitOps

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Added |BluetoothBlueZUnixSocketConnector|

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] WIP: Added |BluetoothBlueZSocketInterface|

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Use separate types for eval operators of |ConstantInitOp{1-3}|

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added |ConstantInitOp{4,5}|

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Added BlueZ helpers

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] WIP: Added Bluetooth adapter and device classes

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] WIP: Added |BluetoothBlueZCoreModule|

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] WIP: Added |BluetoothBlueZInterface|

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] WIP: Build system

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Changes since last rev

  - switch on/off BlueZ
  - scan for devices
  - pair with remote device
  - receive file from device

Sending files depends on bug 1141616.

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Add |BluetoothBlueZSocketConnector|

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] WIP: Add |BluetoothBlueZSocketInterface|

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Use separate types for eval operators of |ConstantInitOp{1-3}|

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Add |ConstantInitOp{4,5}|

Comment 33

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Add BlueZ helpers

Comment 34

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] WIP: Add Bluetooth adapter and device classes

Comment 35

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] WIP: Add |BluetoothBlueZCoreModule|

Comment 36

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] WIP: Add |BluetoothBlueZInterface|

Comment 37

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] WIP: Build system

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] WIP: Use separate types for eval operators of |ConstantInitOp{1-3}|

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] WIP: Add |ConstantInitOp{4,5}|

Comment 40

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] WIP: Add |mozilla::ipc::DaemonSocketPDUHelpers::EmptyInitOp|

Comment 41

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] WIP: Added BlueZ setup interface and module

Comment 42

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] WIP: Added BlueZ helpers

Comment 43

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] WIP: Added BlueZ core interface and module

Comment 44

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] WIP: Add |BluetoothBlueZSocketConnector|

Comment 45

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] WIP: Added BlueZ socket interface and module

Comment 46

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] WIP: Add BlueZ backend interface

Comment 47

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [10] WIP: Build system

Comment 48

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This updated patch set works with the latest Bluetooth code.

Comment 49

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1088574: Use separate types for eval operators of |ConstantInitOp{1-3}|

Comment 50

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1088574: Add |ConstantInitOp{4,5}|

Comment 51

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1088574: Add |mozilla::ipc::DaemonSocketPDUHelpers::EmptyInitOp|

Comment 52

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1088574: Add BlueZ setup interface and module

Comment 53

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1088574: Add BlueZ helpers

Comment 54

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1088574: Add BlueZ core interface and module

Comment 55

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 1088574: Add |BluetoothBlueZSocketConnector|

Comment 56

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] Bug 1088574: Add BlueZ socket interface and module

Comment 57

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] Bug 1088574: Add BlueZ backend interface

Comment 58

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [10] Bug 1088574: Replace BlueZ implementation in build system

Comment 59

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [11] Bug 1088574: Remove original BlueZ 4 support

Comment 60

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Shawn,

The attached patch set implements basic BlueZ 4 support behind the internal backend API. I'd like to replace the current BlueZ 4 code with these patches. The current BlueZ 4 code is broken and unmaintainable. The new code has a clear structure, uses backend APIs and fully re-uses Bluedroid managers. The code can serve as the base for a possible BlueZ 5 backend and both can even share functionality.

The new code supports Core and Socket modules, so OPP, PBAP and MAP are supported. There's probably not much interest in BlueZ 4, but Audio and Handsfree could be added as well if anyone's interested.

I tested the patches on the original Flame (non-kk variant).

Can you help with finding someone to review the code? Thanks!

Comment 61

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01d748f9018

Comment 62

[u535950]

Hello,

I'm interested to test this code, but I have to reswitch my Open C on bluez stack.
Can the patch be applied on B2G 2.5 ?

Comment 63

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

(In reply to micgeri from comment #62)
> Hello,
> 
> I'm interested to test this code, but I have to reswitch my Open C on bluez
> stack.
> Can the patch be applied on B2G 2.5 ?

Thanks for trying to help. Really appreciated!

We recently landed a number of internal interface changes and bug fixes that are crucial to building and running the patch set. So I don't think it will work with v2.5. Sorry. :(

Comment 64

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 1088574: Add |BluetoothBlueZSocketConnector| (v2)

Changes since v1:

 - removed debug logging

Comment 65

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] Bug 1088574: Add BlueZ backend interface (v2)

Changes since v1:

  - made d'tor of |BluetoothBlueZInterface::InitResultHandler| protected (for ref-counting)

Comment 66

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4876b060a887

Comment 67

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #60)
> Hi Shawn,
> 
> The attached patch set implements basic BlueZ 4 support behind the internal
> backend API. I'd like to replace the current BlueZ 4 code with these
> patches. The current BlueZ 4 code is broken and unmaintainable. The new code
> has a clear structure, uses backend APIs and fully re-uses Bluedroid
> managers. The code can serve as the base for a possible BlueZ 5 backend and
> both can even share functionality.
> 
> The new code supports Core and Socket modules, so OPP, PBAP and MAP are
> supported. There's probably not much interest in BlueZ 4, but Audio and
> Handsfree could be added as well if anyone's interested.
> 
> I tested the patches on the original Flame (non-kk variant).
> 
> Can you help with finding someone to review the code? Thanks!

Hi Thomas,
Thanks for taking care of this refactoring. I think I and Bruce can help on review it.

Comment 68

[Thomas Zimmermann [:tzimmermann] [:tdz]]

OK, great! I'll assign everything to you for review. Feel free to distribute.

Comment 69

[u535950]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #63)
> Hi
> 
> (In reply to micgeri from comment #62)
> > Hello,
> > 
> > I'm interested to test this code, but I have to reswitch my Open C on bluez
> > stack.
> > Can the patch be applied on B2G 2.5 ?
> 
> Thanks for trying to help. Really appreciated!
> 
> We recently landed a number of internal interface changes and bug fixes that
> are crucial to building and running the patch set. So I don't think it will
> work with v2.5. Sorry. :(

Ok, no problems.

I'm interested in Audio and Handsfree in BlueZ. I'm available to test if you want (I can switch my Open C in 2.6)

Comment 70

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1088574: Add BlueZ core interface and module (v2)

Changes since v1:

  - Replaced call to removed |nsBaseHashTable<>::Enumerate| by iterator

Comment 71

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1088574: Add BlueZ setup interface and module (v2)

Changes since v1:

  - rebased onto bug 1246931

Comment 72

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1088574: Add BlueZ helpers (v2)

Changes since v1:

  - rebased onto bug 1246931

Comment 73

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1088574: Add BlueZ core interface and module (v3)

Changes since v2:

  - rebased into bug 1246931

Comment 74

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] Bug 1088574: Add BlueZ socket interface and module (v2)

Changes since v1:

  - rebased onto bug 1246931

Comment 75

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] Bug 1088574: Add BlueZ backend interface (v3)

Changes since v2:

  - rebased onto bug 1246931

Comment 76

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

Comment on attachment 8711008 [details] [diff] [review]
[02] Bug 1088574: Add |ConstantInitOp{4,5}|

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

LGTM

Comment 77

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

Comment on attachment 8711016 [details] [diff] [review]
[10] Bug 1088574: Replace BlueZ implementation in build system

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

::: dom/bluetooth/common/BluetoothInterface.cpp
@@ +10,5 @@
>  #endif
>  #ifdef MOZ_B2G_BT_DAEMON
>  #include "BluetoothDaemonInterface.h"
>  #endif
> +#ifdef MOZ_B2G_BT_BLUEZ

Shall we distinguish Bluez4 and Bluez5? Use 'MOZ_B2G_BT_BLUEZ_LEGACY'?

@@ +1057,4 @@
>      nullptr // no default backend; must be final element in array
>    };
>  
> +  auto defaultBackend = static_cast<const char*>(nullptr);

Out of curiosity, what's the benefit to write this way?

Comment 78

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

Comment on attachment 8711009 [details] [diff] [review]
[03] Bug 1088574: Add |mozilla::ipc::DaemonSocketPDUHelpers::EmptyInitOp|

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

LGTM

Comment 79

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi!

(In reply to Shawn Huang [:shawnjohnjr] from comment #77)
> Comment on attachment 8711016 [details] [diff] [review]
> [10] Bug 1088574: Replace BlueZ implementation in build system
> 
> Review of attachment 8711016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/common/BluetoothInterface.cpp
> @@ +10,5 @@
> >  #endif
> >  #ifdef MOZ_B2G_BT_DAEMON
> >  #include "BluetoothDaemonInterface.h"
> >  #endif
> > +#ifdef MOZ_B2G_BT_BLUEZ
> 
> Shall we distinguish Bluez4 and Bluez5? Use 'MOZ_B2G_BT_BLUEZ_LEGACY'?

That's a good point. Maybe MOZ_B2G_BT_BLUEZ4. I'll change this in all patches during the next iteration of the patch set.

We don't really have a dependency on BlueZ; only on DBus. So in the future we could test for DBus in the build system and then detect the availability of BlueZ (at any version) at runtime.

> 
> @@ +1057,4 @@
> >      nullptr // no default backend; must be final element in array
> >    };
> >  
> > +  auto defaultBackend = static_cast<const char*>(nullptr);
> 
> Out of curiosity, what's the benefit to write this way?

There's really no groundbreaking benefit, but maybe a small one.

'auto' requires an rvalue for deducing the variables type. Therefore it's impossible to declare a variable without initializing it (or forgetting to initialize).

  auto value; // produces a compiler error

And a statement like

  double value = 1;

has an implicit type conversion from |int| to |double| in the assignment. The 'static_cast' makes any type conversion obvious.

  auto value = static_cast<double>(1);

Comment 80

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

Comment on attachment 8717431 [details] [diff] [review]
[04] Bug 1088574: Add BlueZ setup interface and module (v2)

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

::: dom/bluetooth/bluez4/BluetoothBlueZSetupInterface.cpp
@@ +7,5 @@
> +#include "BluetoothBlueZSetupInterface.h"
> +#include "mozilla/ipc/DaemonSocketPDUHelpers.h"
> +
> +using namespace mozilla::ipc;
> +using namespace mozilla::ipc::DaemonSocketPDUHelpers;

Does this patch use DaemonSocketPDUHelpers?

Comment 81

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #80)
> Comment on attachment 8717431 [details] [diff] [review]
> [04] Bug 1088574: Add BlueZ setup interface and module (v2)
> 
> Review of attachment 8717431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluez4/BluetoothBlueZSetupInterface.cpp
> @@ +7,5 @@
> > +#include "BluetoothBlueZSetupInterface.h"
> > +#include "mozilla/ipc/DaemonSocketPDUHelpers.h"
> > +
> > +using namespace mozilla::ipc;
> > +using namespace mozilla::ipc::DaemonSocketPDUHelpers;
> 
> Does this patch use DaemonSocketPDUHelpers?

Apparently not. Will be fixed.

Comment 82

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

Comment on attachment 8717432 [details] [diff] [review]
[05] Bug 1088574: Add BlueZ helpers (v2)

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

::: dom/bluetooth/bluez4/BluetoothBlueZHelpers.cpp
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "BluetoothBlueZHelpers.h"
> +#include "BluetoothUtils.h"
> +#include "mozilla/ClearOnShutdown.h"

Do we need 'ClearOnShutdown.h'?

@@ +6,5 @@
> +
> +#include "BluetoothBlueZHelpers.h"
> +#include "BluetoothUtils.h"
> +#include "mozilla/ClearOnShutdown.h"
> +#include "mozilla/LazyIdleThread.h"

I don't see we need 'LazyIdleThread' here. Please remove it.

@@ +7,5 @@
> +#include "BluetoothBlueZHelpers.h"
> +#include "BluetoothUtils.h"
> +#include "mozilla/ClearOnShutdown.h"
> +#include "mozilla/LazyIdleThread.h"
> +#include "nsXULAppAPI.h"

I don't see we need nsXULAppAPI.h here.

@@ +53,5 @@
> +Convert(const bool aIn, BluetoothScanMode& aOut)
> +{
> +  static const BluetoothScanMode sScanMode[] = {
> +    [FALSE] = SCAN_MODE_NONE,
> +    [TRUE] = SCAN_MODE_CONNECTABLE_DISCOVERABLE

nit: [false] = SCAN_MODE_NONE,
     [true] = SCAN_MODE_CONNECTABLE

@@ +100,5 @@
> +Convert(const DBusTypeBoolean& aIn, BluetoothScanMode& aOut)
> +{
> +  static const uint8_t sScanMode[] = {
> +    [FALSE] = SCAN_MODE_NONE,
> +    [TRUE] = SCAN_MODE_CONNECTABLE

nit: [false] = SCAN_MODE_NONE,
     [true] = SCAN_MODE_CONNECTABLE

@@ +244,5 @@
> +  static const detail::StringConversion<BluetoothSspVariant> sConversion[] = {
> +    {"", SSP_VARIANT_PASSKEY_CONFIRMATION},
> +    {"", SSP_VARIANT_PASSKEY_ENTRY},
> +    {"", SSP_VARIANT_CONSENT},
> +    {"", SSP_VARIANT_PASSKEY_NOTIFICATION},

I just wonder why this is just empty string.

@@ +346,5 @@
> +    {"Class", PROPERTY_CLASS_OF_DEVICE},
> +    {"Devices", PROPERTY_UNKNOWN},
> +    {"Discoverable", PROPERTY_ADAPTER_SCAN_MODE},
> +    {"DiscoverableTimeout", PROPERTY_ADAPTER_DISCOVERY_TIMEOUT},
> +    {"Discovering", PROPERTY_UNKNOWN},

I'm a bit worry about 'Discovering' property will be missed after conversion. bluedroid uses DiscoverStateChangeNotification to indicate Discover state, but BlueZ only sends 'Discovering' property.

::: dom/bluetooth/bluez4/BluetoothBlueZHelpers.h
@@ +8,5 @@
> +#define mozilla_dom_bluetooth_BluetoothBlueZHelpers_h
> +
> +#include <dbus/dbus.h>
> +#include "BluetoothCommon.h"
> +#include "mozilla/ipc/DBusMessageRefPtr.h"

I don't see we use DBusMessageRefPtr/DBusReplyHandler in this patch, do you intend to use it in later patch?

@@ +193,5 @@
> +// |UnpackMessage|, but increment the iterator after successfully
> +// unpacking the field.
> +//
> +// After unpacking a DBus message, the user should invoke a call to
> +//|WarnAboutTrailingArguments|. It will output a warning into the

nit: indentation. Add space before |WarnAboutTrailingArguments|.

@@ +195,5 @@
> +//
> +// After unpacking a DBus message, the user should invoke a call to
> +//|WarnAboutTrailingArguments|. It will output a warning into the
> +// log if there are unhandled fields in the message. This is often
> +// a sign of a bug in the message parser.

Why do we want user invoke a call to |WarnAboutTrailingArgument" explicitly? Why don't we just check trailing arguments in UnpackMessage?

Comment 83

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8717431 [details] [diff] [review]
[04] Bug 1088574: Add BlueZ setup interface and module (v2)

Comment on attachment 8717431 [details] [diff] [review] [diff] [review]
[04] Bug 1088574: Add BlueZ setup interface and module (v2)

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

::: dom/bluetooth/bluez4/BluetoothBlueZSetupInterface.cpp
@@ +7,5 @@
> +#include "BluetoothBlueZSetupInterface.h"
> +#include "mozilla/ipc/DaemonSocketPDUHelpers.h"
> +
> +using namespace mozilla::ipc;
> +using namespace mozilla::ipc::DaemonSocketPDUHelpers;

Does this patch use DaemonSocketPDUHelpers?

I checked this again. The |*InitOp| classes are in this file and namespace. I'm reopening the patch for review.

Comment 84

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

(In reply to Shawn Huang [:shawnjohnjr] from comment #82)
> Comment on attachment 8717432 [details] [diff] [review]
> [05] Bug 1088574: Add BlueZ helpers (v2)
> 
> Review of attachment 8717432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluez4/BluetoothBlueZHelpers.cpp
> @@ +5,5 @@
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#include "BluetoothBlueZHelpers.h"
> > +#include "BluetoothUtils.h"
> > +#include "mozilla/ClearOnShutdown.h"
> 
> Do we need 'ClearOnShutdown.h'?
> I don't see we need 'LazyIdleThread' here. Please remove it.
> I don't see we need nsXULAppAPI.h here.

I will go through them and remove them if possible. I think 'nsXULAppAPI.h' is required for some of the string handling.

> @@ +53,5 @@
> > +Convert(const bool aIn, BluetoothScanMode& aOut)
> > +{
> > +  static const BluetoothScanMode sScanMode[] = {
> > +    [FALSE] = SCAN_MODE_NONE,
> > +    [TRUE] = SCAN_MODE_CONNECTABLE_DISCOVERABLE
> 
> nit: [false] = SCAN_MODE_NONE,
>      [true] = SCAN_MODE_CONNECTABLE

I think you're right, but I have to test this first.

It's a bit hacky, as BlueZ has separate states for 'discoverable' and 'connectable', while our API (like Bluedroid) only has a single state; and we fix this up later in |UnpackProperty(DBusMessageIter*, BluetoothProperty&)|. That's probably something we should clean up later on.

> @@ +244,5 @@
> > +  static const detail::StringConversion<BluetoothSspVariant> sConversion[] = {
> > +    {"", SSP_VARIANT_PASSKEY_CONFIRMATION},
> > +    {"", SSP_VARIANT_PASSKEY_ENTRY},
> > +    {"", SSP_VARIANT_CONSENT},
> > +    {"", SSP_VARIANT_PASSKEY_NOTIFICATION},
> 
> I just wonder why this is just empty string.

Me too. :) Probably an oversight. But I never saw any problems from it. Will be fixed.

> @@ +346,5 @@
> > +    {"Class", PROPERTY_CLASS_OF_DEVICE},
> > +    {"Devices", PROPERTY_UNKNOWN},
> > +    {"Discoverable", PROPERTY_ADAPTER_SCAN_MODE},
> > +    {"DiscoverableTimeout", PROPERTY_ADAPTER_DISCOVERY_TIMEOUT},
> > +    {"Discovering", PROPERTY_UNKNOWN},
> 
> I'm a bit worry about 'Discovering' property will be missed after
> conversion. bluedroid uses DiscoverStateChangeNotification to indicate
> Discover state, but BlueZ only sends 'Discovering' property.

I see. I'll add this if possible.

> ::: dom/bluetooth/bluez4/BluetoothBlueZHelpers.h
> @@ +8,5 @@
> > +#define mozilla_dom_bluetooth_BluetoothBlueZHelpers_h
> > +
> > +#include <dbus/dbus.h>
> > +#include "BluetoothCommon.h"
> > +#include "mozilla/ipc/DBusMessageRefPtr.h"
> 
> I don't see we use DBusMessageRefPtr/DBusReplyHandler in this patch, do you
> intend to use it in later patch?

I remove it, if possible.

> @@ +193,5 @@
> > +// |UnpackMessage|, but increment the iterator after successfully
> > +// unpacking the field.
> > +//
> > +// After unpacking a DBus message, the user should invoke a call to
> > +//|WarnAboutTrailingArguments|. It will output a warning into the
> 
> nit: indentation. Add space before |WarnAboutTrailingArguments|.

Ok

> @@ +195,5 @@
> > +//
> > +// After unpacking a DBus message, the user should invoke a call to
> > +//|WarnAboutTrailingArguments|. It will output a warning into the
> > +// log if there are unhandled fields in the message. This is often
> > +// a sign of a bug in the message parser.
> 
> Why do we want user invoke a call to |WarnAboutTrailingArgument" explicitly?
> Why don't we just check trailing arguments in UnpackMessage?

That comment is probably a bit misleading. |UnpackMessage| unpacks the single field to which the iterator is pointing to. |WarnAboutTrainlingArguments| warns if there's a valid field after the iterator. So if there's a message with 5 fields, it would warn for fields 1 to 4 (which have valid fields succeeding them).

Comment 85

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> > ::: dom/bluetooth/bluez4/BluetoothBlueZHelpers.h
> > @@ +8,5 @@
> > > +#define mozilla_dom_bluetooth_BluetoothBlueZHelpers_h
> > > +
> > > +#include <dbus/dbus.h>
> > > +#include "BluetoothCommon.h"
> > > +#include "mozilla/ipc/DBusMessageRefPtr.h"
> > 
> > I don't see we use DBusMessageRefPtr/DBusReplyHandler in this patch, do you
> > intend to use it in later patch?
> 
> I remove it, if possible.

At the bottom of the file is |UnpackDBusMessageInitOp|. It contains a |RefPtr<DBusMessage>|. The code in 'DBusMessageRefPtr.h' specializes RefPtr to work with the reference counting of DBus messages. So the include statement is required.

Comment 86

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> > +    {"Discovering", PROPERTY_UNKNOWN},
> 
> I'm a bit worry about 'Discovering' property will be missed after
> conversion. bluedroid uses DiscoverStateChangeNotification to indicate
> Discover state, but BlueZ only sends 'Discovering' property.

"Discovering" is not handled here, but in patch [06]. |BluetoothBlueZCoreModule::HandleAdapterPropertyChanged| has a branch for handling "Discovering", where it sends out the |DiscoverStateChangedNotification| instance.

Comment 87

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> > @@ +244,5 @@
> > > +  static const detail::StringConversion<BluetoothSspVariant> sConversion[] = {
> > > +    {"", SSP_VARIANT_PASSKEY_CONFIRMATION},
> > > +    {"", SSP_VARIANT_PASSKEY_ENTRY},
> > > +    {"", SSP_VARIANT_CONSENT},
> > > +    {"", SSP_VARIANT_PASSKEY_NOTIFICATION},
> > 
> > I just wonder why this is just empty string.
> 
> Me too. :) Probably an oversight. But I never saw any problems from it. Will
> be fixed.

This function is actually unused an will be removed.

Comment 88

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 1088574: Add BlueZ helpers (v3)

Changes since v2:

  - removed unnecessary include statements
  - use existing HAL conversion where possible
  - moved string helpers to HAL
  - moved DBus helpers to ipc/dbus
  - cleanup scan-mode conversion
  - removed unnecessary string-to-SSP-variant conversion
  - cosmetical fixes

Please also see the various replies I left in the bug report.

Comment 89

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [10] Bug 1088574: Replace BlueZ implementation in build system (v2)

Changes since v1:

  - renamed MOZ_B2G_BT_BLUEZ to MOZ_B2G_BT_BLUEZ4
  - make DBus helpers depend on MOZ_ENABLE_DBUS build flag

Comment 90

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

Comment on attachment 8717433 [details] [diff] [review]
[06] Bug 1088574: Add BlueZ core interface and module (v3)

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

Hi
I just go through the code once, I will re-visit it again, when the first around finished.

::: dom/bluetooth/bluez4/BluetoothBlueZCoreInterface.cpp
@@ +15,5 @@
> +#include "mozilla/ipc/DBusConnectionRefPtr.h"
> +#include "mozilla/ipc/DBusHelpers.h"
> +#include "mozilla/ipc/DBusMessageRefPtr.h"
> +#include "mozilla/ipc/DBusUtils.h"
> +#include "mozilla/Monitor.h"

Since you already removed sGetPropertyMonitor, I think you don't need Monitor.h.

@@ +45,5 @@
> +  struct ScopedDlHandleTraits final
> +  {
> +    typedef void* type;
> +
> +    static void* empty()

Not sure why this function is required. But it looks like it exists before.

@@ +146,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    auto bt_is_enabled =
> +      reinterpret_cast<int (*)()>(dlsym(handle, "bt_is_enabled"));

nit:Remove extra space after 'int'

@@ +217,5 @@
> +#define BLUEZ_INTERFACE_AGENT     DBUS_SERVICE_BLUEZ ".Agent"
> +#define BLUEZ_INTERFACE_DEVICE    DBUS_SERVICE_BLUEZ ".Device"
> +#define BLUEZ_INTERFACE_MANAGER   DBUS_SERVICE_BLUEZ ".Manager"
> +
> +#define B2G_AGENT_CAPABILITIES "DisplayYesNo"

Will it be nice to move these "#define" to the beginning of the file? It will be easier for 4000 lines of code.

@@ +472,5 @@
> +  for (size_t i = 0; i < ArrayLength(sDBusSignals); ++i) {
> +
> +    DBusError err;
> +    dbus_error_init(&err);
> +

In the original code, it calls |mConnection->Watch()|, now it's not necessary?

@@ +577,5 @@
> +{
> +  auto rv = DBusSendMessageWithReply(GetConnection(),
> +                                     DBusReplyHandler::Callback,
> +                                     aDBusReplyHandler,
> +                                     50000,

Why 50000?

@@ +753,5 @@
> +                                     -1,
> +                                     DBUS_SERVICE_BLUEZ,
> +                                     aDevicePath,
> +                                     BLUEZ_INTERFACE_DEVICE,
> +                                     "GetServiceAttributeValue",

Which document do you reference? I can't find GetServiceAttributeValue.
https://android.googlesource.com/platform/external/bluetooth/bluez/+/master/doc/device-api.txt

@@ +1057,5 @@
> +                      nsAutoArrayPtr<BluetoothProperty>>(
> +        STATUS_SUCCESS, deviceAddress, 1, properties.forget()));
> +
> +  } else if (name.Equals("Connected")) {
> +

I'm a bit lost here, why Device PropertyChanged 'Connected' is related to BluetoothBondState. Connected is related to 'ACL link' connected instead of bond-state update
It looks like misplaced.

@@ +1062,5 @@
> +    dbus_message_iter_next(&iter);
> +
> +    BluetoothBondState bondState;
> +
> +    /* Bug 857896: Device property "Connected" could be a boolean value

Maybe change to single line comment for style consistency.

@@ +1068,5 @@
> +     * for the array and manually convert the first byte into a boolean.
> +     */
> +
> +    if (dbus_message_iter_get_arg_type(&iter) == DBUS_TYPE_BOOLEAN) {
> +

nit: Please remove blank line

@@ +1077,5 @@
> +      if (NS_FAILED(rv)) {
> +        return DBUS_HANDLER_RESULT_HANDLED;
> +      }
> +    } else {
> +

nit: Please remove blank line

@@ +1104,5 @@
> +    bondState = bondState == BOND_STATE_NONE ? BOND_STATE_NONE :
> +                                               BOND_STATE_BONDING;
> +
> +  } else if (name.Equals("Paired")) {
> +

nit: Remove extra space

@@ +1129,5 @@
> +     */
> +
> +    dbus_message_iter_next(&iter);
> +    dbus_message_iter_next(&iter);
> +

nit: Please remove blank line

@@ +1144,5 @@
> +     */
> +
> +    dbus_message_iter_next(&iter);
> +    dbus_message_iter_next(&iter);
> +

nit: Please remove blank line

@@ +1522,5 @@
> +    dbus_message_iter_recurse(&iter, &arrayIter);
> +
> +    while ((!hasClass || !hasName) &&
> +           dbus_message_iter_get_arg_type(&arrayIter) != DBUS_TYPE_INVALID) {
> +

nit:remove this line

@@ +1968,5 @@
> +        auto errorName = dbus_message_get_error_name(aReply);
> +
> +        if (!errorName) {
> +          BT_LOGR("DBus error message");
> +

nit: Remove this line.

@@ +2915,5 @@
> +    // On Desktop BlueZ, we'd have to call Device::DiscoverServices and
> +    // parse the returned XML block for the service record.
> +
> +    // First, create a service record.
> +

nit:Remove blank line

@@ +2922,5 @@
> +
> +    // Bug 793977: Just set something for Desktop, until we have a
> +    // parser for the XML block.
> +    properties[0].mServiceRecord.mUuid = mUuid;
> +    properties[0].mServiceRecord.mChannel = static_cast<uint32_t>(-1);

Just want to clarify that mChannel should be '-1'? If it's true, please add comment.

@@ +2928,5 @@
> +           0,
> +           ArrayLength(properties[0].mServiceRecord.mName));
> +
> +    // Second, the command is reported as successful.
> +

nit:Remove blank line

@@ +2934,5 @@
> +      mRes, &BluetoothCoreResultHandler::GetRemoteServiceRecord,
> +      EmptyInitOp());
> +
> +    // The actual service record is handled by a notification handler.
> +

nit:Remove blank line

@@ +3434,5 @@
> +
> +  return NS_OK;
> +}
> +
> +class BluetoothBlueZCoreModule::CancelDeviceCreationTask final

Nice catch! I found CalcelDeviceCreation even disappeared in the original BluetoothDBusService.

::: dom/bluetooth/bluez4/BluetoothBlueZCoreInterface.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_bluetooth_BluetoothBlueZCoreInterface_h
> +#define mozilla_dom_bluetooth_BluetoothBlueZCoreInterface_h
> +
> +#include <dbus/dbus.h>

Can we include dbus.h in BluetoothBlueZCoreInterface.cpp?

Comment 91

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

Comment on attachment 8721976 [details] [diff] [review]
[10] Bug 1088574: Replace BlueZ implementation in build system (v2)

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

::: dom/bluetooth/moz.build
@@ +46,5 @@
>          'ipc'
>      ]
>  
> +    # TODO: These files are not backend-specific any longer. Merge
> +    #       with the ones in common/.

I'm not sure how much change we're going to deal with. There are differences in Avrcp and Gatt for BlueZ4/5 Dbus interface.

Comment 92

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

Comment on attachment 8717433 [details] [diff] [review]
[06] Bug 1088574: Add BlueZ core interface and module (v3)

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

::: dom/bluetooth/bluez4/BluetoothBlueZCoreInterface.cpp
@@ +1057,5 @@
> +                      nsAutoArrayPtr<BluetoothProperty>>(
> +        STATUS_SUCCESS, deviceAddress, 1, properties.forget()));
> +
> +  } else if (name.Equals("Connected")) {
> +

nit: Remove the blank line.

@@ +1758,5 @@
> +    bool hasName = false;
> +
> +    /* Walk over the array elements and find the properties 'Class' and
> +     * 'Name'. If both are available, we call |OnPinRequest|.
> +     */

nit: Remove blank line.

@@ +3013,5 @@
> +      DBusMessageIter arrayIter;
> +      dbus_message_iter_recurse(&iter, &arrayIter);
> +
> +      while (dbus_message_iter_get_arg_type(&arrayIter) != DBUS_TYPE_INVALID) {
> +

nit:Remove blank line

::: dom/bluetooth/bluez4/BluetoothBlueZCoreInterface.h
@@ +351,5 @@
> +
> +  void SetNotificationHandler(
> +    BluetoothCoreNotificationHandler* aNotificationHandler) override;
> +
> +  /* Enable/Disable */

Can you instead use single line comment in the same file?

Comment 93

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

Comment on attachment 8721976 [details] [diff] [review]
[10] Bug 1088574: Replace BlueZ implementation in build system (v2)

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

We're having HID support for firefox OS 2.6 for Nexus Player soon, i think moz.build might require to rebase again.

Comment 94

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Shawn

Sorry for the delay. I was on PTO and later had to get flame (JB) to build with gcc 4.9.

I'll fix the nits and cleanups in this patch. For the rest, please read below.

> ::: dom/bluetooth/bluez4/BluetoothBlueZCoreInterface.cpp
> @@ +15,5 @@
> > +#include "mozilla/ipc/DBusConnectionRefPtr.h"
> > +#include "mozilla/ipc/DBusHelpers.h"
> > +#include "mozilla/ipc/DBusMessageRefPtr.h"
> > +#include "mozilla/ipc/DBusUtils.h"
> > +#include "mozilla/Monitor.h"
> 
> Since you already removed sGetPropertyMonitor, I think you don't need
> Monitor.h.

Indeed. I'll remove the statement.

> @@ +45,5 @@
> > +  struct ScopedDlHandleTraits final
> > +  {
> > +    typedef void* type;
> > +
> > +    static void* empty()
> 
> Not sure why this function is required. But it looks like it exists before.

Apparently, it's an internal function of |Scoped<>|.

> @@ +472,5 @@
> > +  for (size_t i = 0; i < ArrayLength(sDBusSignals); ++i) {
> > +
> > +    DBusError err;
> > +    dbus_error_init(&err);
> > +
> 
> In the original code, it calls |mConnection->Watch()|, now it's not
> necessary?

This functionality has been moved to |BluethoothBlueZDBusIO::Init| (patch [09]) The new code separates signals of DBus and different Bluetooth modules. The code in this |Enable| method only handles the Bluetooth signals that are relevant for the Core module.

> @@ +577,5 @@
> > +{
> > +  auto rv = DBusSendMessageWithReply(GetConnection(),
> > +                                     DBusReplyHandler::Callback,
> > +                                     aDBusReplyHandler,
> > +                                     50000,
> 
> Why 50000?

That's the value from the old code IIRC.

> @@ +753,5 @@
> > +                                     -1,
> > +                                     DBUS_SERVICE_BLUEZ,
> > +                                     aDevicePath,
> > +                                     BLUEZ_INTERFACE_DEVICE,
> > +                                     "GetServiceAttributeValue",
> 
> Which document do you reference? I can't find GetServiceAttributeValue.
> https://android.googlesource.com/platform/external/bluetooth/bluez/+/master/
> doc/device-api.txt

This is one of the Android changes that's not documented. It's only in the code.

> @@ +1057,5 @@
> > +                      nsAutoArrayPtr<BluetoothProperty>>(
> > +        STATUS_SUCCESS, deviceAddress, 1, properties.forget()));
> > +
> > +  } else if (name.Equals("Connected")) {
> > +
> 
> I'm a bit lost here, why Device PropertyChanged 'Connected' is related to
> BluetoothBondState. Connected is related to 'ACL link' connected instead of
> bond-state update
> It looks like misplaced.

I thought that this is what the old code did, but I'll check again.

> @@ +2922,5 @@
> > +
> > +    // Bug 793977: Just set something for Desktop, until we have a
> > +    // parser for the XML block.
> > +    properties[0].mServiceRecord.mUuid = mUuid;
> > +    properties[0].mServiceRecord.mChannel = static_cast<uint32_t>(-1);
> 
> Just want to clarify that mChannel should be '-1'? If it's true, please add
> comment.

OK, I'll comment on this.

> ::: dom/bluetooth/bluez4/BluetoothBlueZCoreInterface.h
> @@ +6,5 @@
> > +
> > +#ifndef mozilla_dom_bluetooth_BluetoothBlueZCoreInterface_h
> > +#define mozilla_dom_bluetooth_BluetoothBlueZCoreInterface_h
> > +
> > +#include <dbus/dbus.h>
> 
> Can we include dbus.h in BluetoothBlueZCoreInterface.cpp?

|DBusConnection| is a typedef of |struct DBusConnection|. It can't be forward declared easily. And I had some trouble when trying this.

Comment 95

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #93)
> Comment on attachment 8721976 [details] [diff] [review]
> [10] Bug 1088574: Replace BlueZ implementation in build system (v2)
> 
> Review of attachment 8721976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're having HID support for firefox OS 2.6 for Nexus Player soon, i think
> moz.build might require to rebase again.

Yes, I had to update the file locally.

Comment 96

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1088574: Add BlueZ setup interface and module (v3)

Changes since v2:

  - rebased onto latest m-c

Comment 97

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1088574: Add BlueZ core interface and module (v4)

Changes since v3:

  - don't include "mozilla/Monitor.h"
  - cosmetic fixes

Currently I cannot test the mentioned issue with 'Connected', because b2g is constantly broken. I'll test ASAP.

Comment 98

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

Comment on attachment 8711009 [details] [diff] [review]
[03] Bug 1088574: Add |mozilla::ipc::DaemonSocketPDUHelpers::EmptyInitOp|

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

I just don't understand why we need a class for returning NS_OK.

Comment 99

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #85)
> > > ::: dom/bluetooth/bluez4/BluetoothBlueZHelpers.h
> > > @@ +8,5 @@
> > > > +#define mozilla_dom_bluetooth_BluetoothBlueZHelpers_h
> > > > +
> > > > +#include <dbus/dbus.h>
> > > > +#include "BluetoothCommon.h"
> > > > +#include "mozilla/ipc/DBusMessageRefPtr.h"
> > > 
> > > I don't see we use DBusMessageRefPtr/DBusReplyHandler in this patch, do you
> > > intend to use it in later patch?
> > 
> > I remove it, if possible.
> 
> At the bottom of the file is |UnpackDBusMessageInitOp|. It contains a
> |RefPtr<DBusMessage>|. The code in 'DBusMessageRefPtr.h' specializes RefPtr
> to work with the reference counting of DBus messages. So the include
> statement is required.

Ok, I see.

Comment 100

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

(In reply to Shawn Huang [:shawnjohnjr] from comment #98)
> Comment on attachment 8711009 [details] [diff] [review]
> [03] Bug 1088574: Add |mozilla::ipc::DaemonSocketPDUHelpers::EmptyInitOp|
> 
> Review of attachment 8711009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just don't understand why we need a class for returning NS_OK.

We use the daemon runnables to transfer data to the main thread and call notifications and results in the backend API. The runnables require an init-op class for initializing their parameters. Parameter values can come anywhere, for example from the fields of a DBus message. |EmptyInitOp| is for cases where we don't have to decode a DBus message; just send the runnable.

Solving this without |EmptyInitOp| would require a different runnable class, which would duplicate the functionality of |Daemon*Runnable0| [2] at least.

All this is inlined, so their should be no overhead from a call to |EmptyInitOp::operator()| in the compiled code.

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/hal/DaemonRunnables.h#105
[2] https://dxr.mozilla.org/mozilla-central/source/ipc/hal/DaemonRunnables.h#57

Comment 101

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1088574: Use separate types for eval operators of |ConstantInitOp{1-3}| (v2)

Changes since v1:

  - update for |UniquePtr| support

Comment 102

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1088574: Add |ConstantInitOp{4,5}| (v2)

Changes since v1:

  - updated for |UniquePtr|

Comment 103

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 1088574: Add BlueZ core interface and module (v5)

Changes since v4:

  - updated for |UniquePtr| support

Comment 104

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [08] Bug 1088574: Add BlueZ socket interface and module (v3)

Changes since v2:

  - updated for |UniquePtr| support

Comment 105

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [09] Bug 1088574: Add BlueZ backend interface (v4)

Changes since v3:

  - updated for |UniquePtr| support