1053804 - [Bluetooth] Convert HFP callbacks to backend-neutral interface

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

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

Attachment: [01] Bug 1053804: Add Bluetooth Handsfree notifications

Comment 2

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

Attachment: [02] Bug 1053804: Implement Bluetooth Handsfree notifications

Comment 3

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

Attachment: [03] Bug 1053804: Use Bluetooth Handsfree notifications

Comment 4

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

Attachment: [04] Bug 1053804: Integrate runnables into Handsfree notifications

Comment 5

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

Attachment: [05] Bug 1053804: Cleanup Bluetooth Handsfree manager

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8473572 [details] [diff] [review]
[01] Bug 1053804: Add Bluetooth Handsfree notifications

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

f=me with nit addressed.

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +136,5 @@
> +  CopsNotification()
> +  { }
> +
> +  virtual void
> +  AtClccNotification()

nit: Rename to |ClccNotification| for consistency with |CopsNotification|, |CindNotification|, and |CnumNotification|.

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8473573 [details] [diff] [review]
[02] Bug 1053804: Implement Bluetooth Handsfree notifications

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

f=me with comment addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1729,5 @@
> +  mConnectionState = aState;
> +
> +  if (aState == HFP_CONNECTION_STATE_SLC_CONNECTED) {
> +    mDeviceAddress = aBdAddress;
> +    BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_CONN_STATE_CHANGED,

This macro was created to run main-thread only code. Please open a followup bug to remove this macro since all callbacks become running on main thread.

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8473574 [details] [diff] [review]
[03] Bug 1053804: Use Bluetooth Handsfree notifications

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

LGTM

Comment 9

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #7)
> > +    BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_CONN_STATE_CHANGED,
> 
> This macro was created to run main-thread only code. Please open a followup
> bug to remove this macro since all callbacks become running on main thread.

Ignore this comment since patch [04] does it.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8473572 [details] [diff] [review]
[01] Bug 1053804: Add Bluetooth Handsfree notifications

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

::: dom/bluetooth/BluetoothCommon.h
@@ +305,5 @@
>    HFP_NETWORK_STATE_AVAILABLE
>  };
>  
> +enum BluetoothHandsfreeNoiseReductionState {
> +  HFP_NOISE_REDUCTION_STOPPED,

nit: Noise Reduction and Echo Cancellation, so I prefer to keep Nrec wording. Otherwise EC is missing. What do you think?

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8473575 [details] [diff] [review]
[04] Bug 1053804: Integrate runnables into Handsfree notifications

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

f=me with nit addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1854,3 @@
>      nsAutoCString newMsg("ATD");
>      newMsg += StringHead(message, message.Length() - 1);
> +                        NotifyDialer(NS_ConvertUTF8toUTF16(newMsg));

nit: indent too long.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8473574 [details] [diff] [review]
[03] Bug 1053804: Use Bluetooth Handsfree notifications

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +2342,5 @@
> +    .cops_cmd_cb = BluetoothHandsfreeCallback::Cops,
> +    .clcc_cmd_cb = BluetoothHandsfreeCallback::AtClcc,
> +    .unknown_at_cmd_cb = BluetoothHandsfreeCallback::UnknownAt,
> +    .key_pressed_cmd_cb = BluetoothHandsfreeCallback::KeyPressed
> +  };

CAF repo defines extra |codec_negotiated_callback| here again. I guess we need to deal with it. :(

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8473576 [details] [diff] [review]
[05] Bug 1053804: Cleanup Bluetooth Handsfree manager

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

f=me with comment addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +62,1 @@
>  enum MainThreadTaskCmd {

Remove |MainThreaTaskCmd| and |BluetoothHfpManager::MainThreadTask| since only macro |BT_HF_PROCESS_CB| uses them.

@@ +1578,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsCString atString(aAtString);
>  
>    BT_LOGR("[%s]", atString.get());

Print |NS_ConvertUTF8toUTF16(aAtString).get()| directly and remove redundant |atString|.

@@ +1580,5 @@
>    nsCString atString(aAtString);
>  
>    BT_LOGR("[%s]", atString.get());
>  
>    NS_ENSURE_TRUE_VOID(sBluetoothHfpInterface);

Remove this check since |SendResponse| checks |sBluetoothHfpInterface| inside.

Comment 14

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #10)
> Comment on attachment 8473572 [details] [diff] [review]
> [01] Bug 1053804: Add Bluetooth Handsfree notifications
> 
> Review of attachment 8473572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothCommon.h
> @@ +305,5 @@
> >    HFP_NETWORK_STATE_AVAILABLE
> >  };
> >  
> > +enum BluetoothHandsfreeNoiseReductionState {
> > +  HFP_NOISE_REDUCTION_STOPPED,
> 
> nit: Noise Reduction and Echo Cancellation, so I prefer to keep Nrec
> wording. Otherwise EC is missing. What do you think?

Shall we call this 'NREC' in all capital letters then?

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #10)
> > Comment on attachment 8473572 [details] [diff] [review]
> > [01] Bug 1053804: Add Bluetooth Handsfree notifications
> > 
> > Review of attachment 8473572 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/BluetoothCommon.h
> > @@ +305,5 @@
> > >    HFP_NETWORK_STATE_AVAILABLE
> > >  };
> > >  
> > > +enum BluetoothHandsfreeNoiseReductionState {
> > > +  HFP_NOISE_REDUCTION_STOPPED,
> > 
> > nit: Noise Reduction and Echo Cancellation, so I prefer to keep Nrec
> > wording. Otherwise EC is missing. What do you think?
> 
> Shall we call this 'NREC' in all capital letters then?
That will be fine.

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8473576 [details] [diff] [review]
[05] Bug 1053804: Cleanup Bluetooth Handsfree manager

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

Let's fix the problem that Ben mentioned :)

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> Comment on attachment 8473574 [details] [diff] [review]
> [03] Bug 1053804: Use Bluetooth Handsfree notifications
> 
> Review of attachment 8473574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +2342,5 @@
> > +    .cops_cmd_cb = BluetoothHandsfreeCallback::Cops,
> > +    .clcc_cmd_cb = BluetoothHandsfreeCallback::AtClcc,
> > +    .unknown_at_cmd_cb = BluetoothHandsfreeCallback::UnknownAt,
> > +    .key_pressed_cmd_cb = BluetoothHandsfreeCallback::KeyPressed
> > +  };
> 
> CAF repo defines extra |codec_negotiated_callback| here again. I guess we
> need to deal with it. :(

I remembered that if we miss the last designated initializer, we will have compiling error, but if we don't use designated initializers, we won't see compiling error.

Comment 18

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #17)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> > Comment on attachment 8473574 [details] [diff] [review]
> > [03] Bug 1053804: Use Bluetooth Handsfree notifications
> > 
> > Review of attachment 8473574 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> > @@ +2342,5 @@
> > > +    .cops_cmd_cb = BluetoothHandsfreeCallback::Cops,
> > > +    .clcc_cmd_cb = BluetoothHandsfreeCallback::AtClcc,
> > > +    .unknown_at_cmd_cb = BluetoothHandsfreeCallback::UnknownAt,
> > > +    .key_pressed_cmd_cb = BluetoothHandsfreeCallback::KeyPressed
> > > +  };
> > 
> > CAF repo defines extra |codec_negotiated_callback| here again. I guess we
> > need to deal with it. :(
> 
> I remembered that if we miss the last designated initializer, we will have
> compiling error, but if we don't use designated initializers, we won't see
> compiling error.

I was able to build these patches for flame-kk without changes. \o/ Can we keep them?

Comment 19

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

Attachment: [01] Bug 1053804: Add Bluetooth Handsfree notifications (v2)

Changes since v1:

  - fixed |AtClccNotification|
  - use NREC

Comment 20

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

Attachment: [02] Bug 1053804: Implement Bluetooth Handsfree notifications (v2)

Changes since v1:

  - rebased onto [01] (v2)

Comment 21

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

Comment on attachment 8473574 [details] [diff] [review]
[03] Bug 1053804: Use Bluetooth Handsfree notifications

I'm reopening this for review because I found that no special handling for flame-kk is required.

Comment 22

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

Attachment: [03] Bug 1053804: Use Bluetooth Handsfree notifications (v2)

Changes since v1:

  - rebased onto [01] (v2)

Comment 23

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

Attachment: [04] Bug 1053804: Integrate runnables into Handsfree notifications (v2)

Changes since v2:

  - fixed indention

Comment 24

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

Attachment: [05] Bug 1053804: Cleanup Bluetooth Handsfree manager (v2)

Changes since v1:

  - removed |atString|
  - removed duplicated test for |sBluetoothHfpInterface|

Comment 25

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

Attachment: [06] Bug 1053804: Replace |MainThreadTask| by |CloseScoRunnable|

I cannot completely remove the functionality of MainThreadTask, so I moved the last used bits into a separate class and deleted the rest.

Comment 26

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

https://hg.mozilla.org/integration/b2g-inbound/rev/adc4a9593007
https://hg.mozilla.org/integration/b2g-inbound/rev/edfc55422f6f
https://hg.mozilla.org/integration/b2g-inbound/rev/a687c6ead950
https://hg.mozilla.org/integration/b2g-inbound/rev/8e164a70118b
https://hg.mozilla.org/integration/b2g-inbound/rev/c0243ec27fde
https://hg.mozilla.org/integration/b2g-inbound/rev/246722701237

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

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/adc4a9593007
https://hg.mozilla.org/mozilla-central/rev/edfc55422f6f
https://hg.mozilla.org/mozilla-central/rev/a687c6ead950
https://hg.mozilla.org/mozilla-central/rev/8e164a70118b
https://hg.mozilla.org/mozilla-central/rev/c0243ec27fde
https://hg.mozilla.org/mozilla-central/rev/246722701237