All users were logged out of Bugzilla on October 13th, 2018

[bluetooth2] Remove ReversedUuidToString

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
2.2 S14 (12june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
Suggest to do UUID reversion inside Bluetooth backend wrapper.
(Assignee)

Updated

3 years ago
Blocks: 1171868
(Assignee)

Updated

3 years ago
Blocks: 1161945
(Assignee)

Comment 1

3 years ago
Created attachment 8616522 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.patch
Attachment #8616522 - Flags: review?(joliu)
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.
Attachment #8616523 - Flags: review?(tzimmermann) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8616620 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v2.patch

Address comment 3.
Attachment #8616522 - Attachment is obsolete: true
Attachment #8616523 - Attachment is obsolete: true
Attachment #8616522 - Flags: review?(joliu)
Attachment #8616620 - Flags: review?(joliu)
Attachment #8616620 - Flags: feedback?(tzimmermann)
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)
Attachment #8616620 - Flags: feedback?(tzimmermann) → feedback+
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
Attachment #8616620 - Flags: review?(joliu)
(Assignee)

Comment 7

3 years ago
Created attachment 8617783 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v3.patch

Address comment 5.
Attachment #8616620 - Attachment is obsolete: true
Attachment #8617783 - Flags: review?(joliu)
(Assignee)

Comment 8

3 years ago
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?
Attachment #8617783 - Flags: feedback?(tzimmermann)
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?
Attachment #8617783 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 8617783 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v3.patch

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

Good job. :)
Attachment #8617783 - Flags: review?(joliu) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8620786 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.v4.patch

This is the fancy one. :)
Attachment #8617783 - Attachment is obsolete: true
Attachment #8620786 - Flags: feedback?(tzimmermann)
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
Attachment #8620786 - Flags: feedback?(tzimmermann) → feedback+
(Assignee)

Comment 13

3 years ago
Created attachment 8620885 [details] [diff] [review]
bug1171866_remove_reverse_uuid_to_string.commit.patch
Attachment #8620786 - Attachment is obsolete: true
Attachment #8620885 - Flags: review+
Attachment #8620885 - Flags: feedback+
(Assignee)

Comment 14

3 years ago
(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.
(Assignee)

Comment 15

3 years ago
|ReversedUuidToString| is used in Bluetooth API v2, which is not covered by try server currently.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a91e5fda5e2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
You need to log in before you can comment on or make changes to this bug.