1171866 - [bluetooth2] Remove ReversedUuidToString

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

Suggest to do UUID reversion inside Bluetooth backend wrapper.

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1171866_remove_reverse_uuid_to_string.patch

Comment 2

[Bruce Sun [:brsun]]

Attachment: https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/34

Comment 3

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

Comment on attachment 8616523 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/34

Belongs into Gecko, except BlueZ does it this way.

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1171866_remove_reverse_uuid_to_string.v2.patch

Address comment 3.

Comment 5

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

Comment on attachment 8616620 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v2.patch

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

The general patch seems good. ACK'ed with a fix of the copying overhead in the daemon backend.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +137,5 @@
>    uint8_t mName[256];
>  };
>  
> +// reversed GATT UUID, see bug 1171866
> +struct BluetoothReversedUuid {

That requires unnecessary copying of the UUID. Rather define structures similar to the existing |PackArray| and |PackConversion| structures.

  template<typename T>
  struct PackReversed<T> {

    PackReversed(const T& aValue)
      : mValue(aValue)
    { }

    const T& mValue;
  };

  template<typename T>
  struct UnpackReversed<T> {

    UnpackReversed(T& aValue)
      : mValue(&aValue)
    { }

    T* mValue;
  };

This will allow you to implement functions for packing and unpacking that don't require copying, but (de-)serialize directly into/from a PDU's buffer.

  nsresult
  PackPDU(const PackReversed<BluetoothUuid>& aIn, BluetoothDaemonPDU& aPDU)

  nsresult
  UnpackPDU(BluetoothDaemonPDU& aPDU, const UnpackReversed<BluetoothUuid>& aOut)

Comment 6

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

Comment on attachment 8616620 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v2.patch

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

Cancel my review request for now, please request for review after Thomas's feedback being addressed.

Thanks,
Jocelyn

Comment 7

[Bruce Sun [:brsun]]

Attachment: bug1171866_remove_reverse_uuid_to_string.v3.patch

Address comment 5.

Comment 8

[Bruce Sun [:brsun]]

Comment on attachment 8617783 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v3.patch

Hi Thomas,

May I have your feedback again to make sure my understanding of comment 5 is correct?

Comment 9

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

Comment on attachment 8617783 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v3.patch

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

This looks good. Thanks a lot!

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +1164,5 @@
>   */
>  nsresult
>  UnpackPDU(BluetoothDaemonPDU& aPDU, const UnpackString0& aOut);
>  
> +/* |UnpackReversedArray| is a helper for unpacking arrays in a reversed order.

Maybe write

  'for unpacking array elements in reversed order'

because if an element is more than one byte in size, the order of elements will be reversed, but not the bytes within each element.

@@ +1169,5 @@
> + * Pass an instance of this structure as the second argument to |UnpackPDU| to
> + * unpack an array in a reversed order.
> + */
> +template <typename T>
> +struct UnpackReversedArray

Oh, you have a separate array struct. Nice! :)

If you want to get fancy, you could implement |UnpackReversedArray| with a combination of |UnpackArray| and |UnpackReversed|. But the current patch is also fine!

@@ +1211,5 @@
> +}
> +
> +template<typename T>
> +inline nsresult
> +UnpackPDU(BluetoothDaemonPDU& aPDU, UnpackReversedArray<T>& aOut)

A special variant for non-const structures?

Comment 10

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

Comment on attachment 8617783 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v3.patch

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

Good job. :)

Comment 11

[Bruce Sun [:brsun]]

Attachment: bug1171866_remove_reverse_uuid_to_string.v4.patch

This is the fancy one. :)

Comment 12

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

Comment on attachment 8620786 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v4.patch

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

Great! :)

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +580,5 @@
>  }
>  
> +/* |PackReversed| is a helper for packing data in a reversed order. Pass an
> + * instance of this structure as the first argument to |PackPDU| to pack data
> + * in a reversed order.

Just a small nit: one says 'in reversed order' without 'a'. Here and everywhere else.

@@ +586,5 @@
> +template<typename T>
> +struct PackReversed
> +{
> +  PackReversed(const T& aValue)
> +  : mValue(aValue)

4-byte indention for the initializers as illustrated here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Comment 13

[Bruce Sun [:brsun]]

Attachment: bug1171866_remove_reverse_uuid_to_string.commit.patch

Comment 14

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #13)
> Created attachment 8620885 [details] [diff] [review]
> bug1171866_remove_reverse_uuid_to_string.commit.patch

Address comment 12.

Comment 15

[Bruce Sun [:brsun]]

|ReversedUuidToString| is used in Bluetooth API v2, which is not covered by try server currently.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/4a91e5fda5e2

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/4a91e5fda5e2