1159267 - Merge Bluetooth v1/v2 managers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1159267: Share Bluedroid's OPP manager between Bluetooth v1 and v2

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1159267: Share Bluedroid's A2DP manager between Bluetooth v1 and v2

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1159267: Share BlueZ's HFP manager between Bluetooth v1 and v2

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2495eeca4885

Comment 6

[Ben Tian (inactive)]

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());

Comment 7

[Ben Tian (inactive)]

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.

Comment 8

[Ben Tian (inactive)]

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.

Comment 9

[Ben Tian (inactive)]

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.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1159267: Share BlueZ's HFP manager between Bluetooth v1 and v2 (v2)

Changes since v1:

  - simplified error message

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2 (v2)

Changes since v1:

  - keep cleanup code separated between variants

Comment 12

[Ben Tian (inactive)]

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.

Comment 13

[Ben Tian (inactive)]

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|.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1159267: Share Bluedroid's HFP manager between Bluetooth v1 and v2 (v3)

Oh, now I got it. Here's an updated patch.

Changes since v2:

  - adopted v1 cleanup

Comment 15

[Ben Tian (inactive)]

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!

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Great! Thanks Ben!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=f7fe73c7e071

Comment 17

[Pulsebot]

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

[Ryan VanderMeulen [:RyanVM]]

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