Closed Bug 1190730 Opened 4 years ago Closed 4 years ago

[cleanup] Replace static variables in BluetoothServiceBluedroid with member ones

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
FxOS-S5 (21Aug)
Tracking Status
firefox42 --- fixed

People

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

Details

Attachments

(5 files, 7 obsolete files)

32.99 KB, patch
yrliou
: review+
Details | Diff | Splinter Review
21.73 KB, patch
Details | Diff | Splinter Review
9.12 KB, patch
Details | Diff | Splinter Review
8.53 KB, patch
Details | Diff | Splinter Review
13.45 KB, patch
Details | Diff | Splinter Review
We should replace static variables with member ones as both methods and notifications run on main thread now.
Assignee: nobody → btian
Attachment #8642896 - Flags: review?(joliu)
Attachment #8642897 - Flags: review?(joliu)
Attachment #8642899 - Flags: review?(joliu)
Comment on attachment 8642896 [details] [diff] [review]
Patch 1/5 (v1): Make adapter properties member variables

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +116,5 @@
>  
>  #ifndef MOZ_B2G_BT_API_V1
>  // Missing in Bluetooth v2
>  #else
> +static uint32_t sDiscoverableTimeout(0);

May I ask why this one isn't a member variable?

@@ +2026,3 @@
>        // properties to bluetooth backend once Bluetooth is enabled.
>        if (IsEnabled()) {
> +        mDiscoverable =

nit: put in one line
Attachment #8642896 - Flags: review?(joliu) → review+
Comment on attachment 8642897 [details] [diff] [review]
Patch 2/5 (v1): Make backend recovery variables member ones

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

LGTM.
Attachment #8642897 - Flags: review?(joliu) → review+
Comment on attachment 8642898 [details] [diff] [review]
Patch 3/5 (v1): Make address-name mapping table member variable

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

LGTM.
Attachment #8642898 - Flags: review?(joliu) → review+
Comment on attachment 8642899 [details] [diff] [review]
Patch 4/5(v1): Make runnable arrays member variables

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

Hi Ben,

Sorry for the delay here, please see my comments below.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +985,5 @@
>    // Missing in bluetooth1
>  #endif
>  
> +  sBtInterface->StartDiscovery(
> +    new StartDiscoveryResultHandler(mChangeDiscoveryRunnables, aRunnable));

Won't this line also be compiled in v1?
Please correct me if I am wrong, thanks.

@@ +1092,5 @@
>  #endif
>  
> +  sBtInterface->CancelDiscovery(
> +    new CancelDiscoveryResultHandler(mChangeDiscoveryRunnables,
> +                                     aRunnable));

DITTO, mChangeDiscoveryRunnables is not defined in v1.

@@ +1445,5 @@
>    sControllerArray.AppendElement(controller);
>  
>    /**
> +   * If the request is the first element of the queue, start from here. Note
> +   * that other request is pushed into the queue and is popped out after the

other requests are pushed ... and are popped out ...

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
@@ +409,5 @@
>  
> +  // Runnable arrays
> +  nsTArray<nsRefPtr<BluetoothReplyRunnable>> mSetAdapterPropertyRunnables;
> +  nsTArray<nsRefPtr<BluetoothReplyRunnable>> mGetDeviceRunnables;
> +  nsTArray<nsRefPtr<BluetoothReplyRunnable>> mBondRunnables;

Suggest to use mCreate/RemoveBondRunnables.
Attachment #8642899 - Flags: review?(joliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #6)
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +116,5 @@
> >  
> >  #ifndef MOZ_B2G_BT_API_V1
> >  // Missing in Bluetooth v2
> >  #else
> > +static uint32_t sDiscoverableTimeout(0);
> 
> May I ask why this one isn't a member variable?

I meant to save the effort as it's for obsolete bluetooth1... Well I'll also make it a member variable.
(In reply to Ben Tian [:btian] from comment #10)
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #6)
> > ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> > @@ +116,5 @@
> > >  
> > >  #ifndef MOZ_B2G_BT_API_V1
> > >  // Missing in Bluetooth v2
> > >  #else
> > > +static uint32_t sDiscoverableTimeout(0);
> > 
> > May I ask why this one isn't a member variable?
> 
> I meant to save the effort as it's for obsolete bluetooth1... Well I'll also
> make it a member variable.

Ben, that makes sense, I'm totally fine to leave it unchanged.
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #9)
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +985,5 @@
> >    // Missing in bluetooth1
> >  #endif
> >  
> > +  sBtInterface->StartDiscovery(
> > +    new StartDiscoveryResultHandler(mChangeDiscoveryRunnables, aRunnable));
> 
> Won't this line also be compiled in v1?
> Please correct me if I am wrong, thanks.

You are right. We should remove bluetooth1 ASAP to save the redundant effort :(
Revise based on reviewer's comment. I've also verified build pass on v1.
Attachment #8642899 - Attachment is obsolete: true
Attachment #8644750 - Flags: review?(joliu)
Rebase on the latest m-c and patch 4 change.
Attachment #8642900 - Attachment is obsolete: true
Attachment #8642900 - Flags: review?(joliu)
Attachment #8644750 - Attachment description: Patch 4/5(v2): Make runnable arrays member variables → Patch 4/5 (v2): Make runnable arrays member variables
Rebase patch 1-3 on the latest m-c.
Attachment #8642898 - Attachment is obsolete: true
Attachment #8644751 - Flags: review?(joliu)
Comment on attachment 8644750 [details] [diff] [review]
[final] Patch 4/5: Make runnable arrays member variables, r=joliu

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

r=me, thanks for the effort!
Attachment #8644750 - Flags: review?(joliu) → review+
Comment on attachment 8644751 [details] [diff] [review]
Patch 5/5 (v2): Wrap get device related variables into get device request

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +844,5 @@
>  
>      BT_WARNING("GetRemoteDeviceProperties(%s) failed: %d",
>                 NS_ConvertUTF16toUTF8(mDeviceAddress).get(), aStatus);
>  
> +    /* Dispatch result after final pending operation */

the final

@@ +845,5 @@
>      BT_WARNING("GetRemoteDeviceProperties(%s) failed: %d",
>                 NS_ConvertUTF16toUTF8(mDeviceAddress).get(), aStatus);
>  
> +    /* Dispatch result after final pending operation */
> +    if (--mRequests[0].mDeviceCount == 0) {

Can we assure the array will never be empty here?
If yes, suggest to add a MOZ_ASSERT here at the beginning.
Otherwise, add an empty check before accessing mRequests[0].

@@ +2339,5 @@
>      return;
>    }
>  
>    // Use address as the index
> +  mGetDeviceRequests[0].mDevicesPack.AppendElement(

Check if mGetDeviceRequests is empty before accessing it?

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h
@@ +413,5 @@
>    bool mIsRestart;
>    bool mIsFirstTimeToggleOffBt;
>  
>    // Runnable arrays
> +  nsTArray<GetDeviceRequest> mGetDeviceRequests;

nit: It doesn't look like a runnable array, I would suggest to write another comment for this member variable.
Attachment #8644751 - Flags: review?(joliu) → review+
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #19)
> @@ +845,5 @@
> >      BT_WARNING("GetRemoteDeviceProperties(%s) failed: %d",
> >                 NS_ConvertUTF16toUTF8(mDeviceAddress).get(), aStatus);
> >  
> > +    /* Dispatch result after final pending operation */
> > +    if (--mRequests[0].mDeviceCount == 0) {
> 
> Can we assure the array will never be empty here?
> If yes, suggest to add a MOZ_ASSERT here at the beginning.
> Otherwise, add an empty check before accessing mRequests[0].

I'll add MOZ_ASSERT at the beginning of |OnError|.

> @@ +2339,5 @@
> >      return;
> >    }
> >  
> >    // Use address as the index
> > +  mGetDeviceRequests[0].mDevicesPack.AppendElement(
> 
> Check if mGetDeviceRequests is empty before accessing it?

There's one (for bluetooth1) but I missed it. Will add it in the final patch.
Attachment #8644750 - Attachment is obsolete: true
Revise based on reviewer's comment.
Attachment #8644751 - Attachment is obsolete: true
Comment on attachment 8644800 [details] [diff] [review]
[final] Patch 4/5: Make runnable arrays member variables, r=joliu

This is the same patch as previous one. Set this patch as obsolete.
Attachment #8644800 - Attachment is obsolete: true
Attachment #8644750 - Attachment description: Patch 4/5 (v2): Make runnable arrays member variables → [final] Patch 4/5: Make runnable arrays member variables, r=joliu
Attachment #8644750 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.