Closed
Bug 1015819
Opened 10 years ago
Closed 10 years ago
[Bluetooth] Fetch UUID and Connect with Bluetooth headset Via nfc-out-of-band pairing
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 verified, b2g-v2.1 fixed)
People
(Reporter: shawnjohnjr, Assigned: shawnjohnjr)
References
Details
Attachments
(2 files, 15 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Via nfc-out-of-band pairing, Bluetooth headset icon is missing due to no CoD extracted from EIR. This is required to have check SDP records. Based on current implementation, BluetoothProfileController checks CoD and this leads NFC out-of-band pairing no chance to initialize connection.
Assignee | ||
Updated•10 years ago
|
Summary: [Bluetooth] Via nfc-out-of-band pairing, Bluetooth headset icon is missing due to no CoD → [Bluetooth] Fail to connect with Bluetooth headset Via nfc-out-of-band pairing
Assignee | ||
Updated•10 years ago
|
blocking-b2g: --- → 2.0?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → shuang
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•10 years ago
|
Blocks: b2g-NFC-2.0
Comment 1•10 years ago
|
||
Target to be fixed in sprint4 per discussion with Shawn.
Target Milestone: --- → 2.0 S4 (20june)
Assignee | ||
Comment 2•10 years ago
|
||
Note: This bug targets on fixing auto-connection to Bluetooth headsets with NFC tags. This is not for file sharing.
Assignee | ||
Updated•10 years ago
|
Summary: [Bluetooth] Fail to connect with Bluetooth headset Via nfc-out-of-band pairing → [Bluetooth] Connect with Bluetooth headset Via nfc-out-of-band pairing
Assignee | ||
Updated•10 years ago
|
Summary: [Bluetooth] Connect with Bluetooth headset Via nfc-out-of-band pairing → [Bluetooth] Fetch UUID and Connect with Bluetooth headset Via nfc-out-of-band pairing
Assignee | ||
Comment 3•10 years ago
|
||
I found sometimes CoD value got updated on bluedroid after BONDED. But even if CoD/Icon got update, the request from NFC application won't be completed due to initialized CoD is empty.
2678 06-12 06:21:44.409 I/GeckoBluetooth( 2882): BondStateChangedCallback: ==================== BONDED !!!!!!!!====================
2680 06-12 06:21:44.419 I/GeckoBluetooth( 2882): RemoteDevicePropertiesCallback: =============== BT_PROPERTY_UUIDS =================
2681 06-12 06:21:44.419 I/GeckoBluetooth( 2882): StartSession: No queued profile.
2682 06-12 06:21:44.419 I/GeckoBluetooth( 2882): EndSession: mSuccess 0
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
One thing I noticed, it looks like whenever NFC turns on bluetooth, discovery will be automatically performed, this is something not good also.
Assignee | ||
Comment 6•10 years ago
|
||
Opps. It looks like Class Of Device value from RemoteDevicePropertiesChanged callback is not reliable (which is from bluedroid) under NFC out-of-band pairing, because we don't see any EIR query here. We got invalid value here, which results both invalid CoD and Icon. However, the UUID lists are correct and reliable.
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
On devices used bluedroid stack, this patch works, but still need to work on bluez based.
Assignee | ||
Updated•10 years ago
|
Attachment #8439852 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8440597 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8440604 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8440611 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8439171 -
Attachment description: (WIP)Bug 1015819 - Part 1: Delay outgoing connection after bonded → Bug 1015819 - Part 1: Delay outgoing connection after bonded
Assignee | ||
Updated•10 years ago
|
Attachment #8440624 -
Attachment description: [WIP]Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records → Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
Assignee | ||
Updated•10 years ago
|
Attachment #8440624 -
Flags: review?(echou)
Attachment #8440624 -
Flags: feedback?(btian)
Comment 13•10 years ago
|
||
Comment on attachment 8440624 [details] [diff] [review]
Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
Review of attachment 8440624 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +484,4 @@
> BdAddressTypeToString(aBdAddress, remoteDeviceBdAddress);
> BT_APPEND_NAMED_VALUE(props, "Address", remoteDeviceBdAddress);
>
> + bool isCodInvalid = false;
I think |isCodValid| is easier to understand than |isCodeInvalid|.
@@ +508,5 @@
> + } else if (p.type == BT_PROPERTY_UUIDS) {
> + InfallibleTArray<nsString> uuidsArray;
> + int uuid_list_leng = p.len / MAX_UUID_SIZE;
> + uint32_t cod = 0;
> + for (int i=0; i < uuid_list_leng; i++) {
nit: space => |i = 0|
@@ +510,5 @@
> + int uuid_list_leng = p.len / MAX_UUID_SIZE;
> + uint32_t cod = 0;
> + for (int i=0; i < uuid_list_leng; i++) {
> + char temp[256];
> + UuidToString((bt_uuid_t*)(p.val + (i * MAX_UUID_SIZE)), temp);
Why not use |BluetoothUuidHelper::GetString|?
@@ +515,5 @@
> + uuidsArray.AppendElement(NS_ConvertUTF8toUTF16(temp));
> + if (isCodInvalid && strncmp(temp, SERVICE_CLASS_HEADSET_UUID,
> + strlen(SERVICE_CLASS_HEADSET_UUID))) {
> + BT_LOGD("Restore Class Of Device to Audio bits");
> + cod |= 0x200000;
How about make this a macro as [1]?
[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothProfileController.h#38
@@ +516,5 @@
> + if (isCodInvalid && strncmp(temp, SERVICE_CLASS_HEADSET_UUID,
> + strlen(SERVICE_CLASS_HEADSET_UUID))) {
> + BT_LOGD("Restore Class Of Device to Audio bits");
> + cod |= 0x200000;
> +
nit: redundant newline.
@@ +521,5 @@
> + }
> + if (isCodInvalid && strncmp(temp, SERVICE_CLASS_SINK_UUID,
> + strlen(SERVICE_CLASS_SINK_UUID))) {
> + BT_LOGD("Restore Class of Device to Rendering bits");
> + cod |= 0x40000;
Ditto.
@@ +522,5 @@
> + if (isCodInvalid && strncmp(temp, SERVICE_CLASS_SINK_UUID,
> + strlen(SERVICE_CLASS_SINK_UUID))) {
> + BT_LOGD("Restore Class of Device to Rendering bits");
> + cod |= 0x40000;
> + }
Rewrite to:
for (...) {
uuidsArray.AppendElement();
if (isCodInvalid) {
if (HEADSET_UUID) {
...
}
if (SINK_UUID) {
...
}
}
}
::: dom/bluetooth/bluedroid/BluetoothUtils.cpp
@@ +65,5 @@
> + memcpy(&uuid1, &(p_uuid->uu[4]), 2);
> + memcpy(&uuid2, &(p_uuid->uu[6]), 2);
> + memcpy(&uuid3, &(p_uuid->uu[8]), 2);
> + memcpy(&uuid4, &(p_uuid->uu[10]), 4);
> + memcpy(&uuid5, &(p_uuid->uu[14]), 2);
Replace 4 with |sizeof(uint32_t)| and 2 with |sizeof(uint16_t)|.
@@ +67,5 @@
> + memcpy(&uuid3, &(p_uuid->uu[8]), 2);
> + memcpy(&uuid4, &(p_uuid->uu[10]), 4);
> + memcpy(&uuid5, &(p_uuid->uu[14]), 2);
> +
> + sprintf((char *)str, "%.8x-%.4x-%.4x-%.4x-%.8x%.4x", ntohl(uuid0), ntohs(uuid1),
No need to cast |str| since it's already of type |char*|.
::: dom/bluetooth/bluedroid/BluetoothUtils.h
@@ +29,5 @@
> BdAddressTypeToString(bt_bdaddr_t* aBdAddressType,
> nsAString& aRetBdAddress);
>
> +void
> +UuidToString(bt_uuid_t *p_uuid, char *str);
nit: newline here.
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Ben Tian [:btian] from comment #13)
> Comment on attachment 8440624 [details] [diff] [review]
> Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
>
> Review of attachment 8440624 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +484,4 @@
> > BdAddressTypeToString(aBdAddress, remoteDeviceBdAddress);
> > BT_APPEND_NAMED_VALUE(props, "Address", remoteDeviceBdAddress);
> >
> > + bool isCodInvalid = false;
>
> I think |isCodValid| is easier to understand than |isCodeInvalid|.
I think |isCodInvalid| make more sense here.
>
> @@ +508,5 @@
> > + } else if (p.type == BT_PROPERTY_UUIDS) {
> > + InfallibleTArray<nsString> uuidsArray;
> > + int uuid_list_leng = p.len / MAX_UUID_SIZE;
> > + uint32_t cod = 0;
> > + for (int i=0; i < uuid_list_leng; i++) {
>
> nit: space => |i = 0|
>
> @@ +510,5 @@
> > + int uuid_list_leng = p.len / MAX_UUID_SIZE;
> > + uint32_t cod = 0;
> > + for (int i=0; i < uuid_list_leng; i++) {
> > + char temp[256];
> > + UuidToString((bt_uuid_t*)(p.val + (i * MAX_UUID_SIZE)), temp);
>
> Why not use |BluetoothUuidHelper::GetString|?
Well, I think to compare the whole UUID string is easier than casting 'raw' (bt_uuid_t) type to BluetoothServiceClass. But |BluetoothUuidHelper::GetString| cannot meet the requirement . I can do:
BluetoothServiceClass serviceClass = UuidToServiceUuid((bt_uuid_t*)(p.val + (i * MAX_UUID_SIZE)));
...
if (isCodInvalid) {
if (serviceClass == BluetoothServiceClass::A2DP) {
...
}
}
> @@ +515,5 @@
> > + uuidsArray.AppendElement(NS_ConvertUTF8toUTF16(temp));
> > + if (isCodInvalid && strncmp(temp, SERVICE_CLASS_HEADSET_UUID,
> > + strlen(SERVICE_CLASS_HEADSET_UUID))) {
> > + BT_LOGD("Restore Class Of Device to Audio bits");
> > + cod |= 0x200000;
>
> How about make this a macro as [1]?
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/
> BluetoothProfileController.h#38
>
Is it necessary to add a macro, it just only used one time?
Assignee | ||
Updated•10 years ago
|
Attachment #8440624 -
Attachment is obsolete: true
Attachment #8440624 -
Flags: review?(echou)
Attachment #8440624 -
Flags: feedback?(btian)
Assignee | ||
Comment 15•10 years ago
|
||
nits and suggestion addressed.
Assignee | ||
Updated•10 years ago
|
Attachment #8441874 -
Flags: feedback?(btian)
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8439171 [details] [diff] [review]
[Gaia]Bug 1015819 - Delay outgoing connection after bonded
Bluetooth backend needs to update SDP records after pairing completed, so I suggest waiting for 1.5 sec to init connection. Otherwise, we can make an outgoing connection without receiving SDP update, which results to connection failure.
Attachment #8439171 -
Flags: review?(ehung)
Comment 17•10 years ago
|
||
Comment on attachment 8441874 [details] [diff] [review]
Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
Review of attachment 8441874 [details] [diff] [review]:
-----------------------------------------------------------------
f=me with comment addressed. Thanks.
::: dom/bluetooth/BluetoothProfileController.h
@@ +45,5 @@
> +#define USE_AUDIO(cod) (cod | 0x200000)
> +
> +// Rendering: Major service class = 0x20 (Bit 18 is set)
> +#define USE_RENDERING(cod) (cod | 0x40000)
> +
Move these definition to |BluetoothServiceBluedroid.cpp|, the file they are used.
::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +41,4 @@
> return result; \
> } \
> } while(0)
> +#define MAX_UUID_SIZE 16
nit: newline here.
::: dom/bluetooth/bluedroid/BluetoothUtils.cpp
@@ +64,5 @@
> + sprintf(tmp, "%.8x", ntohl(uuid0));
> + nsresult rv;
> + nsString serviceClass = NS_ConvertUTF8toUTF16(tmp);
> + uint16_t i = serviceClass.ToInteger(&rv, 16);
> + return i;
Extract uint16_t directly instead of getting uint32_t and then converting it to uint16_t.
::: dom/bluetooth/bluedroid/BluetoothUtils.h
@@ +29,5 @@
> BdAddressTypeToString(bt_bdaddr_t* aBdAddressType,
> nsAString& aRetBdAddress);
>
> +uint16_t
> +UuidToServiceClassInt(bt_uuid_t *p_uuid);
nit: newline here.
Attachment #8441874 -
Flags: feedback?(btian) → feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8441874 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8441944 -
Flags: review?(echou)
Comment 19•10 years ago
|
||
Comment on attachment 8441944 [details] [diff] [review]
Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
Review of attachment 8441944 [details] [diff] [review]:
-----------------------------------------------------------------
Hi Shawn, overall concept looks good to me, r- mainly because too many small nits. Please see my comments below.
::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +504,5 @@
> + } else {
> + // If Cod is invalid, fallback to check UUIDs. It usually happens due to NFC
> + // directly trigger pairing. bluedroid sends wrong CoD due to missing EIR
> + // query records.
> + isCodInvalid = true;
If BT_PROPERTY_UUIDS comes before BT_PROPERTY_CLASS_OF_DEVICE, if it's the case, then your solution still doesn't work. How about setting isCodInvalid to true by default and changing it to false once its CoD is valid?
@@ +508,5 @@
> + isCodInvalid = true;
> + }
> + } else if (p.type == BT_PROPERTY_UUIDS) {
> + InfallibleTArray<nsString> uuidsArray;
> + int uuid_list_leng = p.len / MAX_UUID_SIZE;
nit: please don't use c-style naming. uuidListLength is better.
@@ +529,5 @@
> + serviceClass == BluetoothServiceClass::HEADSET) {
> + BT_LOGD("Restore Class Of Device to Audio bit");
> + cod = USE_AUDIO(cod);
> + }
> + if (serviceClass == BluetoothServiceClass::A2DP_SINK) {
Can we use 'else if' here?
@@ +538,5 @@
> + }
> +
> + if (isCodInvalid) {
> + BT_APPEND_NAMED_VALUE(props, "Class", cod);
> + BT_APPEND_NAMED_VALUE(props, "Icon", NS_LITERAL_STRING("audio-card"));
Since "audio-card" is a fixed string, please add comments to explain why "audio-card" is chosen but not other strings. :)
::: dom/bluetooth/bluedroid/BluetoothUtils.cpp
@@ +55,5 @@
> aRetBdAddress = NS_ConvertUTF8toUTF16(bdstr);
> }
>
> +uint16_t
> +UuidToServiceClassInt(bt_uuid_t *p_uuid)
nit: please append '*' to the class type.
@@ +57,5 @@
>
> +uint16_t
> +UuidToServiceClassInt(bt_uuid_t *p_uuid)
> +{
> + uint16_t uuid0;
Why uuid'0'? Can we just name it uuid or shortUuid?
::: dom/bluetooth/bluedroid/BluetoothUtils.h
@@ +29,5 @@
> BdAddressTypeToString(bt_bdaddr_t* aBdAddressType,
> nsAString& aRetBdAddress);
>
> +uint16_t
> +UuidToServiceClassInt(bt_uuid_t *p_uuid);
nit: please append '*' to the class type.
Attachment #8441944 -
Flags: review?(echou) → review-
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Eric Chou [:ericchou] [:echou] from comment #19)
> Comment on attachment 8441944 [details] [diff] [review]
> Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
>
> Review of attachment 8441944 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +504,5 @@
> > + } else {
> > + // If Cod is invalid, fallback to check UUIDs. It usually happens due to NFC
> > + // directly trigger pairing. bluedroid sends wrong CoD due to missing EIR
> > + // query records.
> > + isCodInvalid = true;
>
> If BT_PROPERTY_UUIDS comes before BT_PROPERTY_CLASS_OF_DEVICE, if it's the
> case, then your solution still doesn't work. How about setting isCodInvalid
> to true by default and changing it to false once its CoD is valid?
>
My major concern is that if application tries to query SDP records, which make UUIDs update, and CoD will be overwritten here.
As far as I know CoD will be always wrapped into RemoteProperties, so this part should work anyway.
Assignee | ||
Updated•10 years ago
|
Attachment #8441944 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Fix nits addressed.
Assignee | ||
Updated•10 years ago
|
Attachment #8442603 -
Flags: review?(echou)
Comment 22•10 years ago
|
||
Comment on attachment 8442603 [details] [diff] [review]
Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records
Review of attachment 8442603 [details] [diff] [review]:
-----------------------------------------------------------------
Shawn, please see my comments below.
::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +45,5 @@
> +#define MAX_UUID_SIZE 16
> +// Audio: Major service class = 0x100 (Bit 21 is set)
> +#define USE_AUDIO(cod) (cod | 0x200000)
> +// Rendering: Major service class = 0x20 (Bit 18 is set)
> +#define USE_RENDERING(cod) (cod | 0x40000)
Since these 3 macros are all used one or two times, I would suggest that let's move them to the block where they are used. In addition, I wonder if we could find a better name for MAX_UUID_SIZE since 16 bytes is actually not the max size of a Bluetooth service uuid.
@@ +508,5 @@
> + isCodInvalid = true;
> + }
> + } else if (p.type == BT_PROPERTY_UUIDS) {
> + InfallibleTArray<nsString> uuidsArray;
> + int uuidListLeng = p.len / MAX_UUID_SIZE;
uuidListLeng'th' may be clearer.
Attachment #8442603 -
Flags: review?(echou)
Assignee | ||
Updated•10 years ago
|
Attachment #8442603 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8442729 -
Flags: review?(echou)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8442767 -
Flags: review?(echou)
Attachment #8442767 -
Flags: feedback?(btian)
Assignee | ||
Updated•10 years ago
|
Attachment #8442767 -
Attachment description: Bug 1015819 - Part 3: [bluez] Restore CoD value based on SDP records → [WIP]Bug 1015819 - Part 3: [bluez] Restore CoD value based on SDP records
Attachment #8442767 -
Flags: review?(echou)
Attachment #8442767 -
Flags: feedback?(btian)
Assignee | ||
Updated•10 years ago
|
Attachment #8442767 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8443233 -
Flags: review?(echou)
Attachment #8443233 -
Flags: feedback?(btian)
Comment 26•10 years ago
|
||
Comment on attachment 8442729 [details] [diff] [review]
Bug 1015819 - Part 1: [bluedroid] Restore CoD value based on SDP records
Review of attachment 8442729 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks.
Attachment #8442729 -
Flags: review?(echou) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8439171 -
Attachment description: Bug 1015819 - Part 1: Delay outgoing connection after bonded → [Gaia]Bug 1015819 - Delay outgoing connection after bonded
Assignee | ||
Updated•10 years ago
|
Attachment #8442729 -
Attachment description: Bug 1015819 - Part 2: [bluedroid] Restore CoD value based on SDP records → Bug 1015819 - Part 1: [bluedroid] Restore CoD value based on SDP records
Assignee | ||
Updated•10 years ago
|
Attachment #8443233 -
Attachment description: Bug 1015819 - Part 3: [bluez] Restore CoD value based on SDP records → Bug 1015819 - Part 2: [bluez] Restore CoD value based on SDP records
Assignee | ||
Updated•10 years ago
|
Attachment #8442729 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
Comment on attachment 8443233 [details] [diff] [review]
Bug 1015819 - Part 2: [bluez] Restore CoD value based on SDP records
Review of attachment 8443233 [details] [diff] [review]:
-----------------------------------------------------------------
f=me with comment addressed. Thanks.
::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +834,4 @@
> }
>
> static bool
> +ContainsClass(const InfallibleTArray<BluetoothNamedValue>& aProperties)
Make |ContainsIcon| a general |ContainsProperty| that takes property name as argument to handle both "Icon" and "Class" properties.
@@ -2682,4 @@
> // We have to manually attach the path to the rest of the elements
> devicePropertiesArray.AppendElement(
> BluetoothNamedValue(NS_LITERAL_STRING("Path"), mObjectPath));
> -
Why remove this newline?
@@ +2712,3 @@
> }
> }
> }
nit: newline here.
@@ +2712,4 @@
> }
> }
> }
> + // Check the whole array contains CoD. If it doesn't, fallback to restore
Check 'whether' the properties array contains CoD.
@@ +2712,5 @@
> }
> }
> }
> + // Check the whole array contains CoD. If it doesn't, fallback to restore
> + // CoD value. This usually happens due to NFC directly trigger pairing,
... due to NFC-triggered direct pairing that makes blueZ not update CoD value.
@@ +2721,5 @@
> + for (uint32_t j = 0; j < devicePropertiesArray.Length(); ++j) {
> + BluetoothNamedValue& deviceProperty = devicePropertiesArray[j];
> +
> + if (deviceProperty.name().EqualsLiteral("UUIDs")) {
> + uint32_t length = deviceProperty.value().get_ArrayOfnsString().Length();
It's clearer to store |deviceProperty.value().get_ArrayOfnsString()| as a local variable.
const InfallibleTArray<nsString>& uuids =
deviceProperty.value().get_ArrayOfnsString();
for (uint32_t i = 0; i < uuids.Length(); i++) {
nsString& data = uuids[i];
...
}
@@ +2744,5 @@
> + BluetoothNamedValue(NS_LITERAL_STRING("Class"), cod));
> + devicePropertiesArray.AppendElement(
> + BluetoothNamedValue(NS_LITERAL_STRING("Icon"),
> + NS_LITERAL_STRING("audio-card")));
> + break;
nit: newline before |break;|
Attachment #8443233 -
Flags: feedback?(btian) → feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8443233 -
Attachment is obsolete: true
Attachment #8443233 -
Flags: review?(echou)
Assignee | ||
Comment 30•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8443404 -
Flags: review?(echou)
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Comment 31•10 years ago
|
||
Comment on attachment 8443404 [details] [diff] [review]
Bug 1015819 - Part 2: [bluez] Restore CoD value based on SDP records
Review of attachment 8443404 [details] [diff] [review]:
-----------------------------------------------------------------
Hi Shawn,
Please see my comments below.
::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +91,3 @@
> #define BT_LAZY_THREAD_TIMEOUT_MS 3000
> +#define SET_AUDIO_BIT(cod) (cod |= 0x200000)
> +#define SET_RENDERING_BIT(cod) (cod |= 0x40000)
nit: please add comments before these two macros. This can also separate SET_AUDIO_BIT and BT_LAZY_THREAD_TIMEOUT_MS since they are not related.
@@ +2712,5 @@
> +
> + for (uint32_t j = 0; j < devicePropertiesArray.Length(); ++j) {
> + BluetoothNamedValue& deviceProperty = devicePropertiesArray[j];
> +
> + if (deviceProperty.name().EqualsLiteral("UUIDs")) {
one suggestion: since you have changed the ContainsIcon() interface to a more generic ContainsProperty() (which is good!), for this "finding index" case, maybe we could change the interface to something like FindProperty() so that we can wrap this for-loop and if-statement into something like:
int uuidIndex = FindProperty(devicePropertiesArray, "UUIDs");
if (uuidIndex >= 0) {
BluetoothNamedValue& deviceProperty = devicePropertiesArray[uuidIndex];
}
Just a suggestion so that we can make our code more 'flat'.
@@ +2716,5 @@
> + if (deviceProperty.name().EqualsLiteral("UUIDs")) {
> + const InfallibleTArray<nsString>& uuids =
> + deviceProperty.value().get_ArrayOfnsString();
> +
> + for (uint32_t i = 0; i < uuids.Length(); i++) {
super-nit: let's use '++i' for consistency since you have already used '++j'
Attachment #8443404 -
Flags: review?(echou)
Assignee | ||
Updated•10 years ago
|
Attachment #8443404 -
Attachment is obsolete: true
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8444393 -
Flags: review?(echou)
Comment 33•10 years ago
|
||
Comment on attachment 8439171 [details] [diff] [review]
[Gaia]Bug 1015819 - Delay outgoing connection after bonded
Can you explain how the 1.5 sec is decided? If it's a guessing value, can we have an event callback instead? Thanks. (Please flag Arthur or Alive to review on the next round since I'm PTO. Thanks!)
Attachment #8439171 -
Flags: review?(ehung)
Comment 34•10 years ago
|
||
Comment on attachment 8444393 [details] [diff] [review]
Bug 1015819 - Part 2: [bluez] Restore CoD value based on SDP records
Review of attachment 8444393 [details] [diff] [review]:
-----------------------------------------------------------------
Hi Shawn,
Please see my comments below.
::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +91,2 @@
> #define BT_LAZY_THREAD_TIMEOUT_MS 3000
> +// Set Class of Device value bit
Could you write something about 'why we need this macro' instead of 'what this macro does'? Also, please insert a blank line. It will be easier for people to group macros.
@@ +835,5 @@
> return false;
> }
>
> +static int
> +FindPropertyIndex(const InfallibleTArray<BluetoothNamedValue>& aProperties,
Question: You left the original ContainsProperty() and added a new function FinePropertyIndex(). Is there any special reason that we can't just have FindPropertyIndex()?
Question 2: I think FindProperty() would be better than FindPropertyIndex() because we actually want to find the 'property' it self but not the index. Or maybe you can use something like IndexOfProperty()?
@@ +836,5 @@
> }
>
> +static int
> +FindPropertyIndex(const InfallibleTArray<BluetoothNamedValue>& aProperties,
> + const char* aPropertyType)
nit: weird indent
@@ +838,5 @@
> +static int
> +FindPropertyIndex(const InfallibleTArray<BluetoothNamedValue>& aProperties,
> + const char* aPropertyType)
> +{
> + for (uint32_t j = 0; j < aProperties.Length(); ++j) {
1. Since uint32_t is used for variable 'j', the return value of this function should be uint32_t as well.
2. Please use either 'i' or other meaningful variable names since this is an one-layer loop.
3. You use uint32_t here but uint8_t in ContainsProperty(). Please be consistent.
@@ +2722,5 @@
> + // makes bluez not update CoD value.
> + if (!ContainsProperty(devicePropertiesArray, "Class")) {
> + uint32_t cod = 0;
> + int uuidIndex = FindPropertyIndex(devicePropertiesArray, "UUIDs");
> + if (uuidIndex > 0) {
Shouldn't this be '>=' 0?
Attachment #8444393 -
Flags: review?(echou) → review-
Assignee | ||
Updated•10 years ago
|
Attachment #8444393 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8444997 -
Flags: review?(echou)
Comment 36•10 years ago
|
||
Comment on attachment 8444997 [details] [diff] [review]
Bug 1015819 - Part 2: [bluez] Restore CoD value based on SDP records
Review of attachment 8444997 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM. Thanks!
Attachment #8444997 -
Flags: review?(echou) → review+
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8444997 -
Attachment is obsolete: true
Assignee | ||
Comment 38•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Attachment #8439171 -
Attachment is obsolete: true
Comment 39•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/491b01971111
https://hg.mozilla.org/integration/b2g-inbound/rev/3a53ffc076a9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/491b01971111
https://hg.mozilla.org/mozilla-central/rev/3a53ffc076a9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 41•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/444dc23190f4
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf58e8801181
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
status-firefox33:
--- → fixed
Comment 42•10 years ago
|
||
Verified on
Gaia 2248c0367661db9332f70f37055e1a8176f5f612
Gecko https://hg.mozilla.org/releases/mozilla-aurora/rev/44d31566a3a6
BuildID 20140629160202
Version 32.0a2
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•