Closed Bug 932770 Opened 11 years ago Closed 11 years ago

[bluedroid] Support GetPairedDevice

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

Attachments

(1 file, 6 obsolete files)

No description provided.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Summary: [bluedroid] GetPairedDevice → [bluedroid] Support GetPairedDevice
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)
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 #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+
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-
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Attachment #828563 - Attachment is obsolete: true
Attachment #828563 - 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+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: