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)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 verified, b2g-v2.1 fixed)

Status:
VERIFIED FIXED
Milestone:
2.0 S5 (4july)
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
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)

Bug 1015819 - Part 1: [bluedroid] Restore CoD value based on SDP records, r=echou, f=btain
7.33 KB, patch
Details | Diff | Splinter Review
Bug 1015819 - Part 2: [bluez] Restore CoD value based on SDP records, r=echou, f=btian
5.74 KB, 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.
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
blocking-b2g: --- → 2.0?
Assignee: nobody → shuang
blocking-b2g: 2.0? → 2.0+
Target to be fixed in sprint4 per discussion with Shawn.
Target Milestone: --- → 2.0 S4 (20june)
Note: This bug targets on fixing auto-connection to Bluetooth headsets with NFC tags. This is not for file sharing.
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
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
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
One thing I noticed, it looks like whenever NFC turns on bluetooth, discovery will be automatically performed, this is something not good also.
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.
On devices used bluedroid stack, this patch works, but still need to work on bluez based.
Attachment #8439171 - Attachment description: (WIP)Bug 1015819 - Part 1: Delay outgoing connection after bonded → Bug 1015819 - Part 1: Delay outgoing connection after bonded
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
Attachment #8440624 - Flags: review?(echou)
Attachment #8440624 - Flags: feedback?(btian)
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.
(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?
Attachment #8440624 - Attachment is obsolete: true
Attachment #8440624 - Flags: review?(echou)
Attachment #8440624 - Flags: feedback?(btian)
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 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+
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-
(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.
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)
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)
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+
Attachment #8439171 - Attachment description: Bug 1015819 - Part 1: Delay outgoing connection after bonded → [Gaia]Bug 1015819 - Delay outgoing connection after bonded
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
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
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+
Attachment #8443233 - Attachment is obsolete: true
Attachment #8443233 - Flags: review?(echou)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
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)
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 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-
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+
Verified on
Gaia      2248c0367661db9332f70f37055e1a8176f5f612
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/44d31566a3a6
BuildID   20140629160202
Version   32.0a2
Status: RESOLVED → VERIFIED
Blocks: 1032627
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: