1156229 - Support Bluetooth v2 on L

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

We should be able to build Bluetooth v2 on L.

Comment 1

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

Attachment: [01] Bug 1156229: Make ref-counted class destructors non-public

Most of these cases were originally fixed in bug 1137151.

Comment 2

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

Attachment: [02] Bug 1156229: Update GATT support to include Android L

Comment 3

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

These two patches allowed to build Bluetooth v2 on nexus-5-l.

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8594700 [details] [diff] [review]
[01] Bug 1156229: Make ref-counted class destructors non-public

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

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +208,5 @@
> +    mReadCharacteristicState.Reset();
> +    mWriteCharacteristicState.Reset();
> +    mReadDescriptorState.Reset();
> +    mWriteDescriptorState.Reset();
> +  }

Hi Thomas,

Just FYI, this destructor will be removed by Bug 1063444 since I found out it's actually redundant when reviewing Bug 1063444.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8594700 [details] [diff] [review]
[01] Bug 1156229: Make ref-counted class destructors non-public

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

Steal this review since there are only few lines left after I took a look at GattManager.cpp/h. ;)

Comment 6

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

I think, ref-counted classes will need a non-public destructor, even if it's empty. If this destructor gets removed, I'll add an empty one.

Comment 7

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

Notes

  - fix virtual ~BluetoothGATTManager in [01]
  - protect helpers by BT_API_V2 in [02]

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8594700 [details] [diff] [review]
[01] Bug 1156229: Make ref-counted class destructors non-public

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

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +208,5 @@
> +    mReadCharacteristicState.Reset();
> +    mWriteCharacteristicState.Reset();
> +    mReadDescriptorState.Reset();
> +    mWriteDescriptorState.Reset();
> +  }

Per Comment 6, let's make it empty.

::: dom/bluetooth/bluedroid/BluetoothGattManager.h
@@ +84,5 @@
>      const nsTArray<uint8_t>& aValue,
>      BluetoothReplyRunnable* aRunnable);
>  
>  private:
> +  virtual ~BluetoothGattManager();

Per irc discussion, agreed there's no point to use virtual since this class is final.

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8594701 [details] [diff] [review]
[02] Bug 1156229: Update GATT support to include Android L

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

::: dom/bluetooth/bluedroid/BluetoothGattHALInterface.cpp
@@ +513,5 @@
>  void
>  BluetoothGattClientHALInterface::Scan(
>    int aClientIf, bool aStart, BluetoothGattClientResultHandler* aRes)
>  {
> +#if ANDROID_VERSION >= 20

Should be 21 here and the rest of this patch.

@@ +1039,5 @@
> +  /* FIXME: This method allocates a large amount of memory on the
> +   * stack. It should be rewritten to prevent the pending stack
> +   * overflow. Additionally |ArrayBuffer| seems like the wrong data
> +   * type here. Why not use plain pointers instead?
> +   */

Yes, no need to be an ArrayBuffer type here.
Also, we currently don't use this function since it's related to peripheral mode.
Peripheral mode will be supported later after we finish the first edition of GATT implementation.
I think we could fix it when we implement peripheral mode support.

Comment 10

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

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

Comment 11

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

Attachment: [01] Bug 1156229: Make ref-counted class destructors non-public (v2)

Changes since v1:

  - fixed virtual constructor
  - empty constructor

Comment 12

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

Attachment: [02] Bug 1156229: Update GATT support to include Android L (v2)

Changes since v1:

  - depend on ANDROID_VERSION >= 21

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/508aec3a6892
https://hg.mozilla.org/integration/b2g-inbound/rev/a07afbd58689

Comment 14

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

Thanks Jocelyn!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=a07afbd58689

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/508aec3a6892
https://hg.mozilla.org/mozilla-central/rev/a07afbd58689