Closed Bug 1141944 Opened 7 years ago Closed 7 years ago

[bluetooth2] Revise device paired/unpaired handlers in BluetoothAdapter

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Tracking Status
firefox39 --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → btian
Attachment #8575843 - Flags: review?(shuang)
Attachment #8575843 - Flags: feedback?(jaliu)
Comment on attachment 8575843 [details] [diff] [review]
Patch 1 (v1): Revise device paired/unpaired handlers in BluetoothAdapter

LGTM, thanks.
Attachment #8575843 - Flags: feedback?(jaliu) → feedback+
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?
Attachment #8575843 - Flags: review?(shuang)
(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.)?
Flags: needinfo?(shuang)
(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.
Flags: needinfo?(shuang)
Revise per reviewer's comment.
Attachment #8575843 - Attachment is obsolete: true
Attachment #8577925 - Flags: review?(shuang)
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
Attachment #8577925 - Flags: review?(shuang) → review+
No try since bluetooth2 is not built by default.

https://hg.mozilla.org/integration/b2g-inbound/rev/889c932508bc
Attachment #8577925 - Attachment description: Patch 1 (v2): Revise device paired/unpaired handlers in BluetoothAdapter, f=jaliu → [final] Patch 1: Revise device paired/unpaired handlers in BluetoothAdapter, f=jaliu, r=shuang
https://hg.mozilla.org/mozilla-central/rev/889c932508bc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.