Closed
Bug 1190730
Opened 9 years ago
Closed 9 years ago
[cleanup] Replace static variables in BluetoothServiceBluedroid with member ones
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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 | ||
Comment 1•9 years ago
|
||
Assignee: nobody → btian
Attachment #8642896 -
Flags: review?(joliu)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8642897 -
Flags: review?(joliu)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8642898 -
Flags: review?(joliu)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8642899 -
Flags: review?(joliu)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8642900 -
Flags: review?(joliu)
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 8•9 years ago
|
||
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 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 12•9 years ago
|
||
(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 :(
Assignee | ||
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
Rebase on the latest m-c and patch 4 change.
Attachment #8642900 -
Attachment is obsolete: true
Attachment #8642900 -
Flags: review?(joliu)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8642896 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8642897 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8644750 -
Attachment description: Patch 4/5(v2): Make runnable arrays member variables → Patch 4/5 (v2): Make runnable arrays member variables
Assignee | ||
Comment 17•9 years ago
|
||
Rebase patch 1-3 on the latest m-c.
Attachment #8642898 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8644751 -
Flags: review?(joliu)
Comment 18•9 years ago
|
||
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 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8644750 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
Revise based on reviewer's comment.
Attachment #8644751 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 24•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ddc7546ded
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/4f7ff69834fe https://hg.mozilla.org/integration/b2g-inbound/rev/334375ea28af https://hg.mozilla.org/integration/b2g-inbound/rev/099de7856b3d https://hg.mozilla.org/integration/b2g-inbound/rev/84662b3e75d4 https://hg.mozilla.org/integration/b2g-inbound/rev/4198f0ba0ec3
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4f7ff69834fe https://hg.mozilla.org/mozilla-central/rev/334375ea28af https://hg.mozilla.org/mozilla-central/rev/099de7856b3d https://hg.mozilla.org/mozilla-central/rev/84662b3e75d4 https://hg.mozilla.org/mozilla-central/rev/4198f0ba0ec3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
You need to log in
before you can comment on or make changes to this bug.
Description
•