Closed Bug 931802 Opened 6 years ago Closed 6 years ago

[bluedroid] Support DeviceFound/DiscoveryStateChange callback

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

Attachments

(1 file, 6 obsolete files)

Support DeviceFound/DiscoveryStateChange callback
Attachment #823357 - Attachment is obsolete: true
Attachment #823357 - Flags: review?(echou)
Attachment #823422 - Attachment is obsolete: true
Attachment #823422 - Flags: review?(echou)
Attachment #824444 - Attachment is obsolete: true
Attachment #824444 - Flags: review?(echou)
Open bug 932855 for tracking RSSI/type. It's nice to have these two fields.
Comment on attachment 824740 [details] [diff] [review]
Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Review of attachment 824740 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Shawn, please see my comments below.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +68,5 @@
>  static bool sAdapterDiscoverable = false;
>  static nsString sAdapterBdAddress;
>  static nsString sAdapterBdName;
>  static uint32_t sAdapterDiscoverableTimeout;
> +static nsTArray<BluetoothReplyRunnable*> sChangeDiscoveryRunnableArray;

Please use nsTArray<nsRefPtr<BluetoothReplyRunnable> >.

@@ +75,5 @@
>  /**
>   *  Static callback functions
>   */
> +
> + static void

nit: extra blank at the head

@@ +267,5 @@
> +  for (int i = 0; i < aNumProperties; ++i) {
> +    bt_property_t p = aProperties[i];
> +
> +    if (p.type == BT_PROPERTY_BDNAME) {
> +      BluetoothValue propertyValue = NS_ConvertUTF8toUTF16((char*)p.val);

p.val should be handled the same as it's handled in AdapterPropertiesChangeCallback because it has no null terminator.

@@ +282,5 @@
> +    } else {
> +      BT_LOGR("Other non-handled device properties. Type: %d", p.type);
> +    }
> +  }
> +

nit: extra line.

@@ +301,5 @@
> +      propertyValue = remoteDeviceBdAddress;
> +      propertiesArray.AppendElement(
> +          BluetoothNamedValue(NS_LITERAL_STRING("Address"), propertyValue));
> +    } else if (p.type == BT_PROPERTY_BDNAME) {
> +      propertyValue = NS_ConvertUTF8toUTF16((char*)p.val);

Ditto.

@@ +330,5 @@
> +  }
> +}
> +
> +static void
> +DiscoveryStateChangedCallback(bt_discovery_state_t state)

nit: the argument name is usually initialized with an 'a'.

@@ +341,5 @@
> +    sChangeDiscoveryRunnableArray.RemoveElementAt(0);
> +  }
> +}
> +
> +bt_callbacks_t sBluetoothCallbacks = 

nit: trailing whitespace

@@ +510,5 @@
>    BluetoothReplyRunnable* aRunnable)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (sBtInterface) {

Please handle the case "if (!sBtInterface)" before this line.

@@ +529,5 @@
>    BluetoothReplyRunnable* aRunnable)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (sBtInterface) {

Please handle the case "if (!sBtInterface)" before this line.
Attachment #824740 - Flags: review?(echou) → review-
> Hi Shawn, please see my comments below.
> 
> @@ +267,5 @@
> > +  for (int i = 0; i < aNumProperties; ++i) {
> > +    bt_property_t p = aProperties[i];
> > +
> > +    if (p.type == BT_PROPERTY_BDNAME) {
> > +      BluetoothValue propertyValue = NS_ConvertUTF8toUTF16((char*)p.val);
> 
> p.val should be handled the same as it's handled in
> AdapterPropertiesChangeCallback because it has no null terminator.
> 
> @@ +301,5 @@
> > +      propertyValue = remoteDeviceBdAddress;
> > +      propertiesArray.AppendElement(
> > +          BluetoothNamedValue(NS_LITERAL_STRING("Address"), propertyValue));
> > +    } else if (p.type == BT_PROPERTY_BDNAME) {
> > +      propertyValue = NS_ConvertUTF8toUTF16((char*)p.val);
> 
> Ditto.
> 
It's quite tricky, both these two places, they contained "null terminator", so NS_ConvertUTF8toUTF16 has no problem to convert. Currently only AdapterPropertiesChangeCallback with BD_NAME property has the problem. So that's why I did not handle it.
Changes: nits addressed were fixed and add assertion in callback
Attachment #826599 - Flags: review?(echou)
Attachment #826599 - Attachment is obsolete: true
Attachment #826599 - Flags: review?(echou)
Comment on attachment 826603 [details] [diff] [review]
Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Review of attachment 826603 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed. Thanks.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +527,5 @@
> +  if (!IsReady()) {
> +    NS_NAMED_LITERAL_STRING(errorStr, "Bluetooth service is not ready yet!");
> +    DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
> +    return NS_OK;
> +  }

super-nit: please add a newline here.

@@ +548,5 @@
> +  if (!IsReady()) {
> +    NS_NAMED_LITERAL_STRING(errorStr, "Bluetooth service is not ready yet!");
> +    DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);
> +    return NS_OK;
> +  }

Ditto.
Attachment #826603 - Flags: review?(echou) → review+
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
https://hg.mozilla.org/mozilla-central/rev/a33fcd67a7db
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.