1036228 - Implement BluetoothPairingListener

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

* BluetoothPairingRequestListeningHandle: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingRequestListeningHandle

Comment 1

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

Attachment: [WIP] Bug 1036228: Add BluetoothPairingRequestListeningHandle.webidl and implement it

* add BluetoothPairingRequestListeningHandle.webidl
* implement BluetoothPairingRequestListeningHandle.cpp/h

Comment 2

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

Attachment: [WIP] Bug 1036228 - Patch1: Add BluetoothPairingRequestListeningHandle.webidl and implement it

* Implement BluetoothPairingRequestListeningHandle
* Notify BluetoothAdapter for pairing requests in PinRequestCallback and
  SspRequestCallback.

Comment 3

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

Attachment: [WIP] Bug 1036228 - Patch2: Handle pairing requests and fire pairing events in BluetoothAdapter.

* Add attribute BluetoothPairingRequestListeningHandle pairingReqs in BluetoothAdapter.
* Handle PairingRequest signals notified by bluedroid and fire PairingEvent for Gaia

Comment 4

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

Attachment: Bug 1036228 - Patch1(v1): Add BluetoothPairingRequestListeningHandle.webidl and implement it

* revise |DispatchPairingEvent| based on latest BluetoothPairingEvent implementation
* split service part into another patch

Comment 5

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

Attachment: Bug 1036228 - Patch2(v1): Handle pairing requests reported by bluedroid in bluetooth services.

* handle pairing requests in bluedroid service

Comment 6

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

Attachment: Bug 1036228 - Patch3(v1): Handle pairing requests and fire pairing events in BluetoothAdapter.

* bug fixing

Comment 7

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

Comment on attachment 8463812 [details] [diff] [review]
Bug 1036228 - Patch3(v1): Handle pairing requests and fire pairing events in BluetoothAdapter.

* handle pairing requests and fire pairing events
* add BluetoothPairingRequestListeningHandle attribute in BluetoothAdapter

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8463810 [details] [diff] [review]
Bug 1036228 - Patch1(v1): Add BluetoothPairingRequestListeningHandle.webidl and implement it

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

r=me with nits addressed.

::: dom/bluetooth2/BluetoothPairingRequestListeningHandle.cpp
@@ +19,5 @@
> +NS_IMPL_RELEASE_INHERITED(BluetoothPairingRequestListeningHandle,
> +                          DOMEventTargetHelper)
> +
> +BluetoothPairingRequestListeningHandle::BluetoothPairingRequestListeningHandle(
> +    nsPIDOMWindow* aWindow)

nit: 2-space indent.

::: dom/bluetooth2/BluetoothPairingRequestListeningHandle.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_bluetooth_BluetoothPairingRequestListeningHandle_h
> +#define mozilla_dom_bluetooth_BluetoothPairingRequestListeningHandle_h

nit: we usually use lower case here.

@@ +24,5 @@
> +  static already_AddRefed<BluetoothPairingRequestListeningHandle>
> +    Create(nsPIDOMWindow* aWindow);
> +
> +  void
> +    DispatchPairingEvent(BluetoothDevice* aDevice,

nit: wrap into 1 line.

@@ +47,5 @@
> +};
> +
> +END_BLUETOOTH_NAMESPACE
> +
> +#endif // mozilla_dom_bluetooth_BluetoothPairingRequestListeningHandle_h

Ditto.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8463811 [details] [diff] [review]
Bug 1036228 - Patch2(v1): Handle pairing requests reported by bluedroid in bluetooth services.

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

Please see my comment below.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +646,5 @@
> +                         BluetoothValue(props));
> +
> +  nsRefPtr<DistributeBluetoothSignalTask> task =
> +    new DistributeBluetoothSignalTask(signal);
> +

nit: remove this newline.

@@ +662,5 @@
>  
> +  InfallibleTArray<BluetoothNamedValue> props;
> +  nsAutoString deviceAddress;
> +  BdAddressTypeToString(aRemoteBdAddress, deviceAddress);
> +  BT_APPEND_NAMED_VALUE(props, "address", deviceAddress);

I suggest to group this line with following |BT_APPEND_NAMED_VALUE| to clearly see which parameters are wrapped.

@@ +667,4 @@
>  
> +  nsAutoString passkey;
> +  nsAutoString pairingType;
> +  if (aPairingVariant == BT_SSP_VARIANT_PASSKEY_CONFIRMATION) {

Make here a |switch|. Also leave comment to state which parameters are assigned for each pairing variant.

@@ +667,5 @@
>  
> +  nsAutoString passkey;
> +  nsAutoString pairingType;
> +  if (aPairingVariant == BT_SSP_VARIANT_PASSKEY_CONFIRMATION) {
> +    pairingType = NS_LITERAL_STRING(PAIRING_REQ_TYPE_CONFIRMATION);

Use |AssignLiteral|.

@@ +688,5 @@
> +                         BluetoothValue(props));
> +
> +  nsRefPtr<DistributeBluetoothSignalTask> task =
> +    new DistributeBluetoothSignalTask(signal);
> +

Ditto.

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8463812 [details] [diff] [review]
Bug 1036228 - Patch3(v1): Handle pairing requests and fire pairing events in BluetoothAdapter.

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

r=me with comment addressed.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +821,5 @@
>  
>  void
> +BluetoothAdapter::HandlePairingRequest(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);

Add MOZ_ASSERT(mPairingReqs).

Comment 11

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

Attachment: Bug 1036228 - Patch1(v2): Add BluetoothPairingRequestListeningHandle.webidl and implement it. r=btian

Comment 12

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

Attachment: Bug 1036228 - Patch2(v2): Handle pairing requests reported by bluedroid in bluetooth services.

* address review comments

Comment 13

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

Attachment: Bug 1036228 - Patch3(v2): Handle pairing requests and fire pairing events in BluetoothAdapter. r=btian

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8465210 [details] [diff] [review]
Bug 1036228 - Patch2(v2): Handle pairing requests reported by bluedroid in bluetooth services.

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

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +645,5 @@
> +                         NS_LITERAL_STRING(KEY_ADAPTER),
> +                         BluetoothValue(props));
> +  nsRefPtr<DistributeBluetoothSignalTask> task =
> +    new DistributeBluetoothSignalTask(signal);
> +

I meant to remove 'this' newline rather than the one above.

@@ +671,5 @@
> +   *
> +   * passkey value based on pairing request type:
> +   * 1) empty string: PAIRING_REQ_TYPE_CONSENT
> +   * 2) aPasskey: PAIRING_REQ_TYPE_CONFIRMATION and
> +   *              PAIRING_REQ_TYPE_DISPLAYPASSKEY

nit: put |aPasskey| as 1) and reorder switch cases to match this comment.

  // 1) aPasskey: PAIRING_REQ_TYPE_CONFIRMATION and PAIRING_REQ_TYPE_DISPLAYPASSKEY
  // 2) empty string: PAIRING_REQ_TYPE_CONSENT
  ...
  switch () {
    case PAIRING_REQ_TYPE_CONFIRMATION:
      ...
      break;
    case PAIRING_REQ_TYPE_DISPLAYPASSKEY:
      ...
      break;
    case PAIRING_REQ_TYPE_CONSENT:
      ...
      break;
    default:
      ...
  }

@@ +676,5 @@
> +   */
> +  switch (aPairingVariant) {
> +    case BT_SSP_VARIANT_PASSKEY_CONFIRMATION:
> +      pairingType.AssignLiteral(PAIRING_REQ_TYPE_CONFIRMATION);
> +      passkey.AppendInt(aPasskey);

Add missing break for each case.

@@ +678,5 @@
> +    case BT_SSP_VARIANT_PASSKEY_CONFIRMATION:
> +      pairingType.AssignLiteral(PAIRING_REQ_TYPE_CONFIRMATION);
> +      passkey.AppendInt(aPasskey);
> +    case BT_SSP_VARIANT_CONSENT:
> +      pairingType.AssignLiteral(PAIRING_REQ_TYPE_CONSENT);

Add |passkey.Truncate()|.

@@ +696,5 @@
> +                         NS_LITERAL_STRING(KEY_ADAPTER),
> +                         BluetoothValue(props));
> +  nsRefPtr<DistributeBluetoothSignalTask> task =
> +    new DistributeBluetoothSignalTask(signal);
> +

Ditto.

Comment 15

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

Attachment: Bug 1036228 - Patch2(v3): Handle pairing requests reported by bluedroid in bluetooth services.

Ah, sorry for misunderstanding and missing breaks.
Comments are addressed, please review it again.

Comment 16

[Ben Tian (inactive)]

Comment on attachment 8465217 [details] [diff] [review]
Bug 1036228 - Patch2(v3): Handle pairing requests reported by bluedroid in bluetooth services.

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

r=me, Thanks!

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +684,5 @@
> +      passkey.AppendInt(aPasskey);
> +      break;
> +    case BT_SSP_VARIANT_CONSENT:
> +      pairingType.AssignLiteral(PAIRING_REQ_TYPE_CONSENT);
> +      passkey.Truncate();

Sorry please remove this redundant truncation. |passkey| is already empty string when it's created.

Comment 17

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

Attachment: [Final] Bug 1036228 - Patch2: Handle pairing requests reported by bluedroid in bluetooth services. r=btian

Thanks for the review, Ben.

Comment 18

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

Comment on attachment 8465209 [details] [diff] [review]
Bug 1036228 - Patch1(v2): Add BluetoothPairingRequestListeningHandle.webidl and implement it. r=btian

Hi Boris,

In our new API, we hold a BluetothPairingRequestListeningHandle[1] in BluetoothAdapter[2] for notifying pairing requests to applications.

Here's the summary of the patch set
patch1:
  * Add BluetoothPairingRequestListeningHandle.webidl
  * BluetoothPairingRequestListeningHandle implementation including |DispatchPairingEvent| for adapter to notify applications of pairing requests

patch2: (covers BT module only)
  * handle pairing requests in bluetooth services

patch3:
  * add mPairingReqs in BluetoothAdapter
  * handle and notify applications of pairing requests

Since patch1 and patch3 expose some APIs, could you also take a look from DOM perspective?

Thanks,
Jocelyn

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingRequestListeningHandle#Interface
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#pairingReqs

Comment 19

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

Comment on attachment 8465211 [details] [diff] [review]
Bug 1036228 - Patch3(v2): Handle pairing requests and fire pairing events in BluetoothAdapter. r=btian

This is the patch3 mentioned in previous comment.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465209 [details] [diff] [review]
Bug 1036228 - Patch1(v2): Add BluetoothPairingRequestListeningHandle.webidl and implement it. r=btian

I really wish the Bluetooth stuff were not off in its own little "bluetooth" namespace... especially because it then includes "bluetooth" in the classnames anyway.  Followup bug to get rid of that, please?

>+#include "BluetoothPairingRequestListeningHandle.h"

  #include "mozilla/dom/BluetoothPairingRequestListeningHandle.h"

>+#include "BluetoothPairingHandle.h"

Likewise, mozilla/dom/BluetoothPairingHandle.h

BluetoothPairingRequestListeningHandle is a pretty long class name, and tells me nothing about what this thing is for.  What _is_ it for?  Can we come up with a better name?

r=me modulo that.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465211 [details] [diff] [review]
Bug 1036228 - Patch3(v2): Handle pairing requests and fire pairing events in BluetoothAdapter. r=btian

>+#include "BluetoothPairingRequestListeningHandle.h"

mozilla/dom

In general, exported headers should be consistently included from the export location.

r=me

Comment 22

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

Attachment: [Final] Bug 1036228 - Patch1: Add BluetoothPairingRequestListeningHandle.webidl and implement it. r=btian, r=bz

* include exported headers from the export location (mozilla/dom/bluetooth)
* Rename BluetoothPairingRequestListeningHandle to BluetoothPairingListener

Comment 23

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

Attachment: [Final] Bug 1036228 - Patch3: Handle pairing requests and fire pairing events in BluetoothAdapter. r=btian, r=bz

* include exported headers from the export location (mozilla/dom/bluetooth)
* Rename BluetoothPairingRequestListeningHandle to BluetoothPairingListener

Comment 24

[Ben Tian (inactive)]

(In reply to jocelyn [:jocelyn] from comment #23)
> * Rename BluetoothPairingRequestListeningHandle to BluetoothPairingListener

Please also revise wiki page once the renaming is confirmed.

Comment 25

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/0a47f1b5ca78
https://hg.mozilla.org/integration/b2g-inbound/rev/b55ab09e6173
https://hg.mozilla.org/integration/b2g-inbound/rev/8686f69017f2

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a47f1b5ca78
https://hg.mozilla.org/mozilla-central/rev/b55ab09e6173
https://hg.mozilla.org/mozilla-central/rev/8686f69017f2