[b2g-bluetooth] Combine pairing-related system messages into one type

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gyeh, Assigned: gyeh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Right now, we have three different types of Bluetooth pairing-related system messages. Discussed with Evelyn, we'd like to combine them into one with a property named "type".

Comment 1

6 years ago
May I know what types you're referring to? Does it block anything or just an improvement in source code level? Thanks!
Flags: needinfo?(gyeh)
(Assignee)

Comment 2

6 years ago
No worries! Just an inprovement in source code leve. Will have a discussion witih Evelyn later. She's working on refactoring the bluetooth part of Settings App, and we think we can make our code shorter and clearer. :)
Flags: needinfo?(gyeh)
(Assignee)

Updated

6 years ago
Component: DOM: Device Interfaces → Bluetooth
Product: Core → Boot2Gecko
(Assignee)

Updated

6 years ago
Assignee: nobody → gyeh
(Assignee)

Comment 3

6 years ago
Created attachment 743532 [details] [diff] [review]
Patch 1(v1): Combine pairing-related system messages into one type
Attachment #743532 - Flags: review?(echou)
(Assignee)

Comment 4

6 years ago
A new type of system message is created: "bluetooth-pairing-request"

This system message combines original three types ("bluetooth-requestconfirmation", "bluetooth-requestpincode", and "bluetooth-requestpasskey") into one and has one extra property named "method" to distinguish different pairing methods.
Comment on attachment 743532 [details] [diff] [review]
Patch 1(v1): Combine pairing-related system messages into one type

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

Overall looks good, so r=me with nits addressed. Nevertheless, you need Gaia support to listen to the different topic of system message. Please make sure not to let 'nobody listens to the new system messages' problem happens. Thank you.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +495,5 @@
>                                   NS_LITERAL_STRING("passkey"),
>                                   passkey));
> +      parameters.AppendElement(BluetoothNamedValue(
> +                                 NS_LITERAL_STRING("method"),
> +                                 NS_LITERAL_STRING("confirmation")));

How about exchange this with the the "passkey" element? That would make "method" become the second parameter just like other parts.

@@ +1989,5 @@
> +  }
> +  const InfallibleTArray<BluetoothNamedValue>& arr =
> +    v.get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(arr[0].name().EqualsLiteral("path"));
> +  MOZ_ASSERT(arr[0].value().type() == BluetoothValue::TnsString);

It looks like this part of code is written for debugging. Please use |#ifdef DEBUG| to wrap it.
(or maybe we should do error handling here)
Attachment #743532 - Flags: review?(echou) → review+
(Assignee)

Updated

5 years ago
Blocks: 873917
(Assignee)

Updated

5 years ago
Blocks: 873919
(Assignee)

Comment 6

5 years ago
Example messages as following:


Broadcasting bluetooth-pairing-request {"address":"00:1E:3B:1F:FC:99","method":"confirmation","passkey":123456,"name":"test"}
Broadcasting bluetooth-pairing-request {"address":"00:1E:3B:1F:FC:99","method":"pincode","name":"test"}
Broadcasting bluetooth-pairing-request {"address":"00:1E:3B:1F:FC:99","method":"passkey","name":"test"}
(Assignee)

Comment 7

5 years ago
Comment on attachment 743532 [details] [diff] [review]
Patch 1(v1): Combine pairing-related system messages into one type

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +495,5 @@
>                                   NS_LITERAL_STRING("passkey"),
>                                   passkey));
> +      parameters.AppendElement(BluetoothNamedValue(
> +                                 NS_LITERAL_STRING("method"),
> +                                 NS_LITERAL_STRING("confirmation")));

Agree. Will update in final patch.

@@ +1989,5 @@
> +  }
> +  const InfallibleTArray<BluetoothNamedValue>& arr =
> +    v.get_ArrayOfBluetoothNamedValue();
> +  MOZ_ASSERT(arr[0].name().EqualsLiteral("path"));
> +  MOZ_ASSERT(arr[0].value().type() == BluetoothValue::TnsString);

I'd prefer to do error handling here. Will also update in final patch.
(Assignee)

Comment 8

5 years ago
Created attachment 751564 [details] [diff] [review]
Final patch, r=echou
Attachment #743532 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/a9547c16624f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

5 years ago
I'd like to re-open this issue since we need to keep original bluetooth pairing request mechanism for now and remove them after Bug 873917 (Gaia part) is landed.

After that, I will revert patch 2 in Bug 873919 (Gecko part).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

5 years ago
Created attachment 753218 [details] [diff] [review]
Patch 2: Keep original mechanisms of pairing request

Eric, please help to review. Thanks.
Attachment #753218 - Flags: review?(echou)
Comment on attachment 753218 [details] [diff] [review]
Patch 2: Keep original mechanisms of pairing request

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

::: dom/bluetooth/BluetoothService.cpp
@@ +752,5 @@
>  BluetoothService::Notify(const BluetoothSignal& aData)
>  {
> +  /**
> +   * We'd like to keep old mechanisms for now and remove them after Bug 873917
> +   * is fixed. Basically, three system messages are going to merged into one

super-nit: going to /be/ merged
Attachment #753218 - Flags: review?(echou) → review+
(Assignee)

Comment 15

5 years ago
Thanks, Eric!
(Assignee)

Comment 16

5 years ago
Created attachment 753681 [details] [diff] [review]
Patch 2 (final version), r=echou
Attachment #753218 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7c6be1bed3d5
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.