1048915 - [Bluetooth] Convert Core callbacks to backend-neutral interface

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

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

Attachment: [01] Bug 1048915: Add infrastructure for Bluetooth notifications

Comment 2

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

Attachment: [02] Bug 1048915: Add Bluetooth Core notifications

Comment 3

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

Attachment: [03] Bug 1048915: Implement Bluetooth Core notifications

Comment 4

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

Attachment: [04] Bug 1048915: Use Bluetooth Core notifications

Comment 5

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

Attachment: [05] Bug 1048915: Integrate helper runnables into notification methods

Comment 6

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

Attachment: [06] Bug 1048915: Cleanup |BluetoothServiceBluedroid|

Comment 7

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

Comment on attachment 8468411 [details] [diff] [review]
[02] Bug 1048915: Add Bluetooth Core notifications

This patch contains the same bug as fixed by bug 1050299. I'll clean this up in the next iteration.

Comment 8

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

Comment on attachment 8468411 [details] [diff] [review]
[02] Bug 1048915: Add Bluetooth Core notifications

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +88,5 @@
> +static nsresult
> +Convert(bt_scan_mode_t aIn, BluetoothScanMode& aOut)
> +{
> +  static const BluetoothScanMode sScanMode[] = {
> +    [BT_SCAN_MODE_NONE] = SCAN_MODE_NONE,

Because of Bug 1048915, this probably requires to add extra CONVERT marco.

Comment 9

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

Attachment: [02] Bug 1048915: Add Bluetooth Core notifications (v2)

Changes since v1:

  - use CONVERT macro in lookup tables

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8469927 [details] [diff] [review]
[02] Bug 1048915: Add Bluetooth Core notifications (v2)

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

Please see my comment below.

::: dom/bluetooth/BluetoothCommon.h
@@ +205,5 @@
> +
> +  /* PROPERTY_BDNAME
> +     PROPERTY_BDADDR
> +     PROPERTY_REMOTE_FRIENDLY_NAME */
> +  nsString mString;

Why not wrap all value variables as an union?

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +316,5 @@
> +Convert(bt_property_type_t aIn, BluetoothPropertyType& aOut)
> +{
> +  static const BluetoothPropertyType sPropertyType[] = {
> +    CONVERT(0, static_cast<BluetoothPropertyType>(0)), // invalid, required by gcc
> +    CONVERT(BT_PROPERTY_BDNAME, PROPERTY_BDNAME),

Should |BluetoothPropertyType| also start from 0x1 as |bt_property_type|? Otherwise both 0 and BT_PROPERTY_BDNAME are converted to 0 (PROPERTY_BDNAME) and we cannot filter out the invalid value 0 with:

 if (aIn >= MOZ_ARRAY_LENGTH(sPropertyType) || !sPropertyType[aIn]) {
   return NS_ERROR_ILLEGAL_VALUE;
 }

@@ +365,5 @@
> +  };
> +  if (aIn == BT_PROPERTY_REMOTE_DEVICE_TIMESTAMP) {
> +    /* This case is handled separately to not populate
> +     * |sPropertyType| with empty entries. */
> +    aOut = NS_LITERAL_STRING("RemoteDeviceTimestamp");

I prefer to use |AssignLiteral|.

@@ +441,5 @@
> +Convert(bt_device_type_t aIn, BluetoothDeviceType& aOut)
> +{
> +  static const BluetoothDeviceType sDeviceType[] = {
> +    CONVERT(0, static_cast<BluetoothDeviceType>(0)), // invalid, required by gcc
> +    CONVERT(BT_DEVICE_DEVTYPE_BREDR, DEVICE_TYPE_BREDR),

The same question as |BluetoothPropertyType|.

@@ +801,5 @@
> +template<typename T>
> +static nsresult
> +Convert(const T& aIn, T& aOut)
> +{
> +#if 0

What's this non-executed section for?

@@ +2872,5 @@
> +  {
> +    bt_property_t* properties;
> +    nsAutoArrayPtr<bt_property_t> propertiesArray;
> +
> +    // See Bug 989976, consider aProperties address is not aligned. If

Wrap this section a function since multiple functions use it.

@@ +2877,5 @@
> +    // it looks unaligned, we make an aligned copy; otherwise we use
> +    // the pointer directly.
> +    if (reinterpret_cast<uintptr_t>(aProperties) % sizeof(void*)) {
> +      properties = new bt_property_t[aNumProperties];
> +      memcpy(properties, aProperties, aNumProperties * sizeof(*properties));

Replace |sizeof(*properties)| with |sizeof(bt_property_t)| to make it easier to understand as "size of single element".

@@ +2878,5 @@
> +    // the pointer directly.
> +    if (reinterpret_cast<uintptr_t>(aProperties) % sizeof(void*)) {
> +      properties = new bt_property_t[aNumProperties];
> +      memcpy(properties, aProperties, aNumProperties * sizeof(*properties));
> +      propertiesArray = properties;

What is |propertiesArray| for?

::: dom/bluetooth/bluedroid/BluetoothUtils.h
@@ +26,5 @@
>  BdAddressTypeToString(bt_bdaddr_t* aBdAddressType,
>                        nsAString& aRetBdAddress);
>  
>  uint16_t
>  UuidToServiceClassInt(bt_uuid_t* p_uuid);

Remove this function in patch [06] since it becomes useless.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8468412 [details] [diff] [review]
[03] Bug 1048915: Implement Bluetooth Core notifications

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

f=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1799,5 @@
> +  InfallibleTArray<BluetoothNamedValue> props;
> +
> +  for (int i = 0; i < aNumProperties; i++) {
> +
> +    const BluetoothProperty* p = aProperties + i;

I prefer to replace |aProperties + i| with |&aProperties[i]| to clearly represent i-th element of array.

@@ +1887,5 @@
> +
> +  bool isCodInvalid = false;
> +  for (int i = 0; i < aNumProperties; ++i) {
> +
> +    const BluetoothProperty* p = aProperties + i;

Ditto.

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8468413 [details] [diff] [review]
[04] Bug 1048915: Use Bluetooth Core notifications

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

LGTM.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8468414 [details] [diff] [review]
[05] Bug 1048915: Integrate helper runnables into notification methods

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

LGTM.

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8468415 [details] [diff] [review]
[06] Bug 1048915: Cleanup |BluetoothServiceBluedroid|

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

f=me. Also remove |UuidToServiceClassInt(bt_uuid_t* p_uuid)| in BluetoothUtil.h/.cpp since it becomes useless.

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8468410 [details] [diff] [review]
[01] Bug 1048915: Add infrastructure for Bluetooth notifications

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

f=me with nits addressed.

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +773,5 @@
> +  {
> +    MOZ_ASSERT(mMethod);
> +  }
> +
> +  template<typename T1,typename T2>

nit: space after comma.

@@ +856,5 @@
> +  {
> +    MOZ_ASSERT(mMethod);
> +  }
> +
> +  template<typename T1,typename T2, typename T3>

Ditto.

@@ +945,5 @@
> +  {
> +    MOZ_ASSERT(mMethod);
> +  }
> +
> +  template<typename T1,typename T2, typename T3, typename T4>

Ditto.

@@ +1043,5 @@
> +  {
> +    MOZ_ASSERT(mMethod);
> +  }
> +
> +  template<typename T1,typename T2, typename T3, typename T4, typename T5>

Ditto.

Comment 16

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

Hi Ben,

Thanks for the feedback. See below for some answers to your questions.


> ::: dom/bluetooth/BluetoothCommon.h
> @@ +205,5 @@
> > +
> > +  /* PROPERTY_BDNAME
> > +     PROPERTY_BDADDR
> > +     PROPERTY_REMOTE_FRIENDLY_NAME */
> > +  nsString mString;
> 
> Why not wrap all value variables as an union?

Because you can't have complex types in C++ unions. The compiler wouldn't know which destructor to call.


> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +316,5 @@
> > +Convert(bt_property_type_t aIn, BluetoothPropertyType& aOut)
> > +{
> > +  static const BluetoothPropertyType sPropertyType[] = {
> > +    CONVERT(0, static_cast<BluetoothPropertyType>(0)), // invalid, required by gcc
> > +    CONVERT(BT_PROPERTY_BDNAME, PROPERTY_BDNAME),
> 
> Should |BluetoothPropertyType| also start from 0x1 as |bt_property_type|?
> Otherwise both 0 and BT_PROPERTY_BDNAME are converted to 0 (PROPERTY_BDNAME)
> and we cannot filter out the invalid value 0 with:

Not starting the Bluedroid values from 0 and not assigning them consecutive numbers requires us to do all this monkey dancing around array initialization here. I don't want this to swap into the Gecko source code. Gecko  enumerators should allow for simple lookup tables without all these workarounds and special cases.


> 
> @@ +801,5 @@
> > +template<typename T>
> > +static nsresult
> > +Convert(const T& aIn, T& aOut)
> > +{
> > +#if 0
> 
> What's this non-executed section for?

Looks like I forgot this. The #else branch is definitely the correct code.


> 
> @@ +2877,5 @@
> > +    // it looks unaligned, we make an aligned copy; otherwise we use
> > +    // the pointer directly.
> > +    if (reinterpret_cast<uintptr_t>(aProperties) % sizeof(void*)) {
> > +      properties = new bt_property_t[aNumProperties];
> > +      memcpy(properties, aProperties, aNumProperties * sizeof(*properties));
> 
> Replace |sizeof(*properties)| with |sizeof(bt_property_t)| to make it easier
> to understand as "size of single element".

But that's very unsafe. If the type of |*properties| changes for some reason, you'd have to find all occurrences of |sizeof(bt_property_t)| and fix the issue. Been bitten by that before. :( 


> @@ +2878,5 @@
> > +    // the pointer directly.
> > +    if (reinterpret_cast<uintptr_t>(aProperties) % sizeof(void*)) {
> > +      properties = new bt_property_t[aNumProperties];
> > +      memcpy(properties, aProperties, aNumProperties * sizeof(*properties));
> > +      propertiesArray = properties;
> 
> What is |propertiesArray| for?

We allocate a temporary buffer iff the input buffer is not aligned correctly. |propertiesArray| cleans up the temporary buffer if it exists, or does nothing otherwise.

Comment 17

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16)
> > ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> > @@ +316,5 @@ 
> Not starting the Bluedroid values from 0 and not assigning them consecutive
> numbers requires us to do all this monkey dancing around array
> initialization here. I don't want this to swap into the Gecko source code.
> Gecko  enumerators should allow for simple lookup tables without all these
> workarounds and special cases.

How about check if |!aIn|?

  if (aIn >= MOZ_ARRAY_LENGTH(sPropertyType || !aIn)
    return NS_ERROR_ILLEGAL_VALUE;
  }

> > @@ +2878,5 @@
> We allocate a temporary buffer iff the input buffer is not aligned
> correctly. |propertiesArray| cleans up the temporary buffer if it exists, or
> does nothing otherwise.

I see. Please leave comment to explain its usage, otherwise it looks like an unused local variable.

Comment 18

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

> 
> How about check if |!aIn|?
> 
>   if (aIn >= MOZ_ARRAY_LENGTH(sPropertyType || !aIn)
>     return NS_ERROR_ILLEGAL_VALUE;
>   }

Hmm, looks like this is missing in the code, although it should be there. :( Thanks for pointing this out!

Comment 19

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

Attachment: [01] Bug 1048915: Add infrastructure for Bluetooth notifications (v2)

Changes since v1:

  - formatting

Comment 20

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

Attachment: [02] Bug 1048915: Add Bluetooth Core notifications (v3)

Changes since v2:

  - removed unused |Convert(bt_property_type_t, nsAString&)|
  - removed #if 0 section
  - moved alignment workaround to separate function
  - handle invalid bt_property_type_t of 0

Comment 21

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

Attachment: [03] Bug 1048915: Implement Bluetooth Core notifications (v2)

Changes since v2:

  - access property by reference

Comment 22

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

Attachment: [05] Bug 1048915: Integrate helper runnables into notification methods (v2)

Changes since v1:

  - rebased only

Comment 23

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

Attachment: [06] Bug 1048915: Cleanup |BluetoothServiceBluedroid| and related functions

Changes since v1:

  - removed unused functions in BluetoothUtils.{cpp,h}

Comment 24

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

Comment on attachment 8471548 [details] [diff] [review]
[02] Bug 1048915: Add Bluetooth Core notifications (v3)

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

::: dom/bluetooth/BluetoothCommon.h
@@ +193,5 @@
> +struct BluetoothVersionInfo {
> +  int mMajor;
> +  int mMinor;
> +  int mVendor;
> +};

I prefer to keep mVersion, mSubver, mManufacturer, it will be aligned with BT Spec LMP version information packet type. Maybe I missed something?

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +424,5 @@
> +
> +static nsresult
> +Convert(const bt_remote_version_t& aIn, BluetoothVersionInfo& aOut)
> +{
> +  aOut.mMajor = aIn.version;

s/mMajor/mVersion, likewise.

@@ +2826,5 @@
> +    // See Bug 989976: consider aProperties address is not aligned. If
> +    // it is aligned, we return the pointer directly; otherwise we make
> +    // an aligned copy. The argument |aPropertiesArray| keeps track of
> +    // the memory buffer.
> +    if (!(reinterpret_cast<uintptr_t>(aProperties) % sizeof(void*))) {

Wow! It never comes to me we can check address alignment like this way.

Comment 25

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #24)
> Comment on attachment 8471548 [details] [diff] [review]
> [02] Bug 1048915: Add Bluetooth Core notifications (v3)
> 
> Review of attachment 8471548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothCommon.h
> @@ +193,5 @@
> > +struct BluetoothVersionInfo {
> > +  int mMajor;
> > +  int mMinor;
> > +  int mVendor;
> > +};
> 
> I prefer to keep mVersion, mSubver, mManufacturer, it will be aligned with
> BT Spec LMP version information packet type. Maybe I missed something?

I don't really care, but what I don't like about this naming scheme is that the structure is already named 'Version' and 'mVersion' doesn't add any new information. 'mMajor' makes it clear that this is the major version number.

What do you think about renaming the structure to |BluetoothRemoteInfo| with the fields |mVerMajor|, |mVerMinor|, and |mManufacturer|?

> 
> @@ +2826,5 @@
> > +    // See Bug 989976: consider aProperties address is not aligned. If
> > +    // it is aligned, we return the pointer directly; otherwise we make
> > +    // an aligned copy. The argument |aPropertiesArray| keeps track of
> > +    // the memory buffer.
> > +    if (!(reinterpret_cast<uintptr_t>(aProperties) % sizeof(void*))) {
> 
> Wow! It never comes to me we can check address alignment like this way.

Most of all, it allows us to _not_ copy the array if the alignment is correct. :) There is a void* in aProperties, so the structure's alignment should always be according to the one of void*.

Comment 26

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

Comment on attachment 8471549 [details] [diff] [review]
[03] Bug 1048915: Implement Bluetooth Core notifications (v2)

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

r=me

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1783,5 @@
> +}
> +
> +/**
> + * AdapterPropertiesNotification will be called after enable() but
> + * before AdapterStateChangeCallback is called. At that moment, both

nit: s/AdapterStateChangeCallback/AdapterStateChangedNotification/

Comment 27

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #24)
> > Comment on attachment 8471548 [details] [diff] [review]
> > [02] Bug 1048915: Add Bluetooth Core notifications (v3)
> > 
> > Review of attachment 8471548 [details] [diff] [review]:
> What do you think about renaming the structure to |BluetoothRemoteInfo| with
> the fields |mVerMajor|, |mVerMinor|, and |mManufacturer|?
I'm fine with it. Thanks!

Comment 28

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

Comment on attachment 8471550 [details] [diff] [review]
[05] Bug 1048915: Integrate helper runnables into notification methods (v2)

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

r=me, thanks so much for this patch to get rid of evil Runnables. God bless Bluetooth. :D

Comment 29

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

Comment on attachment 8471551 [details] [diff] [review]
[06] Bug 1048915: Cleanup |BluetoothServiceBluedroid| and related functions

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

r=me, it's time to forget the past, and move on. :D

Comment 30

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

Comment on attachment 8471543 [details] [diff] [review]
[01] Bug 1048915: Add infrastructure for Bluetooth notifications (v2)

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

Thomas, sorry for late reply.

Comment 31

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

Attachment: [02] Bug 1048915: Add Bluetooth Core notifications (v4)

Changes since v3:

  - renamed |BluetoothVersionInfo| to |BluetoothRemoteInfo|

Comment 32

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

Attachment: [03] Bug 1048915: Implement Bluetooth Core notifications (v3)

Changes since v2:

  - fixed typo in comment

Comment 33

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

Attachment: [06] Bug 1048915: Cleanup |BluetoothServiceBluedroid| and related functions (v2)

Changes since v1:

  - rebased

Comment 34

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

Comment on attachment 8472977 [details] [diff] [review]
[02] Bug 1048915: Add Bluetooth Core notifications (v4)

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

r=me, thanks for the hard work.

Comment 35

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

Thanks! :)

https://hg.mozilla.org/integration/b2g-inbound/rev/bcd03ffa60fb
https://hg.mozilla.org/integration/b2g-inbound/rev/adc5f5c041d4
https://hg.mozilla.org/integration/b2g-inbound/rev/2ad70d0cab34
https://hg.mozilla.org/integration/b2g-inbound/rev/98b89e96809d
https://hg.mozilla.org/integration/b2g-inbound/rev/cd684b601f46
https://hg.mozilla.org/integration/b2g-inbound/rev/4cdcd428e51c

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=4cdcd428e51c

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcd03ffa60fb
https://hg.mozilla.org/mozilla-central/rev/adc5f5c041d4
https://hg.mozilla.org/mozilla-central/rev/2ad70d0cab34
https://hg.mozilla.org/mozilla-central/rev/98b89e96809d
https://hg.mozilla.org/mozilla-central/rev/cd684b601f46
https://hg.mozilla.org/mozilla-central/rev/4cdcd428e51c

Comment 37

[Michael Vines [:m1] [:evilmachines]]

Any idea :tdz?   This is a KK build.

../../../../../../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp: In function 'nsresult mozilla::dom::bluetooth::Convert(bt_property_type_t, mozilla::dom::bluetooth::BluetoothPropertyType&)':
../../../../../../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:334:3: sorry, unimplemented: non-trivial designated initializers not supported

Comment 38

[Michael Vines [:m1] [:evilmachines]]

Ugh.  We have a non-AOSP entry in bt_property_type_t that was added in the middle of the enum so it breaks the initializer.  Will fix this here.

Comment 39

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

Hi Michael,
We have bug 1054218 to track with.

Comment 40

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

Would you also add Q_BLUETOOTH flag in Bluetooth HFP/A2DP callback section for non-AOSP interface?

Comment 41

[Michael Vines [:m1] [:evilmachines]]

We don't need that or bug 1054218 right now, the proper fix goes in CAF bluetooth.h.