[Bluetooth] Port bug 1029389 to bluetooth2

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 2 obsolete attachments)

3.05 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
10.35 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
2.56 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
4.05 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
4.06 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
12.86 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
4.62 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
12.80 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
7.83 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 8456783 [details] [diff] [review]
[01] Bug 1038645: Asynchronous starting and stopping of profile managers (under bluetooth2/)
Attachment #8456783 - Flags: review?(btian)
Created attachment 8456784 [details] [diff] [review]
[02] Bug 1038645: Add result-handler class for Bluetooth Handsfree profile (under bluetooth2/)
Attachment #8456784 - Flags: review?(btian)
Created attachment 8456785 [details] [diff] [review]
[03] Bug 1038645: Asynchronous Bluetooth Handsfree init and cleanup methods (under bluetooth2/)
Attachment #8456785 - Flags: review?(btian)
Created attachment 8456786 [details] [diff] [review]
[04] Bug 1038645: Asynchronous Bluetooth Handsfree connection handling (under bluetooth2/)
Attachment #8456786 - Flags: review?(btian)
Created attachment 8456788 [details] [diff] [review]
[05] Bug 1038645: Asynchronous Bluetooth Handsfree voice-recognition functions (under bluetooth2/)
Attachment #8456788 - Flags: review?(btian)
Created attachment 8456790 [details] [diff] [review]
[06] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::VolumeControl| (under bluetooth2/)
Attachment #8456790 - Flags: review?(btian)
Created attachment 8456791 [details] [diff] [review]
[07] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::DeviceStatusNotification| (under bluetooth2/)
Attachment #8456791 - Flags: review?(btian)
Created attachment 8456792 [details] [diff] [review]
[08] Bug 1038645: Asynchronous Bluetooth Handsfree response methods (under bluetooth2/)
Attachment #8456792 - Flags: review?(btian)
Created attachment 8456794 [details] [diff] [review]
[09] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange| (under bluetooth2/)
Attachment #8456794 - Flags: review?(btian)
Attachment #8456794 - Attachment description: [0] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange| (under bluetooth2/) → [09] Bug 1038645: Asynchronous |BluetoothHandsfreeInterface::PhoneStateChange| (under bluetooth2/)

Comment 10

4 years ago
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

4 years ago
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.
Attachment #8456784 - Flags: review?(btian) → review+

Comment 12

4 years ago
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

4 years ago
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.
Attachment #8456786 - Flags: review?(btian) → review+

Comment 14

4 years ago
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.
Attachment #8456788 - Flags: review?(btian) → review+

Comment 15

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

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

LGTM.
Attachment #8456790 - Flags: review?(btian) → review+

Comment 16

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

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

LGTM.
Attachment #8456791 - Flags: review?(btian) → review+

Comment 17

4 years ago
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.
Attachment #8456792 - Flags: review?(btian) → review+

Comment 18

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

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

LGTM.
Attachment #8456794 - Flags: review?(btian) → review+
(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.
Created attachment 8459510 [details] [diff] [review]
[01] Bug 1038645: Asynchronous starting and stopping of profile managers (under bluetooth2/) (v2)

Changes since v1:

  - added missing callbacks during profile initialization
  - comment fixes
Attachment #8456783 - Attachment is obsolete: true
Attachment #8456783 - Flags: review?(btian)
Attachment #8459510 - Flags: review?(btian)
Created attachment 8459511 [details] [diff] [review]
[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
Attachment #8456785 - Attachment is obsolete: true
Attachment #8456785 - Flags: review?(btian)
Attachment #8459511 - Flags: review?(btian)

Comment 23

4 years ago
(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

4 years ago
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.
Attachment #8459511 - Flags: review?(btian) → review+

Comment 25

4 years ago
(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].
Flags: needinfo?(tzimmermann)
> 
> 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.
Flags: needinfo?(tzimmermann)

Comment 27

4 years ago
(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

4 years ago
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!
Attachment #8459510 - Flags: review?(btian) → review+

Updated

4 years ago
Blocks: 1019376
You need to log in before you can comment on or make changes to this bug.