Closed Bug 1036228 Opened 6 years ago Closed 5 years ago

Implement BluetoothPairingListener

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S1 (1aug)

People

(Reporter: ben.tian, Assigned: yrliou)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [webbt-api])

Attachments

(3 files, 10 obsolete files)

9.62 KB, patch
Details | Diff | Splinter Review
10.20 KB, patch
Details | Diff | Splinter Review
9.88 KB, patch
Details | Diff | Splinter Review
Assignee: nobody → joliu
* add BluetoothPairingRequestListeningHandle.webidl
* implement BluetoothPairingRequestListeningHandle.cpp/h
Depends on: 1038632
Depends on: 1033898
* Implement BluetoothPairingRequestListeningHandle
* Notify BluetoothAdapter for pairing requests in PinRequestCallback and
  SspRequestCallback.
Attachment #8453700 - Attachment is obsolete: true
* Add attribute BluetoothPairingRequestListeningHandle pairingReqs in BluetoothAdapter.
* Handle PairingRequest signals notified by bluedroid and fire PairingEvent for Gaia
Target Milestone: --- → 2.1 S1 (1aug)
* revise |DispatchPairingEvent| based on latest BluetoothPairingEvent implementation
* split service part into another patch
Attachment #8461371 - Attachment is obsolete: true
* handle pairing requests in bluedroid service
Attachment #8463812 - Attachment description: Bug 1036228 - Patch3: Handle pairing requests and fire pairing events in BluetoothAdapter. → [WIP] Bug 1036228 - Patch3: Handle pairing requests and fire pairing events in BluetoothAdapter.
Attachment #8463810 - Attachment description: [WIP] Bug 1036228 - Patch1: Add BluetoothPairingRequestListeningHandle.webidl and implement it → Bug 1036228 - Patch1: Add BluetoothPairingRequestListeningHandle.webidl and implement it
Attachment #8463810 - Flags: review?(btian)
Attachment #8463811 - Attachment description: [WIP] Bug 1036228 - Patch2: Handle pairing requests reported by bluedroid in bluetooth services. → Bug 1036228 - Patch2(v1): Handle pairing requests reported by bluedroid in bluetooth services.
Attachment #8463811 - Flags: review?(btian)
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
Attachment #8463812 - Attachment description: [WIP] Bug 1036228 - Patch3: Handle pairing requests and fire pairing events in BluetoothAdapter. → Bug 1036228 - Patch3: Handle pairing requests and fire pairing events in BluetoothAdapter.
Attachment #8463812 - Flags: review?(btian)
Attachment #8463812 - Attachment description: Bug 1036228 - Patch3: Handle pairing requests and fire pairing events in BluetoothAdapter. → Bug 1036228 - Patch3(v1): Handle pairing requests and fire pairing events in BluetoothAdapter.
Attachment #8463810 - Attachment description: Bug 1036228 - Patch1: Add BluetoothPairingRequestListeningHandle.webidl and implement it → Bug 1036228 - Patch1(v1): Add BluetoothPairingRequestListeningHandle.webidl and implement it
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.
Attachment #8463810 - Flags: review?(btian) → review+
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.
Attachment #8463811 - Flags: review?(btian)
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).
Attachment #8463812 - Flags: review?(btian) → review+
* address review comments
Attachment #8463811 - Attachment is obsolete: true
Attachment #8465210 - Flags: review?(btian)
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.
Attachment #8465210 - Flags: review?(btian)
Ah, sorry for misunderstanding and missing breaks.
Comments are addressed, please review it again.
Attachment #8465210 - Attachment is obsolete: true
Attachment #8465217 - Flags: review?(btian)
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.
Attachment #8465217 - Flags: review?(btian) → review+
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
Attachment #8465209 - Flags: review?(bzbarsky)
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.
Attachment #8465211 - Flags: review?(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.
Attachment #8465209 - Flags: review?(bzbarsky) → review+
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
Attachment #8465211 - Flags: review?(bzbarsky) → review+
* include exported headers from the export location (mozilla/dom/bluetooth)
* Rename BluetoothPairingRequestListeningHandle to BluetoothPairingListener
Attachment #8465209 - Attachment is obsolete: true
* include exported headers from the export location (mozilla/dom/bluetooth)
* Rename BluetoothPairingRequestListeningHandle to BluetoothPairingListener
Attachment #8465211 - Attachment is obsolete: true
Blocks: 1048776
Keywords: checkin-needed
Summary: Implement BluetoothPairingRequestListeningHandle → Implement BluetoothPairingListener
(In reply to jocelyn [:jocelyn] from comment #23)
> * Rename BluetoothPairingRequestListeningHandle to BluetoothPairingListener

Please also revise wiki page once the renaming is confirmed.
You need to log in before you can comment on or make changes to this bug.