931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Support DeviceFound/DiscoveryStateChange callback

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Open bug 932855 for tracking RSSI/type. It's nice to have these two fields.

Comment 6

[Eric Chou [:ericchou] [:echou]]

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.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

> 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.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Changes: nits addressed were fixed and add assertion in callback

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback

Comment 10

[Eric Chou [:ericchou] [: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.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 931802 - [bluedroid] Support DeviceFound/DiscoveryStateChange callback, r=echou

Comment 12

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a33fcd67a7db

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a33fcd67a7db