1141944 - [bluetooth2] Revise device paired/unpaired handlers in BluetoothAdapter

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Revise device paired/unpaired handlers in BluetoothAdapter as following:
- fix MOZ_ASSERT for device paired handler
- simplify code of device paired handler
- replace "devicepaired" and "deviceunpaired" with defined strings
- add comment of |mDevices| for paired/unpaired device
- print log when pairing listener is ready

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Revise device paired/unpaired handlers in BluetoothAdapter

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8575843 [details] [diff] [review]
Patch 1 (v1): Revise device paired/unpaired handlers in BluetoothAdapter

LGTM, thanks.

Comment 3

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

Comment on attachment 8575843 [details] [diff] [review]
Patch 1 (v1): Revise device paired/unpaired handlers in BluetoothAdapter

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

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +857,4 @@
>      return;
>    }
>  
> +  MOZ_ASSERT(aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue);

nit: Add an extra line after MOZ_ASSERT

@@ +873,4 @@
>    if (index == mDevices.NoIndex) {
> +    index = mDevices.Length(); // the new device's index
> +    mDevices.AppendElement(
> +      BluetoothDevice::Create(GetOwner(), aValue));

Is there any reason to remove nsRefPtr<BluetoothDevice>?

::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +155,5 @@
>    NS_ENSURE_TRUE_VOID(bs);
>    bs->RegisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
>                                       this);
>  
> +  BT_LOGR("Pairing listener is ready to handle pairing requests");

Is there any potential problem? Do we really need to print this log every time?

Comment 4

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> @@ +873,4 @@
> >    if (index == mDevices.NoIndex) {
> > +    index = mDevices.Length(); // the new device's index
> > +    mDevices.AppendElement(
> > +      BluetoothDevice::Create(GetOwner(), aValue));
> 
> Is there any reason to remove nsRefPtr<BluetoothDevice>?

Just to simplify the code since the |nsRefPtr<BluetoothDevice>| variable is only created temporarily to append to array. Any reason you want to keep it?

> ::: dom/bluetooth2/BluetoothPairingListener.cpp
> @@ +155,5 @@
> >    NS_ENSURE_TRUE_VOID(bs);
> >    bs->RegisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
> >                                       this);
> >  
> > +  BT_LOGR("Pairing listener is ready to handle pairing requests");
> 
> Is there any potential problem? Do we really need to print this log every
> time?

No potential problem. We just want to know when pairing listener is ready. The log only prints when bluetooth app starts. Or do you prefer to print when pairing listener is NOT ready (i.e., leave the code unchanged.)?

Comment 5

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

(In reply to Ben Tian [:btian] from comment #4)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> > @@ +873,4 @@
> > >    if (index == mDevices.NoIndex) {
> > > +    index = mDevices.Length(); // the new device's index
> > > +    mDevices.AppendElement(
> > > +      BluetoothDevice::Create(GetOwner(), aValue));
> > 
> > Is there any reason to remove nsRefPtr<BluetoothDevice>?
> 
> Just to simplify the code since the |nsRefPtr<BluetoothDevice>| variable is
> only created temporarily to append to array. Any reason you want to keep it?
Well, i checked again and found I missed one piece, |nsTArray<nsRefPtr<BluetoothDevice> > mDevices|. So we don't need to keep it.
 
> > ::: dom/bluetooth2/BluetoothPairingListener.cpp
> > @@ +155,5 @@
> > >    NS_ENSURE_TRUE_VOID(bs);
> > >    bs->RegisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
> > >                                       this);
> > >  
> > > +  BT_LOGR("Pairing listener is ready to handle pairing requests");
> > 
> > Is there any potential problem? Do we really need to print this log every
> > time?
> 
> No potential problem. We just want to know when pairing listener is ready.
> The log only prints when bluetooth app starts. Or do you prefer to print
> when pairing listener is NOT ready (i.e., leave the code unchanged.)?
I preferred to remove the log indicates success and add logs for fail cases.

Comment 6

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Revise device paired/unpaired handlers in BluetoothAdapter, f=jaliu, r=shuang

Revise per reviewer's comment.

Comment 7

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

Comment on attachment 8577925 [details] [diff] [review]
[final] Patch 1: Revise device paired/unpaired handlers in BluetoothAdapter, f=jaliu, r=shuang

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

r=me

Comment 8

[Ben Tian (inactive)]

No try since bluetooth2 is not built by default.

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/889c932508bc