Closed
Bug 932770
Opened 11 years ago
Closed 11 years ago
[bluedroid] Support GetPairedDevice
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(blocking-b2g:1.3+, firefox28 fixed)
Tracking | Status | |
---|---|---|
firefox28 | --- | fixed |
People
(Reporter: shawnjohnjr, Assigned: shawnjohnjr)
References
Details
Attachments
(1 file, 6 obsolete files)
10.72 KB,
patch
|
echou
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Updated•11 years ago
|
Summary: [bluedroid] GetPairedDevice → [bluedroid] Support GetPairedDevice
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #827386 -
Attachment is obsolete: true
Attachment #827386 -
Flags: review?(echou)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #827752 -
Flags: review?(echou)
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #827992 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #827752 -
Attachment description: Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal → Bug 932770 - Patch 1: [bluedroid] Implement GetPairedDeviceInternal
Comment 6•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Attachment #827995 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
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-
Assignee | ||
Updated•11 years ago
|
Attachment #827752 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #828563 -
Flags: review?(echou)
Updated•11 years ago
|
blocking-b2g: --- → 1.3+
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Assignee | ||
Updated•11 years ago
|
Attachment #828534 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #828563 -
Attachment is obsolete: true
Attachment #828563 -
Flags: review?(echou)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #829199 -
Flags: review?(echou)
Assignee | ||
Updated•11 years ago
|
Attachment #829199 -
Attachment description: dom/bluetooth/BluetoothCommon.h.rej → Bug 932770 - [bluedroid] Implement GetPairedDeviceInternaldom/bluetooth/BluetoothCommon.h.rej
Assignee | ||
Updated•11 years ago
|
Attachment #829199 -
Attachment description: Bug 932770 - [bluedroid] Implement GetPairedDeviceInternaldom/bluetooth/BluetoothCommon.h.rej → Bug 932770 - [bluedroid] Implement GetPairedDeviceInternal
Comment 11•11 years ago
|
||
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+
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•