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 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 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 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 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 on attachment 8456790 [details] [diff] [review] [06] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::VolumeControl| (under bluetooth2/) Review of attachment 8456790 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

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

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 on attachment 8456794 [details] [diff] [review] [09] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange| (under bluetooth2/) Review of attachment 8456794 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

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

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.

Changes since v1: - added missing callbacks during profile initialization - comment fixes

Changes since v1: - protect calls to aRes by null-pointer check - added missing callbacks to profile initialization

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

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

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

(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 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!

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

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