932770 - [bluedroid] Support GetPairedDevice

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal

Tests had been executed:
1. Pair/Unpair new device
2. Unpair multiple paired device 
3. Request remote pairing request from the remote side
4. Pairing timeout

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Since gaia settings app requires to have GetConnectedDeviceProperties to set pairList.show(true), and other profiles are not yet ready to summit. Make a dummy GetConnectedDeviceProperties first.

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties

Comment 6

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

Comment on attachment 827995 [details] [diff] [review]
Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties

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

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +753,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  //FIXME: This will be implemented in later patches
> +  BluetoothValue values = InfallibleTArray<BluetoothNamedValue>();
> +  nsTArray<nsString> deviceAddresses;

nit: please remove these two unused variables.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties, r=echou

Comment 8

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

Comment on attachment 827752 [details] [diff] [review]
Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal

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

Hi Shawn, please see my comments below.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +30,5 @@
>  using namespace mozilla;
>  using namespace mozilla::ipc;
>  USING_BLUETOOTH_NAMESPACE
>  
> +#define BDADDRESS_LENGTH  6

nit: we usually put #define right after #include

@@ +201,5 @@
>  }
>  
> +static void
> +BdAddressTypeToStringByIndex(bt_bdaddr_t* aBdAddressType,
> +                             nsAString& aRetBdAddress, int aIndex)

See my comments below about the necessity of this function.

@@ +277,5 @@
> +      // We have to cache addresses of bonded devices. Unlike BlueZ,
> +      // bluedroid would not send an another BT_PROPERTY_ADAPTER_BONDED_DEVICES
> +      // event after bond completed
> +      bt_bdaddr_t* deviceBdAddressTypes = (bt_bdaddr_t*)p.val;
> +      int numOfAddress = p.len/BDADDRESS_LENGTH;

nit: please insert blanks around '/'.
nit: numOfAddress'es'

@@ +282,5 @@
> +      BT_LOGD("Adapter property: BONDED_DEVICES. Count: %d", numOfAddress);
> +      // Whenever reloading paired devices, force refresh
> +      sAdapterBondedAddressArray.Clear();
> +      sDeviceAddressArray.Clear();
> +      nsAutoString deviceBdAddress;

Since this variable is only used in the for-loop below, please declare inside.

@@ +284,5 @@
> +      sAdapterBondedAddressArray.Clear();
> +      sDeviceAddressArray.Clear();
> +      nsAutoString deviceBdAddress;
> +
> +      for (int index = 0, bdaLength = 6; index<numOfAddress; index++) {

You may want to use BDADDRESS_LENGTH instead of a new variable bdaLength. In addition, please insert blanks around '<'.

@@ +286,5 @@
> +      nsAutoString deviceBdAddress;
> +
> +      for (int index = 0, bdaLength = 6; index<numOfAddress; index++) {
> +        index = index * bdaLength;
> +        BdAddressTypeToStringByIndex(deviceBdAddressTypes, deviceBdAddress, index);

To avoid defining a new function which is mostly the same as BdAddressTypeToString(), I think we could reuse the original one:

  BdAddressTypeToString(deviceBdAddressTypes + index, deviceBdAddress);

@@ +373,5 @@
> +  BluetoothValue val = sPairedDeviceProperties;
> +  // Address will be the index, compose nested array
> +  sRemoteDevicesPack.get_ArrayOfBluetoothNamedValue().AppendElement(
> +      BluetoothNamedValue(sDeviceAddressArray[currentPos],
> +        val.get_ArrayOfBluetoothNamedValue()) );

Please fix indentation.

@@ +735,5 @@
>      BluetoothNamedValue(NS_LITERAL_STRING("Name"), sAdapterBdName));
>  
> +  v.get_ArrayOfBluetoothNamedValue().AppendElement(
> +    BluetoothNamedValue(NS_LITERAL_STRING("Devices"),
> +    sAdapterBondedAddressArray));

nit: weird indentation.

@@ +758,5 @@
>  BluetoothServiceBluedroid::GetPairedDevicePropertiesInternal(
>    const nsTArray<nsString>& aDeviceAddress, BluetoothReplyRunnable* aRunnable)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  sPairedDevicesNum = aDeviceAddress.Length();

This operation should be put after the if-statement, otherwise sPairedDevicesNum may be cleared to 0 unexpectedly.

@@ +768,2 @@
>  
> +  if (sBtInterface) {

Please follow other functions checking "if (!IsReady())" at the beginning, then this line wouldn't be needed.

@@ +771,5 @@
> +      // Retrieve all properties of devices
> +      bt_bdaddr_t addressType;
> +      StringToBdAddressType(aDeviceAddress[i], &addressType);
> +      int ret =
> +        sBtInterface->get_remote_device_properties(&addressType);

nit: one line should be fine.

@@ +774,5 @@
> +      int ret =
> +        sBtInterface->get_remote_device_properties(&addressType);
> +      if (ret != BT_STATUS_SUCCESS) {
> +        DispatchBluetoothReply(aRunnable, BluetoothValue(true),
> +            NS_LITERAL_STRING("GetPairedDeviceFailed"));

nit: weird indentation.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal

Comment 11

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

Comment on attachment 829199 [details] [diff] [review]
Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal

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

Looks good. I'll deal with the rest stuff.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +321,5 @@
> +  }
> +
> +  sRequestedDeviceCountArray[0]--;
> +
> +  BT_LOGR("%s:, still needs: %d", __FUNCTION__, sRequestedDeviceCountArray[0]);

nit: I believe this was not left intentionally. Please remove it.

@@ +363,5 @@
> +      BT_LOGR("It's a serious problem. No runnable to return.");
> +      return;
> +    }
> +
> +    BT_LOGR("Dispatch reply: Get paired device");

Ditto.

Comment 12

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

https://hg.mozilla.org/integration/b2g-inbound/rev/97bdec896bec

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97bdec896bec