Closed Bug 1038632 Opened 9 years ago Closed 9 years ago

Maintain a device array in BluetoothAdapter

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S1 (1aug)

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

(Whiteboard: webbt-api)

Attachments

(2 files, 6 obsolete files)

[Final] Bug 1038632 - Patch1: Remove unused API in BluetoothAdapter.webidl for new WebBluetooth API. f=btian, r=bz
1.27 KB, patch
Details | Diff | Splinter Review
[Final] Bug 1038632 - Patch2: Maintain device objects in BluetoothAdapter. r=btian
35.05 KB, patch
Details | Diff | Splinter Review 
We need to maintain a device array(paired+discovered) in BluetoothAdapter for passing device to BluetoothPairingRequestListeningHandle and BluetoothDiscoveryHandle.
|GetPairedDevices| in BluetoothAdapter will also fetch from this array.
* Remove unused API in BluetoothAdapter.webidl
* Maintain device array(paired+discovered devices) in BluetoothAdapter
Attachment #8458523 - Flags: feedback?(btian)
Attachment #8458524 - Flags: review?(btian)
Comment on attachment 8458523 [details] [diff] [review]
Bug 1038632 - Patch1: Remove unused API in BluetoothAdapter.webidl for new WebBluetooth API.

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

LGTM.
Attachment #8458523 - Flags: feedback?(btian) → feedback+
Comment on attachment 8458523 [details] [diff] [review]
Bug 1038632 - Patch1: Remove unused API in BluetoothAdapter.webidl for new WebBluetooth API.

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

::: dom/webidl/BluetoothAdapter2.webidl
@@ -71,2 @@
>    // Fired when pairing process is completed
>             attribute EventHandler   onpairedstatuschanged;

Forgot to mention: please confirm whether we need this event handler anymore.
Comment on attachment 8458524 [details] [diff] [review]
Bug 1038632 - Patch2: Maintain device objects in BluetoothAdapter.

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

Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +48,5 @@
> + */
> +class BluetoothDeviceComparator
> +{
> +public:
> +  bool Equals(const BluetoothDevice* a, const BluetoothDevice* b) const

Use |aDeviceA| and |aDeviceB| to be clearer.

@@ +64,3 @@
>  class StartDiscoveryTask : public BluetoothReplyRunnable
>  {
> +public:

nice catch!

@@ +117,2 @@
>      BluetoothReplyRunnable(aReq),
>      mAdapterPtr(aAdapterPtr)

nit: conform to other task ctors.
  GetDevicesTask(BluetoothAdapter* aAdapterPtr, nsIDOMDOMRequest* aReq)
  : BluetoothReplyRunnable(aReq)
  , mAdapterPtr(aAdapterPtr)

@@ +251,5 @@
>  }
>  
>  void
> +BluetoothAdapter::GetPairedDeviceProperties(
> +    const nsTArray<nsString>& aDeviceAddresses)

nit: two-space indent.

@@ +262,4 @@
>  
> +  nsRefPtr<BluetoothVoidReplyRunnable> results =
> +    new BluetoothVoidReplyRunnable(nullptr, nullptr,
> +        NS_LITERAL_STRING("GetPairedDeviceProperties"));

Only |new BluetoothVoidReplyRunnable(nullptr)| is required since the method name parameter is only used when aPromise is not nullptr.

@@ +302,3 @@
>    } else if (name.EqualsLiteral("UUIDs")) {
>      mUuids = value.get_ArrayOfnsString();
> +  } else if (name.EqualsLiteral("PairedDevices")) {

Is the event fired from backend when bluetooth is just turned on?

@@ +302,5 @@
>    } else if (name.EqualsLiteral("UUIDs")) {
>      mUuids = value.get_ArrayOfnsString();
> +  } else if (name.EqualsLiteral("PairedDevices")) {
> +    nsTArray<nsString> deviceAddresses;
> +    deviceAddresses = value.get_ArrayOfnsString();

Name |pairedDeviceAddresses| is clearer.

nit: how about wrap them into one line?

@@ +307,2 @@
>  
> +    for (size_t i = 0; i < deviceAddresses.Length(); i++) {

|uint32_t i = 0| to conform with other loops.

@@ +310,5 @@
> +      BT_APPEND_NAMED_VALUE(props, "Address", deviceAddresses[i]);
> +      BT_APPEND_NAMED_VALUE(props, "Paired", true);
> +
> +      // Create paired device with address and paired attributes
> +      nsRefPtr<BluetoothDevice> device =

nit: I think |pairedDevice| is clearer.

@@ +313,5 @@
> +      // Create paired device with address and paired attributes
> +      nsRefPtr<BluetoothDevice> device =
> +        BluetoothDevice::Create(GetOwner(), BluetoothValue(props));
> +
> +      // Save into adapter's device array if the device hasn't been created

nit: 'append to' is clearer.

@@ +352,5 @@
> +  } else if (aData.name().EqualsLiteral("DeviceFound")) {
> +    /*
> +     * DeviceFound signal will be distributed to all existing adapters while
> +     * doing discovery operations.
> +     * The signal need to be handled only if this adapter is holding a valid

nit: needs

@@ +436,5 @@
>  
>    BT_API2_LOGR("aStart %d", aStart);
>    nsresult rv;
>    if (aStart) {
> +    // Clear unpaired devices before start discovery

Move this section to |startDiscovery|. |startDiscovery| and |stopDiscovery| are separated in the latest codebase.

@@ +437,5 @@
>    BT_API2_LOGR("aStart %d", aStart);
>    nsresult rv;
>    if (aStart) {
> +    // Clear unpaired devices before start discovery
> +    for (size_t i = 0; i < mDevices.Length(); i++) {

|uint32_t i = 0| to conform with other loops.

@@ +869,5 @@
> +                                  0, /* aStart */
> +                                  BluetoothDeviceComparator());
> +
> +  if (index == mDevices.NoIndex) {
> +    // New device, save it into adapter's device array

nit: 'append it to' is clearer.

@@ +873,5 @@
> +    // New device, save it into adapter's device array
> +    mDevices.AppendElement(discoveredDevice);
> +    init.mDevice = discoveredDevice;
> +  } else {
> +    // Existed device

nit: Existing

@@ +878,5 @@
> +    init.mDevice = mDevices[index];
> +  }
> +
> +  // Report discovered device to Gaia
> +  nsRefPtr<BluetoothDeviceEvent> event =

I suggest BluetoothAdapter only pass BluetoothDevice into BluetoothDiscoveryHandle and leave rest of the work inside BluetoothDiscoveryHandle, since BluetoothDiscoveryHandle is the interface that really fires event.

::: dom/bluetooth2/BluetoothAdapter.h
@@ -84,4 @@
>    }
>  
>    uint32_t
>    DiscoverableTimeout() const

Remove this function as well.

@@ +191,5 @@
> +   * @param aValue [in] Properties array of the found device
> +   */
> +  void DispatchDeviceEvent(const BluetoothValue& aValue);
> +
> +  nsTArray<nsRefPtr<BluetoothDevice>> mDevices;

Please leave comment to explain when |mDevices| is constructed and cleaned.

nit: space between two '>'.
  nsTArray<nsRefPtr<BluetoothDevice> > mDevices;

@@ -213,1 @@
>    uint32_t mDiscoverableTimeout;

Remove discoverable timeout as well.

@@ -216,2 @@
>    nsTArray<nsString> mUuids;
>    bool mIsRooted;

Remove |mUuids| and |mIsRooted| since they are useless.

::: dom/bluetooth2/BluetoothDiscoveryHandle.cpp
@@ -41,5 @@
> -  NS_ENSURE_TRUE_VOID(bs);
> -
> -  if (aStart) {
> -    bs->RegisterBluetoothSignalHandler(
> -      NS_LITERAL_STRING(KEY_DISCOVERY_HANDLE), this);

Remove KEY_DISCOVERY_HANDLE in BluetoothCommon.h

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +330,5 @@
>        bt_bdaddr_t* deviceBdAddressTypes = (bt_bdaddr_t*)p.val;
>        int numOfAddresses = p.len / BLUETOOTH_ADDRESS_BYTES;
>        BT_LOGD("Adapter property: BONDED_DEVICES. Count: %d", numOfAddresses);
>  
> +      nsTArray<nsString> deviceAddresses;

Name |pairedDeviceAddresses| is clearer.

@@ +681,2 @@
>  
> +  // Update paired attribute to BluetoothDevice

nit: // Update attribute BluetoothDevice.paired

@@ +693,5 @@
> +  // Update bonding status to BluetoothAdapter
> +  BT_APPEND_NAMED_VALUE(props, "address", remoteBdAddress);
> +  BT_APPEND_NAMED_VALUE(props, "status", bonded);
> +
> +  BluetoothSignal adapterSignal(NS_LITERAL_STRING(PAIRED_STATUS_CHANGED_ID),

We should remove |onpairingstatuschanged| from BluetoothAdapter so this firing is needless.

@@ +1088,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  int requestedDeviceCount = aDeviceAddress.Length();
> +  NS_ENSURE_TRUE(requestedDeviceCount > 0, NS_OK);

|aRunnable| should be replied.
* Address review comments
Attachment #8458524 - Attachment is obsolete: true
Attachment #8458524 - Flags: review?(btian)
Attachment #8460049 - Flags: review?(btian)
Thanks for your review comments, most of them are addressed in v2.
Please see my reply below for some of the items.

(In reply to Ben Tian [:btian] from comment #5)
> Comment on attachment 8458524 [details] [diff] [review]
> Bug 1038632 - Patch2: Maintain device objects in BluetoothAdapter.
> 
> Review of attachment 8458524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment below.
> 

> 
> @@ +302,3 @@
> >    } else if (name.EqualsLiteral("UUIDs")) {
> >      mUuids = value.get_ArrayOfnsString();
> > +  } else if (name.EqualsLiteral("PairedDevices")) {
> 
> Is the event fired from backend when bluetooth is just turned on?
> 
Yes.

> @@ +693,5 @@
> > +  // Update bonding status to BluetoothAdapter
> > +  BT_APPEND_NAMED_VALUE(props, "address", remoteBdAddress);
> > +  BT_APPEND_NAMED_VALUE(props, "status", bonded);
> > +
> > +  BluetoothSignal adapterSignal(NS_LITERAL_STRING(PAIRED_STATUS_CHANGED_ID),
> 
> We should remove |onpairingstatuschanged| from BluetoothAdapter so this
> firing is needless.
> 

We still need device address and bond status for firing ondevicepaired and ondeviceunpaired.
Current PAIRED_STATUS_CHANGED_ID signal can cover them, and the implementation of these event handlers in adapter hasn't started, so I left this unchanged for now.
Would you suggest we should divide into two signals like DevicePaired and DeviceUnpaired now?

Thanks,
Jocelyn
(In reply to jocelyn [:jocelyn] from comment #7) 
> We still need device address and bond status for firing ondevicepaired and
> ondeviceunpaired.
> Current PAIRED_STATUS_CHANGED_ID signal can cover them, and the
> implementation of these event handlers in adapter hasn't started, so I left
> this unchanged for now.
> Would you suggest we should divide into two signals like DevicePaired and
> DeviceUnpaired now?

That's fine. Let's keep |onpairedstatuschanged| until |devicepaired| and |deviceunpaired| are implemented.
Comment on attachment 8460049 [details] [diff] [review]
Bug 1038632 - Patch2(v2): Maintain device objects in BluetoothAdapter.

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

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +42,5 @@
>  NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, DOMEventTargetHelper)
>  
> +/*
> + * A comparator that does the comparison of BluetoothDevice instances.
> + * Two BluetoothDevices are equivalent if they have an identical address.

nit: I think 'an' is redundant since a device contains only one address.

@@ +255,3 @@
>  {
> +  BluetoothService* bs = BluetoothService::Get();
> +  if (!bs) {

Use NS_ENSURE_TRUE_VOID.

@@ +263,5 @@
> +    new BluetoothVoidReplyRunnable(nullptr);
> +
> +  nsresult rv =
> +    bs->GetPairedDevicePropertiesInternal(aDeviceAddresses, results);
> +

nit: remove this newline.

@@ +431,5 @@
>  
>    BT_API2_LOGR();
>  
> +  // Clear unpaired devices before start discovery
> +  for (uint32_t i = 0; i < mDevices.Length(); i++) {

|mDevices.Length| and element indexes change as you remove elements from |mDevices|. These change result in unexpected behaviour. For example, when an array [A, B, C] removes A and becomes [B, C], B becomes unchecked since index increases from 0 to 1.

@@ +885,5 @@
>  
>  void
> +BluetoothAdapter::HandleDeviceFound(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);

Add MOZ_ASSERT(mDiscoveryHandleInUse);

@@ +887,5 @@
> +BluetoothAdapter::HandleDeviceFound(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);
> +
> +  // Create a temporary discovered BluetoothDevice for existence checking

nit: to check existence

@@ +903,5 @@
> +    // Existing device, discard temporary discovered device
> +    discoveredDevice = mDevices[index];
> +  }
> +
> +  // Use discovery handle to notify gaia the discovered device

I suggest to comment as 'Notify application of discovered device via discovery handle'.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +35,5 @@
>  {
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>  
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothAdapter, DOMEventTargetHelper)

nit: this line exceeds 80 characters.

@@ +181,5 @@
> +
> +  void HandleDeviceFound(const BluetoothValue& aValue);
> +
> +  /**
> +   * mDevices hold references of all created device objects.

nit: holds

@@ +185,5 @@
> +   * mDevices hold references of all created device objects.
> +   * It is an empty array when the adapter state is disabled.
> +   *
> +   * Devices will be appended when
> +   * 1) Enabling BT: Paired devices reported by stack.

Do stack report paired devices when adapter state is Enabling or Enabled?

@@ +187,5 @@
> +   *
> +   * Devices will be appended when
> +   * 1) Enabling BT: Paired devices reported by stack.
> +   * 2) Discovering: Discovered devices during discovery operation.
> +   * A device won't be appended if there is a device object with the

nit: ... if a device object with the same address already exists.

@@ +191,5 @@
> +   * A device won't be appended if there is a device object with the
> +   * same address existed.
> +   *
> +   * Devices will be removed when
> +   * 1) Stopping discovery: All unpaired devices will be removed.

Unpaired devices are removed before this adapter starts a new discovery, not stopping discovery.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1086,1 @@
>  

nit: remove this newline.

@@ +1089,4 @@
>      return NS_OK;
>    }
>  
> +  ENSURE_BLUETOOTH_IS_READY(aRunnable, NS_OK);

Why move this after |requestedDeviceCount| check?
Attachment #8460049 - Flags: review?(btian)
Comments will be addressed by updated patch shortly, thanks.
Please see my comments below for my answer of your questions.

(In reply to Ben Tian [:btian] from comment #9)
> Comment on attachment 8460049 [details] [diff] [review]
> Bug 1038632 - Patch2(v2): Maintain device objects in BluetoothAdapter.
> 
> Review of attachment 8460049 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +185,5 @@
> > +   * mDevices hold references of all created device objects.
> > +   * It is an empty array when the adapter state is disabled.
> > +   *
> > +   * Devices will be appended when
> > +   * 1) Enabling BT: Paired devices reported by stack.
> 
> Do stack report paired devices when adapter state is Enabling or Enabled?
> 
Enabling.

> @@ +191,5 @@
> > +   * A device won't be appended if there is a device object with the
> > +   * same address existed.
> > +   *
> > +   * Devices will be removed when
> > +   * 1) Stopping discovery: All unpaired devices will be removed.
> 
> Unpaired devices are removed before this adapter starts a new discovery, not
> stopping discovery.
> 
I wrote a wrong comment here, thanks!
* Address above review comments
Attachment #8460049 - Attachment is obsolete: true
Attachment #8461407 - Flags: review?(btian)
Comment on attachment 8461407 [details] [diff] [review]
Bug 1038632 - Patch2(v3): Maintain device objects in BluetoothAdapter.

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

r=me with comment addressed. Thanks.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +428,5 @@
>  
> +  // Clear unpaired devices before start discovery
> +  uint32_t i = mDevices.Length();
> +  while (i > 0) {
> +    --i;

Make it a for-loop.

 for (uint32_t i = mDevices.Length() - 1; i >= 0; i--) {
   ...
 }

@@ +792,5 @@
>  
>  void
> +BluetoothAdapter::HandleDeviceFound(const BluetoothValue& aValue)
> +{
> +  MOZ_ASSERT(mDiscoveryHandleInUse &&

I prefer to divide it into 2 MOZ_ASSERT to clearly know which condition fails.

@@ +811,5 @@
> +    // Existing device, discard temporary discovered device
> +    discoveredDevice = mDevices[index];
> +  }
> +
> +  // Nofity application of discovered device via discovery handle

typo: Notify

::: dom/bluetooth2/BluetoothAdapter.h
@@ +180,5 @@
> +   * A device won't be appended if a device object with the same
> +   * address already exists.
> +   *
> +   * Devices will be removed when
> +   * 1) Starting discovery: All unpaired devices will be removed before we

Replace 'we start' with 'this adapter starts' since devices are not removed when another adapter starts discovery.
Attachment #8461407 - Flags: review?(btian) → review+
Hi Boris,

This patch removed some APIs that are no longer be used in our new WebBluetooth API.
Could you help to review this patch?

Thanks,
Jocelyn
Attachment #8458523 - Attachment is obsolete: true
Attachment #8462297 - Flags: review?(bzbarsky)
Comments are addressed.
Thanks for your time, Ben.
Attachment #8461407 - Attachment is obsolete: true
Comment on attachment 8462297 [details] [diff] [review]
Bug 1038632 - Patch1: Remove unused API in BluetoothAdapter.webidl for new WebBluetooth API. f=btian

r=me

Is there C++ implementation to remove too?
Attachment #8462297 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #15)
> Comment on attachment 8462297 [details] [diff] [review]
> Bug 1038632 - Patch1: Remove unused API in BluetoothAdapter.webidl for new
> WebBluetooth API. f=btian
> 
> r=me
> 
> Is there C++ implementation to remove too?

Yes.
GetDevices, GetUuids, and IMPL_EVENT_HANDLER(devicefound) in BluetoothAdapter.h/BluetoothAdapter.cpp are removed by attachment 8462299 [details] [diff] [review].
Cleanup of original mDevices and mUuids are also done by it.
But the main purpose of that patch is maintaining a internal device array in class BluetoothAdapter and change some of our implementations based on the array.

Please let me know if you have any concerns, I will marked checkin-needed for these two patches after that.
Flags: needinfo?(bzbarsky)
> Please let me know if you have any concerns

Nope.  Just making sure we don't leave dead code hanging around.  Good to hear we're not.
Flags: needinfo?(bzbarsky)
Keywords: checkin-needed
Cancel checkin-needed for rebasing
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: --- → 2.0 S6 (18july)
Sorry, I set the wrong date...
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
You need to log in before you can comment on or make changes to this bug.