Closed Bug 853235 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gyeh, Assigned: gyeh)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(2 files, 2 obsolete files)

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".
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)
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)
Component: DOM: Device Interfaces → Bluetooth
Product: Core → Boot2Gecko
Assignee: nobody → gyeh
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+
Blocks: 873917
Blocks: 873919
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"}
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.
Attachment #743532 - Attachment is obsolete: true
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/a9547c16624f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 → ---
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+
Thanks, Eric!
Attachment #753218 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7c6be1bed3d5
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.