Closed
Bug 1159267
Opened 10 years ago
Closed 10 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•10 years ago
|
||
Attachment #8598665 -
Flags: review?(btian)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8598667 -
Flags: review?(btian)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8598668 -
Flags: review?(btian)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8598669 -
Flags: review?(btian)
Assignee | ||
Comment 5•10 years ago
|
||
Comment 6•10 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•10 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•10 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•10 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•10 years ago
|
||
Changes since v1:
- simplified error message
Attachment #8598669 -
Attachment is obsolete: true
Attachment #8599840 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
Changes since v1:
- keep cleanup code separated between variants
Attachment #8598667 -
Attachment is obsolete: true
Attachment #8599842 -
Flags: review?(btian)
Comment 12•10 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•10 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•10 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•10 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•10 years ago
|
||
Great! Thanks Ben!
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=f7fe73c7e071
Comment 17•10 years ago
|
||
Comment 18•10 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: 10 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
•