1036234 - Implement pairing in BluetoothAdapter (event handlers)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

* Pairing: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#pair.28DOMString_aDeviceAddress.29

Attributes:
  [AvailableIn=CertifiedApps]
  readonly attribute BluetoothPairingRequestListeningHandle pairingReqs; 
Event handlers:
  attribute EventHandler ondevicepaired;
  attribute EventHandler ondeviceunpaired;

Comment 1

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

Following part has been moved to Bug 1036228.

Attributes:
  [AvailableIn=CertifiedApps]
  readonly attribute BluetoothPairingRequestListeningHandle pairingReqs;

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v0)

Comment 3

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v1)

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8465216 [details] [diff] [review]
Implement pairing event handlers of BluetoothAdapter. (v1)

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

Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +837,5 @@
>    DispatchTrustedEvent(event);
>  }
>  
> +void
> +BluetoothAdapter::DispatchDevicePairedUnpairedEvent(const BluetoothValue& aValue)

I suggest to divert ondevicepaired and ondeviceunpaired to two functions. You can refer to BluetoothManager firing onadapteradded and onadapterremoved. Also modify bluetooth signal firing in |BluetoothServiceBluedroid|.

@@ +856,5 @@
> +      isBonded = value.get_bool();
> +    }
> +  }
> +
> +  if (mDevices.Length() == 0) {

mDevices.IsEmpty()

@@ +872,5 @@
> +    }
> +  }
> +
> +  BluetoothDeviceEventInit init;
> +  nsRefPtr<BluetoothDeviceEvent> event = nullptr;

I suggest we just fire address only for unpaired devices but keep them in |mDevices| array. Our original design was to remove BluetoothDevice once it's unpaired, but now we keep not only paired devices but also discovered devices in |mDevices| array. Let me know if you have other idea.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +149,5 @@
>      SendMediaPlayStatus(const MediaPlayStatus& aMediaPlayStatus, ErrorResult& aRv);
>  
>    IMPL_EVENT_HANDLER(a2dpstatuschanged);
>    IMPL_EVENT_HANDLER(hfpstatuschanged);
>    IMPL_EVENT_HANDLER(pairedstatuschanged);

Remove this event handler.

@@ +187,5 @@
>  
>    /**
> +   * Fire BluetoothDeviceEvent to notify adapter the changed bonding status.
> +   *
> +   * When a remote device was paired, the event would be fired to 'ondevicepaired'

nit: 'is' paired

@@ +188,5 @@
>    /**
> +   * Fire BluetoothDeviceEvent to notify adapter the changed bonding status.
> +   *
> +   * When a remote device was paired, the event would be fired to 'ondevicepaired'
> +   * with with property device as the paired remote bluetooth device.

nit: extra 'with'

::: dom/webidl/BluetoothAdapter2.webidl
@@ +83,5 @@
> +           attribute EventHandler   ondevicepaired;
> +
> +  // Fired when a remote device gets unpaired from the adapter.
> +           attribute EventHandler   ondeviceunpaired;
> +

Remove |onpairedstatuschanged|.

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v2)

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

Thank you for your comments.
I've uploaded a new patch based on you comments.

(In reply to Ben Tian [:btian] from comment #4)
> Comment on attachment 8465216 [details] [diff] [review]

> @@ +872,5 @@
> > +    }
> > +  }
> > +
> > +  BluetoothDeviceEventInit init;
> > +  nsRefPtr<BluetoothDeviceEvent> event = nullptr;
> 
> I suggest we just fire address only for unpaired devices but keep them in
> |mDevices| array. Our original design was to remove BluetoothDevice once
> it's unpaired, but now we keep not only paired devices but also discovered
> devices in |mDevices| array. Let me know if you have other idea.

It's a great topic to discuss.
I prefer to remove unpaired device from cached array since it might not close enough to local device for any Bluetooth communication. 
Chrome OS provide a similar API called getDevices(). It returns a device array which contains paired and recently discovered devices.
I think it would be easier to fulfill the requirement for regular UI of Bluetooth setting app since we usually don't need to display unpaired and unreachable device.

Of course, it's always open for further discussion.
Thanks.

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8465988 [details] [diff] [review]
Implement pairing event handlers of BluetoothAdapter. (v2)

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

Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +844,5 @@
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  nsString deviceAddress;
> +  const InfallibleTArray<BluetoothNamedValue>& values = aValue.get_ArrayOfBluetoothNamedValue();

nit: this line is too long.

@@ +853,5 @@
> +      deviceAddress = value.get_nsString();
> +    }
> +  }
> +
> +  if (mDevices.IsEmpty()) {

If |mDevices| is empty, we should create and append the paired device into array.

@@ +861,5 @@
> +  }
> +
> +  nsString address;
> +  uint32_t index = 0;
> +  for (index = 0; index < mDevices.Length(); ++index) {

nit: for (uint32_t index = 0; ...

@@ +871,5 @@
> +
> +  BluetoothDeviceEventInit init;
> +  nsRefPtr<BluetoothDeviceEvent> event = nullptr;
> +
> +  if (index != mDevices.Length()) {

Revise the flow as:
- If device w/ |remoteBdAddress| is in |mDevices|, dispatch the device;
- Otherwise create and append a new device.

@@ +902,5 @@
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  nsString deviceAddress;
> +  const InfallibleTArray<BluetoothNamedValue>& values = aValue.get_ArrayOfBluetoothNamedValue();

Ditto.

@@ +929,5 @@
> +
> +  BluetoothDeviceEventInit init;
> +  nsRefPtr<BluetoothDeviceEvent> event = nullptr;
> +
> +  if (index != mDevices.Length()) {

index < mDevices.Length()

::: dom/bluetooth2/BluetoothAdapter.h
@@ +190,5 @@
> +   * When a remote device is paired, the event would be fired to 'ondevicepaired'
> +   * with property device as the paired remote bluetooth device.
> +   *
> +   * @param  aValue A BluetoothValue which represents a TArrayOfBluetoothNamedValue.
> +   *                The array should contain these two properties.

should contain 'the property:'

@@ +193,5 @@
> +   * @param  aValue A BluetoothValue which represents a TArrayOfBluetoothNamedValue.
> +   *                The array should contain these two properties.
> +   *                - nsString  'address'
> +   */
> +  void DispatchDevicePairedEvent(const BluetoothValue& aValue);

I suggest to rename it |HandleDevicePaired| and |HandleDeviceUnpaired|. Also add a function |DispatchDeviceEvent| that only dispatches BluetoothDeviceEvent. Refer to adapterAdded and adapterRemoved events in BluetoothManager.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +766,2 @@
>                                  NS_LITERAL_STRING(KEY_ADAPTER),
>                                  BluetoothValue(props));

Wrap nsstring |remoteBdAddress| directly instead of array in bluetooth signal.

Comment 8

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #7)
> @@ +871,5 @@
> > +
> > +  BluetoothDeviceEventInit init;
> > +  nsRefPtr<BluetoothDeviceEvent> event = nullptr;
> > +
> > +  if (index != mDevices.Length()) {
> 
> Revise the flow as:
> - If device w/ |remoteBdAddress| is in |mDevices|, dispatch the device;
> - Otherwise create and append a new device.

You can refer to |HandleDeviceFound| to find device in |mDevices| array with comparator. In that way you have to 1) keep bluetooth signal below a named-value array and 2) wrap both |remoteBdaddress| and |bonded| in bluetooth signal, in order to create device from bluetooth value carried in the bluetooth signal.

> ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +766,2 @@
> >                                  NS_LITERAL_STRING(KEY_ADAPTER),
> >                                  BluetoothValue(props));
> 
> Wrap nsstring |remoteBdAddress| directly instead of array in bluetooth
> signal.

Ignore this suggestion if you follow |HandleDeviceFound|.

Comment 9

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v3)

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #8)
> (In reply to Ben Tian [:btian] from comment #7)
> > @@ +871,5 @@
> > > +
> > > +  BluetoothDeviceEventInit init;
> > > +  nsRefPtr<BluetoothDeviceEvent> event = nullptr;
> > > +
> > > +  if (index != mDevices.Length()) {
> > 
> > Revise the flow as:
> > - If device w/ |remoteBdAddress| is in |mDevices|, dispatch the device;
> > - Otherwise create and append a new device.
> 
> You can refer to |HandleDeviceFound| to find device in |mDevices| array with
> comparator. In that way you have to 1) keep bluetooth signal below a
> named-value array and 2) wrap both |remoteBdaddress| and |bonded| in
> bluetooth signal, in order to create device from bluetooth value carried in
> the bluetooth signal.
> 
> > ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> > @@ +766,2 @@
> > >                                  NS_LITERAL_STRING(KEY_ADAPTER),
> > >                                  BluetoothValue(props));
> > 
> > Wrap nsstring |remoteBdAddress| directly instead of array in bluetooth
> > signal.
> 
> Ignore this suggestion if you follow |HandleDeviceFound|.

Thank you for your comments.
I uploaded a new patch based on your suggestion.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8469149 [details] [diff] [review]
Implement pairing event handlers of BluetoothAdapter. (v3)

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

Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +919,5 @@
> +BluetoothAdapter::HandleDevicePaired(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  if (mState != BluetoothAdapterState::Enabled) {

Use NS_ENSURE_TRUE_VOID(mState == BluetoothAdapterState::Enabled) to show warning if condition fails.

@@ +933,5 @@
> +                                  BluetoothDeviceComparator());
> +
> +  BluetoothDeviceEventInit init;
> +  if (index != mDevices.NoIndex) {
> +    if (!mDevices[index]->Paired()) {

Do not update 'paired' property here since 'PropertyChanged' signal of device does.

@@ +953,5 @@
> +BluetoothAdapter::HandleDeviceUnpaired(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  if (mState != BluetoothAdapterState::Enabled) {

Ditto.

@@ +957,5 @@
> +  if (mState != BluetoothAdapterState::Enabled) {
> +    return;
> +  }
> +
> +  // Create paired device with address and paired attributes

unpaired device

@@ +965,5 @@
> +  size_t index = mDevices.IndexOf(unpairedDevice,
> +                                  0, /* aStart */
> +                                  BluetoothDeviceComparator());
> +  nsString deviceAddress;
> +  if (index != mDevices.NoIndex) {

Extract device address from |unpairedDevice| instead of parsing bluetooth named-value array again.

  nsString deviceAddress;
  if (index != mDevices.NoIndex) {
    mDevices.RemoveElementAt(index);
  }
  unpairedDevice->GetAddress(deviceAddress);

::: dom/bluetooth2/BluetoothAdapter.h
@@ +186,5 @@
>  
>    /**
> +   * Handle DEVICE_PAIRED_ID bluetooth signal.
> +   *
> +   * When a remote device is paired, the event would be fired to 'ondevicepaired'

Remove this section since it's for device event of ondevicepaired, not |HandleDevicePaired| function.

@@ +190,5 @@
> +   * When a remote device is paired, the event would be fired to 'ondevicepaired'
> +   * with property device as the paired remote bluetooth device.
> +   *
> +   * @param  aValue A BluetoothValue which represents a TArrayOfBluetoothNamedValue.
> +   *                The array should contain these two properties.

nit: contain these two properties':' Also make the comment format consistent to |DispatchDeviceEvent|.

  @param aValue [in] Properties array of the paired device. The array should contain two properties:

@@ +200,5 @@
> +
> +  /**
> +   * Handle DEVICE_UNPAIRED_ID bluetooth signal.
> +   *
> +   * When a remote device was unpaired, the event would be fired to

Ditto.

@@ +205,5 @@
> +   * 'ondeviceunpaired' with property address as the unpaired remote bluetooth
> +   * device's address.
> +   *
> +   * @param  aValue A BluetoothValue which represents a TArrayOfBluetoothNamedValue.
> +   *                The array should contain these two properties.

Ditto.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +754,5 @@
>    NS_DispatchToMainThread(new DistributeBluetoothSignalTask(deviceSignal));
>  
>    props.Clear();
>  
> +  // Append signal property and notify adapter.

nit: properties

@@ +761,4 @@
>  
> +  nsString signalName = bonded
> +                      ? NS_LITERAL_STRING(DEVICE_PAIRED_ID)
> +                      : NS_LITERAL_STRING(DEVICE_UNPAIRED_ID);

nit: make this two lines.

  nsString signalName = bonded ? NS_LITERAL_STRING(DEVICE_PAIRED_ID)
                               : NS_LITERAL_STRING(DEVICE_UNPAIRED_ID);

Comment 12

[Jamin Liu [:jaliu][:Jamin]]

Thank you for your comments.

(In reply to Ben Tian [:btian] from comment #11)
> Comment on attachment 8469149 [details] [diff] [review]
> Implement pairing event handlers of BluetoothAdapter. (v3)
> 
> Review of attachment 8469149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment below.
> 
> ::: dom/bluetooth2/BluetoothAdapter.cpp
> @@ +919,5 @@
> > +BluetoothAdapter::HandleDevicePaired(const BluetoothValue& aValue)
> > +{
> > +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> > +
> > +  if (mState != BluetoothAdapterState::Enabled) {
> 
> Use NS_ENSURE_TRUE_VOID(mState == BluetoothAdapterState::Enabled) to show
> warning if condition fails.

I prefer to add BT_WARNING to the scope of if statement, since NS_ENSURE_TRUE_VOID is marked as 'deprecated'.
http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h?from=nsdebug.h&case=true#276

I try to avoid function-style macro which contains 'return' statement unless it's necessary, since it don't obey the rules of function argument passing and has no language-level protection in C++.

I will uploaded a new patch and I'll be glad to revise it based on your feedback.
Thank you.

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v4)

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8469840 [details] [diff] [review]
Implement pairing event handlers of BluetoothAdapter. (v4)

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

r=me with comment addressed.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +907,5 @@
> +    BT_WARNING("HandleDevicePaired() is called when adapter isn't enabled.");
> +    return;
> +  }
> +
> +  // Create unpaired device with address and paired attributes

Create paired device with 'address' and 'paired' attributes

@@ +908,5 @@
> +    return;
> +  }
> +
> +  // Create unpaired device with address and paired attributes
> +  nsRefPtr<BluetoothDevice> unpairedDevice =

Revise to |pairedDevice| and all the following.

@@ +921,5 @@
> +    init.mDevice = mDevices[index];
> +  } else {
> +    mDevices.AppendElement(unpairedDevice);
> +    init.mDevice = unpairedDevice;
> +  }

A minor suggestion to rewrite here to make event init close to |DispatchDeviceEvent|.

  if (index != mDevices.NoIndex) {
    pairedDevice = mDevices[index];
  } else {
    mDevices.AppendElement(pairedDevice);
  }

  // Notify application of paired device
  BluetoothDeviceEventInit init;
  init.mDevice = pairedDevice.
  DispatchDeviceEvent(NS_LITERAL_STRING("devicepaired"), init);

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v5) r=btian

Thank Ben for the review.
* Revisee based on comment 14.
* Rebased.

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8469965 [details] [diff] [review]
Implement pairing event handlers of BluetoothAdapter. (v5) r=btian

Hi Blake,

This is Jamin from Bluetooth team.
Bluetooth team is working on API refinement and we plan to expose Bluetooth APIs v2 at B2G 2.2.
Bz has helped us a lot by reviewing the patches which modified webidl, including BT on/off, discovery and pairing event.

Since bz is taking a long vacation, I'm looking for a reviewer from DOM perspective.

In this bug, I revised BluetoothAdapter2.webidl[1] and implemented three pairing related methods.
- EventHandler ondevicepaired;
- EventHandler ondeviceunpaired;

The implementation patch #Attachment 8469965 [details] [diff] has already been reviewed by Ben, one of our Bluetooth module peers.
Would you mind to take a look and give me some feedback as a reviewer, or could you recommend someone else ?

Thank you. :)

Comment 17

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #16)
> In this bug, I revised BluetoothAdapter2.webidl[1] and implemented three
> pairing related methods.
> - EventHandler ondevicepaired;
> - EventHandler ondeviceunpaired;

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#Interfaces

Comment 18

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v5) r=btian, r=mrbkap

* Add reviewer's names to commit message.

Comment 19

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #18)
> Created attachment 8471282 [details] [diff] [review]

Thank Blake and Ben for reviewing the patch.

Comment 20

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/2d0ebc1bc7d6
https://hg.mozilla.org/integration/b2g-inbound/rev/c5d50f1bc995

Comment 21

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #20)
> https://hg.mozilla.org/integration/b2g-inbound/rev/2d0ebc1bc7d6
> https://hg.mozilla.org/integration/b2g-inbound/rev/c5d50f1bc995

Wrong links. Should be this one:
https://hg.mozilla.org/integration/b2g-inbound/rev/a471c7c73409

Comment 22

[Nigel Babu [:nigelb]]

This has caused build failures on b2g-inbound. Can you please look into it immediately?

Comment 23

[Nigel Babu [:nigelb]]

Backed for build bustage in https://hg.mozilla.org/integration/b2g-inbound/rev/11a544aadfaa

Bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=45716489&tree=B2g-Inbound

Comment 24

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Implement pairing event handlers of BluetoothAdapter. (v6) r=btian, r=mrbkap

* Added "onpairedstatuschanged' back to nsGkAtomList.h since it still necessary in bluetooth1.

Comment 25

[Jamin Liu [:jaliu][:Jamin]]

TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=4bfdde5c95b4

This patch wouldn't affect bluetooth1 and pass all Bluetooth tests on TBPL.
- TEST-PASS | test_dom_BluetoothManager_enabled.js | took 17954ms
- TEST-PASS | test_dom_BluetoothManager_adapteradded.js | took 8786ms
- TEST-PASS | test_dom_BluetoothAdapter_setters.js | took 8462ms
- TEST-PASS | test_dom_BluetoothAdapter_getters.js | took 7965ms
- TEST-PASS | test_dom_BluetoothAdapter_discovery.js | took 8642ms
- TEST-PASS | test_dom_BluetoothAdapter_pair.js | took 11693ms

Comment 26

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/4c8a9727660f

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c8a9727660f

Comment 28

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the follow-up errors from implementation of pairing event handlers.

Comment 29

[Ben Tian (inactive)]

Comment on attachment 8472345 [details] [diff] [review]
Fix the follow-up errors from implementation of pairing event handlers.

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

r=me. Please always ensure patches pass compilation before uploading for review.

Comment 30

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the follow-up errors from implementation of pairing event handlers. r=btian

* Add "r=btian" to commit message.

Comment 31

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/946772491ea2

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/946772491ea2

Comment 33

[Ben Tian (inactive)]

This bug is part of WebBluetooth API refinement (bug 1005848). Preliminary documentation is on wiki [1] and this bug implements comment 0.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2

Comment 34

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Mozilla/B2G_OS/API/BluetoothAdapter