[bluedroid] Support GetPairedDevice

RESOLVED FIXED in Firefox 28

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

Tracking

unspecified
1.3 Sprint 4 - 11/8
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Summary: [bluedroid] GetPairedDevice → [bluedroid] Support GetPairedDevice
Created attachment 827386 [details] [diff] [review]
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
Attachment #827386 - Flags: review?(echou)
Attachment #827386 - Attachment is obsolete: true
Attachment #827386 - Flags: review?(echou)
Created attachment 827752 [details] [diff] [review]
Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal
Attachment #827752 - Flags: review?(echou)
Created attachment 827992 [details] [diff] [review]
Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties
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.
Attachment #827992 - Attachment is obsolete: true
Created attachment 827995 [details] [diff] [review]
Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties
Attachment #827752 - Attachment description: Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal → Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal
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.
Attachment #827995 - Flags: review+
Attachment #827995 - Attachment is obsolete: true
Created attachment 828534 [details] [diff] [review]
Bug 932770 - Patch 2: Add dummy GetConnectedDeviceProperties, r=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.
Attachment #827752 - Flags: review?(echou) → review-
Attachment #827752 - Attachment is obsolete: true
Created attachment 828563 [details] [diff] [review]
Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal
Attachment #828563 - Flags: review?(echou)
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Attachment #828534 - Attachment is obsolete: true
Attachment #828563 - Attachment is obsolete: true
Attachment #828563 - Flags: review?(echou)
Created attachment 829199 [details] [diff] [review]
Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal
Attachment #829199 - Flags: review?(echou)
Attachment #829199 - Attachment description: dom/bluetooth/BluetoothCommon.h.rej → Bug 932770 - [bluedroid] Implement GetPairedDeviceInternaldom/bluetooth/BluetoothCommon.h.rej
Attachment #829199 - Attachment description: Bug 932770 - [bluedroid] Implement GetPairedDeviceInternaldom/bluetooth/BluetoothCommon.h.rej → Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal
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.
Attachment #829199 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/97bdec896bec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox28: --- → fixed
You need to log in before you can comment on or make changes to this bug.