Closed Bug 1048915 Opened 6 years ago Closed 6 years ago

[Bluetooth] Convert Core callbacks to backend-neutral interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(6 files, 9 obsolete files)

3.94 KB, patch
shawnjohnjr
: review+
ben.tian
: feedback+
Details | Diff | Splinter Review
12.49 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
9.75 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
26.04 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
17.63 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
23.02 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8468411 - Flags: review?(shuang)
Attachment #8468411 - Flags: feedback?(btian)
Attachment #8468412 - Flags: review?(shuang)
Attachment #8468412 - Flags: feedback?(btian)
Attachment #8468415 - Flags: review?(shuang)
Attachment #8468415 - Flags: feedback?(btian)
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 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.
Attachment #8468411 - Flags: review?(shuang)
Changes since v1:

  - use CONVERT macro in lookup tables
Attachment #8468411 - Attachment is obsolete: true
Attachment #8469927 - Flags: review?(shuang)
Attachment #8469927 - Flags: feedback?(btian)
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.
Attachment #8469927 - Flags: feedback?(btian)
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.
Attachment #8468412 - Flags: feedback?(btian) → feedback+
Comment on attachment 8468413 [details] [diff] [review]
[04] Bug 1048915: Use Bluetooth Core notifications

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

LGTM.
Attachment #8468413 - Flags: feedback?(btian) → feedback+
Comment on attachment 8468414 [details] [diff] [review]
[05] Bug 1048915: Integrate helper runnables into notification methods

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

LGTM.
Attachment #8468414 - Flags: feedback?(btian) → feedback+
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.
Attachment #8468415 - Flags: feedback?(btian) → feedback+
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.
Attachment #8468410 - Flags: feedback?(btian) → feedback+
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.
(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.
Flags: needinfo?(tzimmermann)
> 
> 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!
Flags: needinfo?(tzimmermann)
Changes since v1:

  - formatting
Attachment #8468410 - Attachment is obsolete: true
Attachment #8468410 - Flags: review?(shuang)
Attachment #8471543 - Flags: review?(shuang)
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
Attachment #8469927 - Attachment is obsolete: true
Attachment #8469927 - Flags: review?(shuang)
Attachment #8471548 - Flags: review?(shuang)
Changes since v2:

  - access property by reference
Attachment #8468412 - Attachment is obsolete: true
Attachment #8468412 - Flags: review?(shuang)
Attachment #8471549 - Flags: review?(shuang)
Changes since v1:

  - rebased only
Attachment #8468414 - Attachment is obsolete: true
Attachment #8468414 - Flags: review?(shuang)
Attachment #8471550 - Flags: review?(shuang)
Changes since v1:

  - removed unused functions in BluetoothUtils.{cpp,h}
Attachment #8468415 - Attachment is obsolete: true
Attachment #8468415 - Flags: review?(shuang)
Attachment #8471551 - Flags: review?(shuang)
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.
Attachment #8471548 - Flags: review?(shuang)
(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 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/
Attachment #8471549 - Flags: review?(shuang) → review+
(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 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
Attachment #8471550 - Flags: review?(shuang) → review+
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
Attachment #8471551 - Flags: review?(shuang) → review+
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.
Attachment #8471543 - Flags: review?(shuang) → review+
Changes since v3:

  - renamed |BluetoothVersionInfo| to |BluetoothRemoteInfo|
Attachment #8471548 - Attachment is obsolete: true
Attachment #8472977 - Flags: review?(shuang)
Changes since v2:

  - fixed typo in comment
Attachment #8471549 - Attachment is obsolete: true
Attachment #8472979 - Flags: review+
Changes since v1:

  - rebased
Attachment #8471551 - Attachment is obsolete: true
Attachment #8472982 - Flags: review+
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.
Attachment #8472977 - Flags: review?(shuang) → review+
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
Flags: needinfo?(tzimmermann)
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.
Flags: needinfo?(tzimmermann)
Would you also add Q_BLUETOOTH flag in Bluetooth HFP/A2DP callback section for non-AOSP interface?
Flags: needinfo?(mvines)
We don't need that or bug 1054218 right now, the proper fix goes in CAF bluetooth.h.
Flags: needinfo?(mvines)
You need to log in before you can comment on or make changes to this bug.