Closed Bug 1159267 Opened 9 years ago Closed 9 years ago

Merge Bluetooth v1/v2 managers

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

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)

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.
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 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 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 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)
Changes since v1:

  - simplified error message
Attachment #8598669 - Attachment is obsolete: true
Attachment #8599840 - Flags: review+
Changes since v1:

  - keep cleanup code separated between variants
Attachment #8598667 - Attachment is obsolete: true
Attachment #8599842 - Flags: review?(btian)
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 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|.
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 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+
You need to log in before you can comment on or make changes to this bug.