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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

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

[Kai-Chih Hu [:khu]]

May I know what types you're referring to? Does it block anything or just an improvement in source code level? Thanks!

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

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. :)

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Combine pairing-related system messages into one type

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

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 5

[Eric Chou [:ericchou] [:echou]]

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)

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

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 7

[Gina Yeh [:gyeh] [:ginayeh]]

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.

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Final patch, r=echou

Comment 9

[Gina Yeh [:gyeh] [:ginayeh]]

http://hg.mozilla.org/projects/birch/rev/a9547c16624f

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

try server:
https://tbpl.mozilla.org/?tree=Try&rev=db32e1c2bdb4
https://tbpl.mozilla.org/?tree=Try&rev=c855d7814125

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9547c16624f

Comment 12

[Gina Yeh [:gyeh] [:ginayeh]]

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).

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2: Keep original mechanisms of pairing request

Eric, please help to review. Thanks.

Comment 14

[Eric Chou [:ericchou] [: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

Comment 15

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks, Eric!

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2 (final version), r=echou

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

try: 
https://tbpl.mozilla.org/?tree=Try&rev=27fb52fc31f6
https://tbpl.mozilla.org/?tree=Try&rev=0522d9895644

Comment 18

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/projects/birch/rev/7c6be1bed3d5

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7c6be1bed3d5