1038645 - [Bluetooth] Port bug 1029389 to bluetooth2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

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

Attachment: [01] Bug 1038645: Asynchronous starting and stopping of profile managers (under bluetooth2/)

Comment 2

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

Attachment: [02] Bug 1038645: Add result-handler class for Bluetooth Handsfree profile (under bluetooth2/)

Comment 3

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

Attachment: [03] Bug 1038645: Asynchronous Bluetooth Handsfree init and cleanup methods (under bluetooth2/)

Comment 4

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

Attachment: [04] Bug 1038645: Asynchronous Bluetooth Handsfree connection handling (under bluetooth2/)

Comment 5

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

Attachment: [05] Bug 1038645: Asynchronous Bluetooth Handsfree voice-recognition functions (under bluetooth2/)

Comment 6

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

Attachment: [06] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::VolumeControl| (under bluetooth2/)

Comment 7

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

Attachment: [07] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::DeviceStatusNotification| (under bluetooth2/)

Comment 8

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

Attachment: [08] Bug 1038645: Asynchronous Bluetooth Handsfree response methods (under bluetooth2/)

Comment 9

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

Attachment: [09] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange| (under bluetooth2/)

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8456783 [details] [diff] [review]
[01] Bug 1038645: Asynchronous starting and stopping of profile managers (under bluetooth2/)

Review of attachment 8456783 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth2/bluedroid/BluetoothA2dpManager.cpp
@@ +507,5 @@
>  
>    sBtA2dpInterface = btInf->GetBluetoothA2dpInterface();
>    NS_ENSURE_TRUE_VOID(sBtA2dpInterface);
>  
>    int ret = sBtA2dpInterface->Init(&sBtA2dpCallbacks);

We should call |aRes->OnError| when a2dp/avrcp init fails.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +151,5 @@
>      return NS_OK;
>    }
>  };
>  
> +/* |ProfileDeinitResultHandler| collect the results of all profile

nit: collects

@@ +809,5 @@
>      }
>    }
>  };
>  
> +/* |ProfileInitResultHandler| collect the results of all profile

Ditto.

::: dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +444,5 @@
>      btInf->GetBluetoothHandsfreeInterface();
>    NS_ENSURE_TRUE_VOID(interface);
>  
> +  if (interface->Init(&sBluetoothHfpCallbacks) != BT_STATUS_SUCCESS) {
> +    aRes->OnError(NS_ERROR_FAILURE);

Ensure |aRes| is not nullptr.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8456784 [details] [diff] [review]
[02] Bug 1038645: Add result-handler class for Bluetooth Handsfree profile (under bluetooth2/)

Review of attachment 8456784 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8456785 [details] [diff] [review]
[03] Bug 1038645: Asynchronous Bluetooth Handsfree init and cleanup methods (under bluetooth2/)

Review of attachment 8456785 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +498,5 @@
>  void
>  BluetoothHfpManager::InitHfpInterface(BluetoothProfileResultHandler* aRes)
>  {
>    BluetoothInterface* btInf = BluetoothInterface::GetInstance();
>    NS_ENSURE_TRUE_VOID(btInf);

We should also call |aRes->OnError| for when |btInf| is nullptr.

@@ +504,3 @@
>    BluetoothHandsfreeInterface *interface =
>      btInf->GetBluetoothHandsfreeInterface();
>    NS_ENSURE_TRUE_VOID(interface);

Ditto.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8456786 [details] [diff] [review]
[04] Bug 1038645: Asynchronous Bluetooth Handsfree connection handling (under bluetooth2/)

Review of attachment 8456786 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8456788 [details] [diff] [review]
[05] Bug 1038645: Asynchronous Bluetooth Handsfree voice-recognition functions (under bluetooth2/)

Review of attachment 8456788 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8456790 [details] [diff] [review]
[06] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::VolumeControl| (under bluetooth2/)

Review of attachment 8456790 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 16

[Ben Tian (inactive)]

Comment on attachment 8456791 [details] [diff] [review]
[07] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::DeviceStatusNotification| (under bluetooth2/)

Review of attachment 8456791 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8456792 [details] [diff] [review]
[08] Bug 1038645: Asynchronous Bluetooth Handsfree response methods (under bluetooth2/)

Review of attachment 8456792 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 18

[Ben Tian (inactive)]

Comment on attachment 8456794 [details] [diff] [review]
[09] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange| (under bluetooth2/)

Review of attachment 8456794 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 19

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

(In reply to Ben Tian [:btian] from comment #10)
> Comment on attachment 8456783 [details] [diff] [review]
> [01] Bug 1038645: Asynchronous starting and stopping of profile managers
> (under bluetooth2/)
> 
> Review of attachment 8456783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth2/bluedroid/BluetoothA2dpManager.cpp
> @@ +507,5 @@
> >  
> >    sBtA2dpInterface = btInf->GetBluetoothA2dpInterface();
> >    NS_ENSURE_TRUE_VOID(sBtA2dpInterface);
> >  
> >    int ret = sBtA2dpInterface->Init(&sBtA2dpCallbacks);
> 
> We should call |aRes->OnError| when a2dp/avrcp init fails.

Well, this place is actually correct. We'll call |aRes->Init| below. But I fixed all other occurrences were the code simply returned without ever calling either OnError or Init.

Comment 20

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

Ben,

When you're done reviewing a patch that needs more work, please set the flag to r- or clear it. Otherwise I usually won't notice easily.

Comment 21

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

Attachment: [01] Bug 1038645: Asynchronous starting and stopping of profile managers (under bluetooth2/) (v2)

Changes since v1:

  - added missing callbacks during profile initialization
  - comment fixes

Comment 22

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

Attachment: [03] Bug 1038645: Asynchronous Bluetooth Handsfree init and cleanup methods (under bluetooth2/) (v2)

Changes since v1:

  - protect calls to aRes by null-pointer check
  - added missing callbacks to profile initialization

Comment 23

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> Ben,
> 
> When you're done reviewing a patch that needs more work, please set the flag
> to r- or clear it. Otherwise I usually won't notice easily.

No problem. I'll set the flag next time.

Comment 24

[Ben Tian (inactive)]

Comment on attachment 8459511 [details] [diff] [review]
[03] Bug 1038645: Asynchronous Bluetooth Handsfree init and cleanup methods (under bluetooth2/) (v2)

Review of attachment 8459511 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. Thanks.

Comment 25

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #19)
> (In reply to Ben Tian [:btian] from comment #10)
> > Comment on attachment 8456783 [details] [diff] [review]
> > [01] Bug 1038645: Asynchronous starting and stopping of profile managers
> > (under bluetooth2/)
> > 
> > Review of attachment 8456783 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth2/bluedroid/BluetoothA2dpManager.cpp
> > @@ +507,5 @@
> > >  
> > >    sBtA2dpInterface = btInf->GetBluetoothA2dpInterface();
> > >    NS_ENSURE_TRUE_VOID(sBtA2dpInterface);
> > >  
> > >    int ret = sBtA2dpInterface->Init(&sBtA2dpCallbacks);
> > 
> > We should call |aRes->OnError| when a2dp/avrcp init fails.
> 
> Well, this place is actually correct. We'll call |aRes->Init| below. But I
> fixed all other occurrences were the code simply returned without ever
> calling either OnError or Init.

Do you mean a2dp and avrcp init failures will be handled after you add |DispatchBluetoothA2dpResult| into |BluetoothA2dpInterface::Init| as patch [03] on Handsfree? Otherwise I don't get why not propagate out |sBtA2dpInterface->Init| failure as |BluetoothHfpManager::InitHfpInterface| of patch [01].

Comment 26

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

> 
> Do you mean a2dp and avrcp init failures will be handled after you add
> |DispatchBluetoothA2dpResult| into |BluetoothA2dpInterface::Init| as patch
> [03] on Handsfree? Otherwise I don't get why not propagate out
> |sBtA2dpInterface->Init| failure as |BluetoothHfpManager::InitHfpInterface|
> of patch [01].

First, the original code didn't report any failures. This patchset is about building asynchronous interfaces, not changing semantics. So I try to limit the differences to a minimum. (I already violated this by adding OnError calls).

The other reason is that there's a similar patchset coming for A2DP/AVRCP in bug 1029390. This will cleanup the code for these profiles.

Comment 27

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #26)
> The other reason is that there's a similar patchset coming for A2DP/AVRCP in
> bug 1029390. This will cleanup the code for these profiles.

Got it. Please handle A2DP error propagation in bug 1029390. Thanks.

Comment 28

[Ben Tian (inactive)]

Comment on attachment 8459510 [details] [diff] [review]
[01] Bug 1038645: Asynchronous starting and stopping of profile managers (under bluetooth2/) (v2)

Review of attachment 8459510 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. Thanks for the effort!

Comment 29

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

Thanks!

https://hg.mozilla.org/integration/b2g-inbound/rev/6ffb42cac945
https://hg.mozilla.org/integration/b2g-inbound/rev/4c6ebce26400
https://hg.mozilla.org/integration/b2g-inbound/rev/19018a3dbab1
https://hg.mozilla.org/integration/b2g-inbound/rev/87417c017cee
https://hg.mozilla.org/integration/b2g-inbound/rev/f1ebd0cb6df2
https://hg.mozilla.org/integration/b2g-inbound/rev/833f317b1553
https://hg.mozilla.org/integration/b2g-inbound/rev/e8a84985d813
https://hg.mozilla.org/integration/b2g-inbound/rev/a2d5d4b0cb90
https://hg.mozilla.org/integration/b2g-inbound/rev/bc0aa44b2885

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=bc0aa44b2885

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6ffb42cac945
https://hg.mozilla.org/mozilla-central/rev/4c6ebce26400
https://hg.mozilla.org/mozilla-central/rev/19018a3dbab1
https://hg.mozilla.org/mozilla-central/rev/87417c017cee
https://hg.mozilla.org/mozilla-central/rev/f1ebd0cb6df2
https://hg.mozilla.org/mozilla-central/rev/833f317b1553
https://hg.mozilla.org/mozilla-central/rev/e8a84985d813
https://hg.mozilla.org/mozilla-central/rev/a2d5d4b0cb90
https://hg.mozilla.org/mozilla-central/rev/bc0aa44b2885