Closed
Bug 1159267
Opened 9 years ago
Closed 9 years ago
Merge Bluetooth v1/v2 managers
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
2.2 S12 (15may)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: tzimmermann, Assigned: tzimmermann)
References
Details
Attachments
(4 files, 3 obsolete files)
4.51 KB,
patch
|
ben.tian
:
review+
|
Details | Diff | Splinter Review |
2.07 KB,
patch
|
ben.tian
:
review+
|
Details | Diff | Splinter Review |
3.80 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
4.85 KB,
patch
|
ben.tian
:
review+
|
Details | Diff | Splinter Review |
The manager classes in the Bluetooth backend code have mostly minor differences between v1 and v2. We should merge the code as far as possible.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8598665 -
Flags: review?(btian)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8598667 -
Flags: review?(btian)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8598668 -
Flags: review?(btian)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8598669 -
Flags: review?(btian)
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2495eeca4885
Comment 6•9 years ago
|
||
Comment on attachment 8598669 [details] [diff] [review] [04] Bug 1159267: Share BlueZ's HFP manager between Bluetooth v1 and v2 Review of attachment 8598669 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp @@ +1111,5 @@ > nsCString warningMsg; > warningMsg.Append(NS_LITERAL_CSTRING("Unsupported AT command: ")); > warningMsg.Append(msg); > warningMsg.Append(NS_LITERAL_CSTRING(", reply with ERROR")); > BT_WARNING(warningMsg.get()); Simplify to BT_WARNING("Unsupported AT command: %s, reply with ERROR", msg.get());
Attachment #8598669 -
Flags: review?(btian) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8598665 [details] [diff] [review] [01] Bug 1159267: Share Bluedroid's OPP manager between Bluetooth v1 and v2 Review of attachment 8598665 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8598665 -
Flags: review?(btian) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8598668 [details] [diff] [review] [03] Bug 1159267: Share Bluedroid's A2DP manager between Bluetooth v1 and v2 Review of attachment 8598668 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8598668 -
Flags: review?(btian) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8598667 [details] [diff] [review] [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2 Review of attachment 8598667 [details] [diff] [review]: ----------------------------------------------------------------- Please keep |Cleanup| function per bug 1027506. Unlike |Reset|, |Cleanup| keeps call array to handle HFP link loss.
Attachment #8598667 -
Flags: review?(btian)
Assignee | ||
Comment 10•9 years ago
|
||
Changes since v1: - simplified error message
Attachment #8598669 -
Attachment is obsolete: true
Attachment #8599840 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
Changes since v1: - keep cleanup code separated between variants
Attachment #8598667 -
Attachment is obsolete: true
Attachment #8599842 -
Flags: review?(btian)
Comment 12•9 years ago
|
||
Comment on attachment 8599842 [details] [diff] [review] [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2 (v2) Review of attachment 8599842 [details] [diff] [review]: ----------------------------------------------------------------- In comment 9 I meant to adopt APIv1 version rather than APIv2 one. So besides this patch's change, please keep both |Cleanup| and |Reset| as APIv1, and remove APIv2 version code. Sorry I didn't state clearly.
Attachment #8599842 -
Flags: review?(btian)
Comment 13•9 years ago
|
||
Comment on attachment 8598667 [details] [diff] [review] [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2 Review of attachment 8598667 [details] [diff] [review]: ----------------------------------------------------------------- Per comment 12, state clearly to avoid misunderstanding. ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp @@ -254,5 @@ > - // Clear Sco state > - mAudioState = HFP_AUDIO_STATE_DISCONNECTED; > - Cleanup(); > -} > -#endif I meant to keep code inside |#else| rather than |#if MOZ_B2G_BT_API_V2|. @@ -628,4 @@ > Reset(); > -#else > - Cleanup(); > -#endif Ditto. Call |Cleanup| here. ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h @@ -175,5 @@ > -#ifdef MOZ_B2G_BT_API_V2 > - // Removed in bluetooth2 > -#else > - void Cleanup(); > -#endif Keep |Cleanup|.
Assignee | ||
Comment 14•9 years ago
|
||
Oh, now I got it. Here's an updated patch. Changes since v2: - adopted v1 cleanup
Attachment #8599842 -
Attachment is obsolete: true
Attachment #8601514 -
Flags: review?(btian)
Comment 15•9 years ago
|
||
Comment on attachment 8601514 [details] [diff] [review] [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2 (v3) Review of attachment 8601514 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks!
Attachment #8601514 -
Flags: review?(btian) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Great! Thanks Ben! https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=f7fe73c7e071
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/e0b4026e42f6 https://hg.mozilla.org/integration/b2g-inbound/rev/3698cc94f038 https://hg.mozilla.org/integration/b2g-inbound/rev/45ae8fa4b2e6
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e0b4026e42f6 https://hg.mozilla.org/mozilla-central/rev/3698cc94f038 https://hg.mozilla.org/mozilla-central/rev/45ae8fa4b2e6 https://hg.mozilla.org/mozilla-central/rev/f7fe73c7e071
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S12 (15may)
You need to log in
before you can comment on or make changes to this bug.
Description
•